|
| 1 | +From ff37657e4d94cf4db80ab9652ae8e6acb84f3019 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Stefan Eissing <stefan@eissing.org> |
| 3 | +Date: Fri, 11 Apr 2025 12:05:05 +0200 |
| 4 | +Subject: [PATCH] cpool/cshutdown: force close connections under pressure |
| 5 | + |
| 6 | +when CURLMOPT_MAX_HOST_CONNECTIONS or CURLMOPT_MAX_TOTAL_CONNECTIONS |
| 7 | +limits are reached, force close connections in shutdown to go below |
| 8 | +limit when possible. |
| 9 | + |
| 10 | +Fixes #17020 |
| 11 | +Reported-by: Fujii Hironori |
| 12 | +Closes #17022 |
| 13 | +--- |
| 14 | + lib/conncache.c | 68 +++++++++++++++++++++------------- |
| 15 | + lib/cshutdn.c | 30 +++++++++++++-- |
| 16 | + lib/cshutdn.h | 6 +++ |
| 17 | + lib/url.c | 9 +++-- |
| 18 | + tests/http/test_19_shutdown.py | 31 +++++++++++++++- |
| 19 | + 5 files changed, 109 insertions(+), 35 deletions(-) |
| 20 | + |
| 21 | +diff --git a/lib/conncache.c b/lib/conncache.c |
| 22 | +index fe0b07dcb..072fdd44f 100644 |
| 23 | +--- a/lib/conncache.c |
| 24 | ++++ b/lib/conncache.c |
| 25 | +@@ -375,24 +375,33 @@ int Curl_cpool_check_limits(struct Curl_easy *data, |
| 26 | + |
| 27 | + bundle = cpool_find_bundle(cpool, conn); |
| 28 | + live = bundle ? Curl_llist_count(&bundle->conns) : 0; |
| 29 | +- shutdowns = Curl_cshutdn_dest_count(data, conn->destination); |
| 30 | +- while(!shutdowns && bundle && live >= dest_limit) { |
| 31 | +- struct connectdata *oldest_idle = NULL; |
| 32 | +- /* The bundle is full. Extract the oldest connection that may |
| 33 | +- * be removed now, if there is one. */ |
| 34 | +- oldest_idle = cpool_bundle_get_oldest_idle(bundle); |
| 35 | +- if(!oldest_idle) |
| 36 | ++ shutdowns = Curl_cshutdn_dest_count(data, conn->destination); |
| 37 | ++ while((live + shutdowns) >= dest_limit) { |
| 38 | ++ if(shutdowns) { |
| 39 | ++ /* close one connection in shutdown right away, if we can */ |
| 40 | ++ if(!Curl_cshutdn_close_oldest(data, conn->destination)) |
| 41 | ++ break; |
| 42 | ++ } |
| 43 | ++ else if(!bundle) |
| 44 | + break; |
| 45 | +- /* disconnect the old conn and continue */ |
| 46 | +- CURL_TRC_M(data, "Discarding connection #%" |
| 47 | +- FMT_OFF_T " from %zu to reach destination " |
| 48 | +- "limit of %zu", oldest_idle->connection_id, |
| 49 | +- Curl_llist_count(&bundle->conns), dest_limit); |
| 50 | +- Curl_conn_terminate(cpool->idata, oldest_idle, FALSE); |
| 51 | +- |
| 52 | +- /* in case the bundle was destroyed in disconnect, look it up again */ |
| 53 | +- bundle = cpool_find_bundle(cpool, conn); |
| 54 | +- live = bundle ? Curl_llist_count(&bundle->conns) : 0; |
| 55 | ++ else { |
| 56 | ++ struct connectdata *oldest_idle = NULL; |
| 57 | ++ /* The bundle is full. Extract the oldest connection that may |
| 58 | ++ * be removed now, if there is one. */ |
| 59 | ++ oldest_idle = cpool_bundle_get_oldest_idle(bundle); |
| 60 | ++ if(!oldest_idle) |
| 61 | ++ break; |
| 62 | ++ /* disconnect the old conn and continue */ |
| 63 | ++ CURL_TRC_M(data, "Discarding connection #%" |
| 64 | ++ FMT_OFF_T " from %zu to reach destination " |
| 65 | ++ "limit of %zu", oldest_idle->connection_id, |
| 66 | ++ Curl_llist_count(&bundle->conns), dest_limit); |
| 67 | ++ Curl_conn_terminate(cpool->idata, oldest_idle, FALSE); |
| 68 | ++ |
| 69 | ++ /* in case the bundle was destroyed in disconnect, look it up again */ |
| 70 | ++ bundle = cpool_find_bundle(cpool, conn); |
| 71 | ++ live = bundle ? Curl_llist_count(&bundle->conns) : 0; |
| 72 | ++ } |
| 73 | + shutdowns = Curl_cshutdn_dest_count(cpool->idata, conn->destination); |
| 74 | + } |
| 75 | + if((live + shutdowns) >= dest_limit) { |
| 76 | +@@ -404,15 +413,22 @@ int Curl_cpool_check_limits(struct Curl_easy *data, |
| 77 | + if(total_limit) { |
| 78 | + shutdowns = Curl_cshutdn_count(cpool->idata); |
| 79 | + while((cpool->num_conn + shutdowns) >= total_limit) { |
| 80 | +- struct connectdata *oldest_idle = cpool_get_oldest_idle(cpool); |
| 81 | +- if(!oldest_idle) |
| 82 | +- break; |
| 83 | +- /* disconnect the old conn and continue */ |
| 84 | +- CURL_TRC_M(data, "Discarding connection #%" |
| 85 | +- FMT_OFF_T " from %zu to reach total " |
| 86 | +- "limit of %zu", |
| 87 | +- oldest_idle->connection_id, cpool->num_conn, total_limit); |
| 88 | +- Curl_conn_terminate(cpool->idata, oldest_idle, FALSE); |
| 89 | ++ if(shutdowns) { |
| 90 | ++ /* close one connection in shutdown right away, if we can */ |
| 91 | ++ if(!Curl_cshutdn_close_oldest(data, NULL)) |
| 92 | ++ break; |
| 93 | ++ } |
| 94 | ++ else { |
| 95 | ++ struct connectdata *oldest_idle = cpool_get_oldest_idle(cpool); |
| 96 | ++ if(!oldest_idle) |
| 97 | ++ break; |
| 98 | ++ /* disconnect the old conn and continue */ |
| 99 | ++ CURL_TRC_M(data, "Discarding connection #%" |
| 100 | ++ FMT_OFF_T " from %zu to reach total " |
| 101 | ++ "limit of %zu", |
| 102 | ++ oldest_idle->connection_id, cpool->num_conn, total_limit); |
| 103 | ++ Curl_conn_terminate(cpool->idata, oldest_idle, FALSE); |
| 104 | ++ } |
| 105 | + shutdowns = Curl_cshutdn_count(cpool->idata); |
| 106 | + } |
| 107 | + if((cpool->num_conn + shutdowns) >= total_limit) { |
| 108 | +diff --git a/lib/cshutdn.c b/lib/cshutdn.c |
| 109 | +index 45581bd08..83972e375 100644 |
| 110 | +--- a/lib/cshutdn.c |
| 111 | ++++ b/lib/cshutdn.c |
| 112 | +@@ -166,7 +166,9 @@ void Curl_cshutdn_terminate(struct Curl_easy *data, |
| 113 | + * not done so already. */ |
| 114 | + cshutdn_run_once(admin, conn, &done); |
| 115 | + } |
| 116 | +- CURL_TRC_M(admin, "[SHUTDOWN] closing connection"); |
| 117 | ++ CURL_TRC_M(admin, "[SHUTDOWN] %sclosing connection #%" FMT_OFF_T, |
| 118 | ++ conn->bits.shutdown_filters ? "" : "force ", |
| 119 | ++ conn->connection_id); |
| 120 | + Curl_conn_close(admin, SECONDARYSOCKET); |
| 121 | + Curl_conn_close(admin, FIRSTSOCKET); |
| 122 | + Curl_detach_connection(admin); |
| 123 | +@@ -181,13 +183,21 @@ void Curl_cshutdn_terminate(struct Curl_easy *data, |
| 124 | + } |
| 125 | + } |
| 126 | + |
| 127 | +-static void cshutdn_destroy_oldest(struct cshutdn *cshutdn, |
| 128 | +- struct Curl_easy *data) |
| 129 | ++static bool cshutdn_destroy_oldest(struct cshutdn *cshutdn, |
| 130 | ++ struct Curl_easy *data, |
| 131 | ++ const char *destination) |
| 132 | + { |
| 133 | + struct Curl_llist_node *e; |
| 134 | + struct connectdata *conn; |
| 135 | + |
| 136 | + e = Curl_llist_head(&cshutdn->list); |
| 137 | ++ while(e) { |
| 138 | ++ conn = Curl_node_elem(e); |
| 139 | ++ if(!destination || !strcmp(destination, conn->destination)) |
| 140 | ++ break; |
| 141 | ++ e = Curl_node_next(e); |
| 142 | ++ } |
| 143 | ++ |
| 144 | + if(e) { |
| 145 | + SIGPIPE_VARIABLE(pipe_st); |
| 146 | + conn = Curl_node_elem(e); |
| 147 | +@@ -196,7 +206,19 @@ static void cshutdn_destroy_oldest(struct cshutdn *cshutdn, |
| 148 | + sigpipe_apply(data, &pipe_st); |
| 149 | + Curl_cshutdn_terminate(data, conn, FALSE); |
| 150 | + sigpipe_restore(&pipe_st); |
| 151 | ++ return TRUE; |
| 152 | ++ } |
| 153 | ++ return FALSE; |
| 154 | ++} |
| 155 | ++ |
| 156 | ++bool Curl_cshutdn_close_oldest(struct Curl_easy *data, |
| 157 | ++ const char *destination) |
| 158 | ++{ |
| 159 | ++ if(data && data->multi) { |
| 160 | ++ struct cshutdn *csd = &data->multi->cshutdn; |
| 161 | ++ return cshutdn_destroy_oldest(csd, data, destination); |
| 162 | + } |
| 163 | ++ return FALSE; |
| 164 | + } |
| 165 | + |
| 166 | + #define NUM_POLLS_ON_STACK 10 |
| 167 | +@@ -414,7 +436,7 @@ void Curl_cshutdn_add(struct cshutdn *cshutdn, |
| 168 | + (conns_in_pool + Curl_llist_count(&cshutdn->list)))) { |
| 169 | + CURL_TRC_M(data, "[SHUTDOWN] discarding oldest shutdown connection " |
| 170 | + "due to connection limit of %zu", max_total); |
| 171 | +- cshutdn_destroy_oldest(cshutdn, data); |
| 172 | ++ cshutdn_destroy_oldest(cshutdn, data, NULL); |
| 173 | + } |
| 174 | + |
| 175 | + if(cshutdn->multi->socket_cb) { |
| 176 | +diff --git a/lib/cshutdn.h b/lib/cshutdn.h |
| 177 | +index 202e86983..7b9514447 100644 |
| 178 | +--- a/lib/cshutdn.h |
| 179 | ++++ b/lib/cshutdn.h |
| 180 | +@@ -75,6 +75,12 @@ size_t Curl_cshutdn_count(struct Curl_easy *data); |
| 181 | + size_t Curl_cshutdn_dest_count(struct Curl_easy *data, |
| 182 | + const char *destination); |
| 183 | + |
| 184 | ++/* Close the oldest connection in shutdown to destination or, |
| 185 | ++ * when destination is NULL for any destination. |
| 186 | ++ * Return TRUE if a connection has been closed. */ |
| 187 | ++bool Curl_cshutdn_close_oldest(struct Curl_easy *data, |
| 188 | ++ const char *destination); |
| 189 | ++ |
| 190 | + /* Add a connection to have it shut down. Will terminate the oldest |
| 191 | + * connection when total connection limit of multi is being reached. */ |
| 192 | + void Curl_cshutdn_add(struct cshutdn *cshutdn, |
| 193 | +diff --git a/lib/url.c b/lib/url.c |
| 194 | +index 2125a97af..d94a29375 100644 |
| 195 | +--- a/lib/url.c |
| 196 | ++++ b/lib/url.c |
| 197 | +@@ -3597,10 +3597,12 @@ static CURLcode create_conn(struct Curl_easy *data, |
| 198 | + conn->bits.tls_enable_alpn = TRUE; |
| 199 | + } |
| 200 | + |
| 201 | +- if(waitpipe) |
| 202 | ++ if(waitpipe) { |
| 203 | + /* There is a connection that *might* become usable for multiplexing |
| 204 | + "soon", and we wait for that */ |
| 205 | ++ infof(data, "Waiting on connection to negotiate possible multiplexing."); |
| 206 | + connections_available = FALSE; |
| 207 | ++ } |
| 208 | + else { |
| 209 | + switch(Curl_cpool_check_limits(data, conn)) { |
| 210 | + case CPOOL_LIMIT_DEST: |
| 211 | +@@ -3614,7 +3616,8 @@ static CURLcode create_conn(struct Curl_easy *data, |
| 212 | + else |
| 213 | + #endif |
| 214 | + { |
| 215 | +- infof(data, "No connections available in cache"); |
| 216 | ++ infof(data, "No connections available, total of %ld reached.", |
| 217 | ++ data->multi->max_total_connections); |
| 218 | + connections_available = FALSE; |
| 219 | + } |
| 220 | + break; |
| 221 | +@@ -3624,8 +3627,6 @@ static CURLcode create_conn(struct Curl_easy *data, |
| 222 | + } |
| 223 | + |
| 224 | + if(!connections_available) { |
| 225 | +- infof(data, "No connections available."); |
| 226 | +- |
| 227 | + Curl_conn_free(data, conn); |
| 228 | + *in_connect = NULL; |
| 229 | + |
| 230 | +diff --git a/tests/http/test_19_shutdown.py b/tests/http/test_19_shutdown.py |
| 231 | +index ea6839135..fad1314fc 100644 |
| 232 | +--- a/tests/http/test_19_shutdown.py |
| 233 | ++++ b/tests/http/test_19_shutdown.py |
| 234 | +@@ -153,7 +153,7 @@ class TestShutdown: |
| 235 | + r.check_response(http_status=200, count=count) |
| 236 | + # check that we closed all connections |
| 237 | + closings = [line for line in r.trace_lines |
| 238 | +- if re.match(r'.*SHUTDOWN\] closing', line)] |
| 239 | ++ if re.match(r'.*SHUTDOWN\] (force )?closing', line)] |
| 240 | + assert len(closings) == count, f'{closings}' |
| 241 | + # check that all connection sockets were removed from event |
| 242 | + removes = [line for line in r.trace_lines |
| 243 | +@@ -180,3 +180,32 @@ class TestShutdown: |
| 244 | + shutdowns = [line for line in r.trace_lines |
| 245 | + if re.match(r'.*SHUTDOWN\] shutdown, done=1', line)] |
| 246 | + assert len(shutdowns) == 1, f'{shutdowns}' |
| 247 | ++ |
| 248 | ++ # run connection pressure, many small transfers, not reusing connections, |
| 249 | ++ # limited total |
| 250 | ++ @pytest.mark.parametrize("proto", ['http/1.1']) |
| 251 | ++ def test_19_07_shutdown_by_curl(self, env: Env, httpd, proto): |
| 252 | ++ if not env.curl_is_debug(): |
| 253 | ++ pytest.skip('only works for curl debug builds') |
| 254 | ++ count = 500 |
| 255 | ++ docname = 'data.json' |
| 256 | ++ url = f'https://localhost:{env.https_port}/{docname}' |
| 257 | ++ client = LocalClient(name='hx-download', env=env, run_env={ |
| 258 | ++ 'CURL_GRACEFUL_SHUTDOWN': '2000', |
| 259 | ++ 'CURL_DEBUG': 'ssl,multi' |
| 260 | ++ }) |
| 261 | ++ if not client.exists(): |
| 262 | ++ pytest.skip(f'example client not built: {client.name}') |
| 263 | ++ r = client.run(args=[ |
| 264 | ++ '-n', f'{count}', #that many transfers |
| 265 | ++ '-f', # forbid conn reuse |
| 266 | ++ '-m', '10', # max parallel |
| 267 | ++ '-T', '5', # max total conns at a time |
| 268 | ++ '-V', proto, |
| 269 | ++ url |
| 270 | ++ ]) |
| 271 | ++ r.check_exit_code(0) |
| 272 | ++ shutdowns = [line for line in r.trace_lines |
| 273 | ++ if re.match(r'.*SHUTDOWN\] shutdown, done=1', line)] |
| 274 | ++ # we see less clean shutdowns as total limit forces early closes |
| 275 | ++ assert len(shutdowns) < count, f'{shutdowns}' |
| 276 | +-- |
| 277 | +2.49.0.windows.1 |
| 278 | + |
0 commit comments