conncache: apply multi limits to transfers using a shared pool

- Determine max host and total connection limits from the transfer's own
  multi handle and test that it works for shared connections.

Prior to this change, since df67269 (precedes 8.13.0),
Curl_cpool_check_limits() took the limits from the multi handle that
owns the pool. A share-owned pool is not owned by any multi, so the
limit check always passed.

Prior to df67269 the limits came from the transfer's own multi handle.

Fixes https://github.com/curl/curl/issues/22265
Closes https://github.com/curl/curl/pull/22266

diff --git a/docs/libcurl/opts/CURLSHOPT_SHARE.md b/docs/libcurl/opts/CURLSHOPT_SHARE.md
index 69b59779ad6b3ffeb8739bc92a62e899003d8b06..827ba77ab60754f9bd6356849cd3d1dfeaf76fc9 100644 (file)
--- a/docs/libcurl/opts/CURLSHOPT_SHARE.md
+++ b/docs/libcurl/opts/CURLSHOPT_SHARE.md
@@ -79,6 +79,11 @@ in different threads using a shared connection.
 Note that when you use the multi interface, all easy handles added to the same
 multi handle share the connection cache by default without using this option.
 
+Connection limits set with CURLMOPT_MAX_HOST_CONNECTIONS(3) and
+CURLMOPT_MAX_TOTAL_CONNECTIONS(3) also apply to transfers using a shared
+connection cache. Each transfer applies the limits of the multi handle it
+runs on to the shared cache.
+
 ## CURL_LOCK_DATA_PSL
 
 The Public Suffix List stored in the share object is made available to all

diff --git a/lib/conncache.c b/lib/conncache.c
index 6fc29be4b4e207551a8f84acd196b07637f77dd2..17e8d715b0dcacd3441de80ab3669ecc2dee1250 100644 (file)
--- a/lib/conncache.c
+++ b/lib/conncache.c
@@ -367,6 +367,22 @@ static struct connectdata *cpool_get_oldest_idle(struct cpool *cpool,
   return oldest_idle;
 }
 
+/* Evict an idle connection to make room in the pool. A pool owned by
+ * a share has no multi that could perform a controlled shutdown of the
+ * connection; terminate it right away. Otherwise, hand it to the
+ * transfer's multi for shutdown. Expects the pool to be locked. */
+static void cpool_evict_conn(struct cpool *cpool,
+                             struct Curl_easy *data,
+                             struct connectdata *conn)
+{
+  if(cpool->share) {
+    cpool_remove_conn(cpool, conn);
+    Curl_cshutdn_terminate(cpool->idata, conn, TRUE);
+  }
+  else
+    Curl_conn_terminate(data, conn, FALSE);
+}
+
 int Curl_cpool_check_limits(struct Curl_easy *data,
                             struct connectdata *conn)
 {
@@ -380,9 +396,10 @@ int Curl_cpool_check_limits(struct Curl_easy *data,
   if(!cpool)
     return CPOOL_LIMIT_OK;
 
-  if(cpool->idata->multi) {
-    dest_limit = cpool->idata->multi->max_host_connections;
-    total_limit = cpool->idata->multi->max_total_connections;
+  /* multi determines the limits, no matter who owns the pool */
+  if(data->multi) {
+    dest_limit = data->multi->max_host_connections;
+    total_limit = data->multi->max_total_connections;
   }
 
   if(!dest_limit && !total_limit)
@@ -416,13 +433,13 @@ int Curl_cpool_check_limits(struct Curl_easy *data,
                    " from %zu to reach destination limit of %zu",
                    oldest_idle->connection_id,
                    Curl_llist_count(&bundle->conns), dest_limit);
-        Curl_conn_terminate(cpool->idata, oldest_idle, FALSE);
+        cpool_evict_conn(cpool, data, oldest_idle);
 
         /* in case the bundle was destroyed in disconnect, look it up again */
         bundle = cpool_find_bundle(cpool, conn);
         live = bundle ? Curl_llist_count(&bundle->conns) : 0;
       }
-      shutdowns = Curl_cshutdn_dest_count(cpool->idata, conn->destination);
+      shutdowns = Curl_cshutdn_dest_count(data, conn->destination);
     }
     if((live + shutdowns) >= dest_limit) {
       res = CPOOL_LIMIT_DEST;
@@ -431,7 +448,7 @@ int Curl_cpool_check_limits(struct Curl_easy *data,
   }
 
   if(total_limit) {
-    shutdowns = Curl_cshutdn_count(cpool->idata);
+    shutdowns = Curl_cshutdn_count(data);
     while((cpool->num_conn + shutdowns) >= total_limit) {
       if(shutdowns) {
         /* close one connection in shutdown right away, if we can */
@@ -448,9 +465,9 @@ int Curl_cpool_check_limits(struct Curl_easy *data,
                    FMT_OFF_T " from %zu to reach total "
                    "limit of %zu",
                    oldest_idle->connection_id, cpool->num_conn, total_limit);
-        Curl_conn_terminate(cpool->idata, oldest_idle, FALSE);
+        cpool_evict_conn(cpool, data, oldest_idle);
       }
-      shutdowns = Curl_cshutdn_count(cpool->idata);
+      shutdowns = Curl_cshutdn_count(data);
     }
     if((cpool->num_conn + shutdowns) >= total_limit) {
       res = CPOOL_LIMIT_TOTAL;
@@ -582,7 +599,7 @@ bool Curl_cpool_conn_now_idle(struct Curl_easy *data,
       oldest_idle = cpool_get_oldest_idle(cpool, Curl_pgrs_now(data));
       kept = (oldest_idle != conn);
       if(oldest_idle) {
-        Curl_conn_terminate(data, oldest_idle, FALSE);
+        cpool_evict_conn(cpool, data, oldest_idle);
       }
     }
     if(do_lock)

diff --git a/tests/http/test_02_download.py b/tests/http/test_02_download.py
index c75f932b7218e11cd5bcc5d48012409bfa224481..de2f1a3e0935a1bbbad4fda87293a07c575aaebe 100644 (file)
--- a/tests/http/test_02_download.py
+++ b/tests/http/test_02_download.py
@@ -586,7 +586,8 @@ class TestDownload:
 
     @pytest.mark.parametrize("proto", Env.http_h1_h2_protos())
     @pytest.mark.parametrize("max_host_conns", [0, 1, 5])
-    def test_02_33_max_host_conns(self, env: Env, httpd, nghttpx, proto, max_host_conns):
+    @pytest.mark.parametrize("share_connect", [False, True])
+    def test_02_33_max_host_conns(self, env: Env, httpd, nghttpx, proto, max_host_conns, share_connect):
         if not env.curl_is_debug():
             pytest.skip('only works for curl debug builds')
         if not env.curl_is_verbose():
@@ -601,6 +602,7 @@ class TestDownload:
         client = LocalClient(name='cli_hx_download', env=env, run_env=run_env)
         if not client.exists():
             pytest.skip(f'example client not built: {client.name}')
+        extra_args = ['-S'] if share_connect else []  # share connections
         r = client.run(args=[
              '-n', f'{count}',
              '-m', f'{max_parallel}',
@@ -608,8 +610,7 @@ class TestDownload:
              '-x',  # always use a fresh connection
              '-M',  str(max_host_conns),  # limit conns per host
              '-r', f'{env.domain1}:{port}:127.0.0.1',
-             '-V', proto, url
-        ])
+        ] + extra_args + ['-V', proto, url])
         r.check_exit_code(0)
         srcfile = os.path.join(httpd.docs_dir, docname)
         self.check_downloads(client, srcfile, count)
@@ -625,7 +626,8 @@ class TestDownload:
 
     @pytest.mark.parametrize("proto", Env.http_h1_h2_protos())
     @pytest.mark.parametrize("max_total_conns", [0, 1, 5])
-    def test_02_34_max_total_conns(self, env: Env, httpd, nghttpx, proto, max_total_conns):
+    @pytest.mark.parametrize("share_connect", [False, True])
+    def test_02_34_max_total_conns(self, env: Env, httpd, nghttpx, proto, max_total_conns, share_connect):
         if not env.curl_is_debug():
             pytest.skip('only works for curl debug builds')
         if not env.curl_is_verbose():
@@ -640,6 +642,7 @@ class TestDownload:
         client = LocalClient(name='cli_hx_download', env=env, run_env=run_env)
         if not client.exists():
             pytest.skip(f'example client not built: {client.name}')
+        extra_args = ['-S'] if share_connect else []  # share connections
         r = client.run(args=[
              '-n', f'{count}',
              '-m', f'{max_parallel}',
@@ -647,8 +650,7 @@ class TestDownload:
              '-x',  # always use a fresh connection
              '-T',  str(max_total_conns),  # limit total connections
              '-r', f'{env.domain1}:{port}:127.0.0.1',
-             '-V', proto, url
-        ])
+        ] + extra_args + ['-V', proto, url])
         r.check_exit_code(0)
         srcfile = os.path.join(httpd.docs_dir, docname)
         self.check_downloads(client, srcfile, count)

diff --git a/tests/http/test_19_shutdown.py b/tests/http/test_19_shutdown.py
index 4ed9ad27fb01e9a3a5d244a89ed95b760eefec8a..f8a948acd661aadd621470c9dd3012766e29f717 100644 (file)
--- a/tests/http/test_19_shutdown.py
+++ b/tests/http/test_19_shutdown.py
@@ -185,7 +185,8 @@ class TestShutdown:
     # run connection pressure, many small transfers, not reusing connections,
     # limited total
     @pytest.mark.parametrize("proto", ['http/1.1'])
-    def test_19_07_shutdown_by_curl(self, env: Env, httpd, proto):
+    @pytest.mark.parametrize("share_connect", [False, True])
+    def test_19_07_shutdown_by_curl(self, env: Env, httpd, proto, share_connect):
         if not env.curl_is_debug():
             pytest.skip('only works for curl debug builds')
         count = 500
@@ -197,15 +198,14 @@ class TestShutdown:
         })
         if not client.exists():
             pytest.skip(f'example client not built: {client.name}')
+        extra_args = ['-S'] if share_connect else []  # share connections
         r = client.run(args=[
              '-n', f'{count}',  # that many transfers
              '-C', env.ca.cert_file,
              '-f',  # forbid conn reuse
              '-m', '10',  # max parallel
              '-T', '5',  # max total conns at a time
-             '-V', proto,
-             url
-        ])
+        ] + extra_args + ['-V', proto, url])
         r.check_exit_code(0)
         shutdowns = [line for line in r.trace_lines
                      if re.match(r'.*SHUTDOWN] shutdown, done=1', line)]

diff --git a/tests/libtest/cli_hx_download.c b/tests/libtest/cli_hx_download.c
index f964067f1550c9361d357321858a45110b54dce1..6ad18e124a9469a98d36417dea3752683e689262 100644 (file)
--- a/tests/libtest/cli_hx_download.c
+++ b/tests/libtest/cli_hx_download.c
@@ -285,6 +285,7 @@ static void usage_hx_download(const char *msg)
     "  -M number  max concurrent connections to a host\n"
     "  -P number  pause transfer after `number` response bytes\n"
     "  -r <host>:<port>:<addr>  resolve information\n"
+    "  -S         share connections between easy handles\n"
     "  -T number  max concurrent connections total\n"
     "  -V http_version (http/1.1, h2, h3) http version to use\n"
     "  -6 use IPv6 for resolving the FIRST URL\n"
@@ -314,13 +315,14 @@ static CURLcode test_cli_hx_download(const char *URL)
   size_t max_host_conns = 0;
   size_t max_total_conns = 0;
   int fresh_connect = 0;
+  int share_connect = 0;
   char *cafile = NULL;
   bool first_ipv6 = FALSE;
   CURLcode result = CURLE_OK;
 
   (void)URL;
 
-  while((ch = cgetopt(test_argc, test_argv, "aefhm:n:xA:C:F:M:P:r:T:V:6"))
+  while((ch = cgetopt(test_argc, test_argv, "aefhm:n:xA:C:F:M:P:r:ST:V:6"))
         != -1) {
     const char *opt = coptarg;
     curl_off_t num;
@@ -373,6 +375,9 @@ static CURLcode test_cli_hx_download(const char *URL)
       curlx_free(resolve);
       resolve = curlx_strdup(coptarg);
       break;
+    case 'S':
+      share_connect = 1;
+      break;
     case 'T':
       if(!curlx_str_number(&opt, &num, LONG_MAX))
         max_total_conns = (size_t)num;
@@ -430,9 +435,8 @@ static CURLcode test_cli_hx_download(const char *URL)
   curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_COOKIE);
   curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS);
   curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_SSL_SESSION);
-#if 0
-  curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT);
-#endif
+  if(share_connect)
+    curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT);
   curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_PSL);
   curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_HSTS);