cf-ip-happy: limit concurrent attempts

Introduce a limit on the concurrent connect attempts of 6:

- document this in CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS
- close the oldest attempt before opening a new one that would
  exceed the limit
- closing failed attempts early to avoid sockets use beyong their
  usefulness
- add tests for limits in unit2600

These changes are externally visible as file descriptors will be
reassigned where we previously kept the old one around and started a
new socket, allocating always a new descriptor.

Closes #21252

diff --git a/docs/libcurl/opts/CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.md b/docs/libcurl/opts/CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.md
index e38a7c3be578077178d31d5c8821be0cd2e179a5..c4b29d876f4f769e5764d962d88a885dcc4750f8 100644 (file)
--- a/docs/libcurl/opts/CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.md
+++ b/docs/libcurl/opts/CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.md
@@ -65,6 +65,12 @@ anything back). That took 3 times the happy eyeballs timeout, so 600ms
 in the default setting. When any of those four report a success, that
 socket is used for the transfer and the other three are closed.
 
+There is a limit on the number of sockets opened for connect attempts. When
+that limit is reached and more addresses are available, the oldest
+attempt is discarded. This limit is currently 6. With the default
+happy eyeballs timeout of 200ms, this closes attempts after 1.2 seconds
+*as long as there are more addresses to try*.
+
 There are situations where connect attempts fail, but the failure is
 considered being inconclusive. The QUIC protocol may encounter this.
 When a QUIC server restarts, it may send replies indicating that it

diff --git a/lib/cf-ip-happy.c b/lib/cf-ip-happy.c
index 0eb35d467aa6ee83134f2c12b3548311fb388880..0659f56aededc12452ffd843c3cd3856e7f19f30 100644 (file)
--- a/lib/cf-ip-happy.c
+++ b/lib/cf-ip-happy.c
@@ -229,8 +229,12 @@ static CURLcode cf_ip_attempt_connect(struct cf_ip_attempt *a,
         a->connected = TRUE;
       }
     }
-    else if(a->result == CURLE_WEIRD_SERVER_REPLY)
-      a->inconclusive = TRUE;
+    else {
+      if(a->result == CURLE_WEIRD_SERVER_REPLY)
+        a->inconclusive = TRUE;
+      if(a->cf)
+        Curl_conn_cf_discard_chain(&a->cf, data);
+    }
   }
   return a->result;
 }
@@ -247,6 +251,7 @@ struct cf_ip_ballers {
   struct curltime last_attempt_started;
   timediff_t attempt_delay_ms;
   int last_attempt_ai_family;
+  uint32_t max_concurrent;
   uint8_t transport;
 };
 
@@ -254,13 +259,12 @@ static CURLcode cf_ip_attempt_restart(struct cf_ip_attempt *a,
                                       struct Curl_cfilter *cf,
                                       struct Curl_easy *data)
 {
-  struct Curl_cfilter *cf_prev = a->cf;
   struct Curl_cfilter *wcf;
   CURLcode result;
 
-  /* When restarting, we tear down and existing filter *after* we
-   * started up the new one. This gives us a new socket number and
-   * probably a new local port. Which may prevent confusion. */
+  if(a->cf)
+    Curl_conn_cf_discard_chain(&a->cf, data);
+
   a->result = CURLE_OK;
   a->connected = FALSE;
   a->inconclusive = FALSE;
@@ -276,8 +280,6 @@ static CURLcode cf_ip_attempt_restart(struct cf_ip_attempt *a,
     }
     a->result = cf_ip_attempt_connect(a, data, &dummy);
   }
-  if(cf_prev)
-    Curl_conn_cf_discard_chain(&cf_prev, data);
   return result;
 }
 
@@ -299,12 +301,14 @@ static CURLcode cf_ip_ballers_init(struct cf_ip_ballers *bs, int ip_version,
                                    struct Curl_cfilter *cf,
                                    cf_ip_connect_create *cf_create,
                                    uint8_t transport,
-                                   timediff_t attempt_delay_ms)
+                                   timediff_t attempt_delay_ms,
+                                   uint32_t max_concurrent)
 {
   memset(bs, 0, sizeof(*bs));
   bs->cf_create = cf_create;
   bs->transport = transport;
   bs->attempt_delay_ms = attempt_delay_ms;
+  bs->max_concurrent = max_concurrent;
   bs->last_attempt_ai_family = AF_INET; /* so AF_INET6 is next */
 
   if(transport == TRNSPRT_UNIX) {
@@ -333,6 +337,35 @@ static CURLcode cf_ip_ballers_init(struct cf_ip_ballers *bs, int ip_version,
   return CURLE_OK;
 }
 
+static void cf_ip_ballers_prune(struct cf_ip_ballers *bs,
+                                struct Curl_cfilter *cf,
+                                struct Curl_easy *data,
+                                uint32_t max_concurrent)
+{
+  struct cf_ip_attempt *a = NULL, **panchor;
+  uint32_t ongoing = 0;
+
+  for(a = bs->running; a; a = a->next) {
+    if(!a->result && !a->connected)
+      ++ongoing;
+  }
+
+  panchor = &bs->running;
+  while(*panchor && (ongoing > max_concurrent)) {
+    a = *panchor;
+    if(!a->result && !a->connected) {
+      *panchor = a->next;
+      a->next = NULL;
+      cf_ip_attempt_free(a, data);
+      --ongoing;
+      CURL_TRC_CF(data, cf, "discarding oldest attempt to keep limit");
+    }
+    else {
+      panchor = &a->next;
+    }
+  }
+}
+
 static CURLcode cf_ip_ballers_run(struct cf_ip_ballers *bs,
                                   struct Curl_cfilter *cf,
                                   struct Curl_easy *data,
@@ -343,7 +376,7 @@ static CURLcode cf_ip_ballers_run(struct cf_ip_ballers *bs,
   struct cf_ip_attempt *a = NULL, **panchor;
   bool do_more;
   timediff_t next_expire_ms;
-  int inconclusive, ongoing;
+  uint32_t inconclusive, ongoing;
   VERBOSE(int i);
 
   if(bs->winner)
@@ -431,6 +464,11 @@ evaluate:
 
     if(ai) {  /* try another address */
       struct Curl_sockaddr_ex addr;
+
+      /* Discard oldest to make room for new attempt */
+      if(bs->max_concurrent)
+        cf_ip_ballers_prune(bs, cf, data, bs->max_concurrent - 1);
+
       result = Curl_socket_addr_from_ai(&addr, ai, bs->transport);
       if(result)
         goto out;
@@ -545,7 +583,7 @@ static CURLcode cf_ip_ballers_shutdown(struct cf_ip_ballers *bs,
   *done = TRUE;
   for(a = bs->running; a; a = a->next) {
     bool bdone = FALSE;
-    if(a->shutdown)
+    if(a->shutdown || !a->cf)
       continue;
     a->result = a->cf->cft->do_shutdown(a->cf, data, &bdone);
     if(a->result || bdone)
@@ -578,7 +616,7 @@ static bool cf_ip_ballers_pending(struct cf_ip_ballers *bs,
   for(a = bs->running; a; a = a->next) {
     if(a->result)
       continue;
-    if(a->cf->cft->has_data_pending(a->cf, data))
+    if(a->cf && a->cf->cft->has_data_pending(a->cf, data))
       return TRUE;
   }
   return FALSE;
@@ -594,7 +632,7 @@ static struct curltime cf_ip_ballers_max_time(struct cf_ip_ballers *bs,
   memset(&tmax, 0, sizeof(tmax));
   for(a = bs->running; a; a = a->next) {
     memset(&t, 0, sizeof(t));
-    if(!a->cf->cft->query(a->cf, data, query, NULL, &t)) {
+    if(a->cf && !a->cf->cft->query(a->cf, data, query, NULL, &t)) {
       if((t.tv_sec || t.tv_usec) && curlx_ptimediff_us(&t, &tmax) > 0)
         tmax = t;
     }
@@ -609,8 +647,8 @@ static int cf_ip_ballers_min_reply_ms(struct cf_ip_ballers *bs,
   struct cf_ip_attempt *a;
 
   for(a = bs->running; a; a = a->next) {
-    if(!a->cf->cft->query(a->cf, data, CF_QUERY_CONNECT_REPLY_MS,
-                          &breply_ms, NULL)) {
+    if(a->cf && !a->cf->cft->query(a->cf, data, CF_QUERY_CONNECT_REPLY_MS,
+                                   &breply_ms, NULL)) {
       if(breply_ms >= 0 && (reply_ms < 0 || breply_ms < reply_ms))
         reply_ms = breply_ms;
     }
@@ -695,6 +733,8 @@ static CURLcode is_connected(struct Curl_cfilter *cf,
   return result;
 }
 
+#define IP_HE_MAX_CONCURRENT_ATTEMPTS     6
+
 static CURLcode cf_ip_happy_init(struct Curl_cfilter *cf,
                                  struct Curl_easy *data)
 {
@@ -710,7 +750,8 @@ static CURLcode cf_ip_happy_init(struct Curl_cfilter *cf,
   ctx->started = *Curl_pgrs_now(data);
   return cf_ip_ballers_init(&ctx->ballers, cf->conn->ip_version, cf,
                             ctx->cf_create, ctx->transport,
-                            data->set.happy_eyeballs_timeout);
+                            data->set.happy_eyeballs_timeout,
+                            IP_HE_MAX_CONCURRENT_ATTEMPTS);
 }
 
 static void cf_ip_happy_ctx_clear(struct Curl_cfilter *cf,

diff --git a/tests/unit/unit2600.c b/tests/unit/unit2600.c
index b3d616003944fa4c528af5be0be52692e1145d78..e0da5da756a823d4f29c00f0f38d46c557979525 100644 (file)
--- a/tests/unit/unit2600.c
+++ b/tests/unit/unit2600.c
@@ -86,6 +86,7 @@ struct test_case {
   timediff_t max_duration_ms;
   CURLcode result_exp;
   const char *pref_family;
+  uint32_t max_concurrent;
 };
 
 struct ai_family_stats {
@@ -101,6 +102,8 @@ struct test_result {
   struct curltime ended;
   struct ai_family_stats cf4;
   struct ai_family_stats cf6;
+  uint32_t max_concurrent;
+  uint32_t ongoing;
 };
 
 static const struct test_case *current_tc;
@@ -120,8 +123,10 @@ struct cf_test_ctx {
 static void cf_test_destroy(struct Curl_cfilter *cf, struct Curl_easy *data)
 {
   struct cf_test_ctx *ctx = cf->ctx;
-  infof(data, "%04dms: cf[%s] destroyed",
-        (int)curlx_timediff_ms(curlx_now(), current_tr->started), ctx->id);
+  current_tr->ongoing--;
+  infof(data, "%04dms: cf[%s] destroyed, now %u ongoing",
+        (int)curlx_timediff_ms(curlx_now(), current_tr->started),
+        ctx->id, current_tr->ongoing);
   curlx_free(ctx);
   cf->ctx = NULL;
 }
@@ -198,6 +203,9 @@ static CURLcode cf_test_create(struct Curl_cfilter **pcf,
   ctx->ai_family = addr->family;
   ctx->transport = transport;
   ctx->started = curlx_now();
+  current_tr->ongoing++;
+  if(current_tr->ongoing > current_tr->max_concurrent)
+    current_tr->max_concurrent = current_tr->ongoing;
 #ifdef USE_IPV6
   if(ctx->ai_family == AF_INET6) {
     ctx->stats = &current_tr->cf6;
@@ -218,7 +226,8 @@ static CURLcode cf_test_create(struct Curl_cfilter **pcf,
   if(ctx->stats->creations == 1)
     ctx->stats->first_created = created_at;
   ctx->stats->last_created = created_at;
-  infof(data, "%04dms: cf[%s] created", (int)created_at, ctx->id);
+  infof(data, "%04dms: cf[%s] created, now %u ongoing",
+        (int)created_at, ctx->id, current_tr->ongoing);
 
   result = Curl_cf_create(&cf, &cft_test, ctx);
   if(result)
@@ -294,6 +303,11 @@ static void check_result(const struct test_case *tc, struct test_result *tr)
       fail(msg);
     }
   }
+  if(tr->max_concurrent != tc->max_concurrent) {
+    curl_msprintf(msg, "%d: expected max %u ongoing, but reported %u",
+                  tc->id, tc->max_concurrent, tr->max_concurrent);
+    fail(msg);
+  }
 }
 
 static void test_connect(CURL *easy, const struct test_case *tc)
@@ -358,37 +372,51 @@ static CURLcode test_unit2600(const char *arg)
 
   static const struct test_case TEST_CASES[] = {
     /* TIMEOUT_MS,    FAIL_MS      CREATED    DURATION     Result, HE_PREF */
-    /* CNCT   HE      v4    v6     v4 v6      MIN   MAX */
+    /* CNCT   HE      v4    v6     v4 v6      MIN   MAX  MAX_CONCURRENT */
     { 1, TURL, "test.com:123:192.0.2.1", CURL_IPRESOLVE_WHATEVER,
-      CNCT_TMOT, 150, 250,  250,    1,  0,      200,  TC_TMOT,  R_FAIL, NULL },
+      CNCT_TMOT, 150, 250,  250,    1,  0,      200,  TC_TMOT,  R_FAIL, NULL,
+      1 },
     /* 1 ipv4, fails after ~200ms, reports COULDNT_CONNECT */
     { 2, TURL, "test.com:123:192.0.2.1,192.0.2.2", CURL_IPRESOLVE_WHATEVER,
-      CNCT_TMOT, 150, 250,  250,    2,  0,      400,  TC_TMOT,  R_FAIL, NULL },
+      CNCT_TMOT, 150, 250,  250,    2,  0,      400,  TC_TMOT,  R_FAIL, NULL,
+      2 },
     /* 2 ipv4, fails after ~400ms, reports COULDNT_CONNECT */
 #ifdef USE_IPV6
     { 3, TURL, "test.com:123:::1", CURL_IPRESOLVE_WHATEVER,
-      CNCT_TMOT, 150, 250,  250,    0,  1,      200,  TC_TMOT,  R_FAIL, NULL },
+      CNCT_TMOT, 150, 250,  250,    0,  1,      200,  TC_TMOT,  R_FAIL, NULL,
+      1 },
     /* 1 ipv6, fails after ~200ms, reports COULDNT_CONNECT */
     { 4, TURL, "test.com:123:::1,::2", CURL_IPRESOLVE_WHATEVER,
-      CNCT_TMOT, 150, 250,  250,    0,  2,      400,  TC_TMOT,  R_FAIL, NULL },
+      CNCT_TMOT, 150, 250,  250,    0,  2,      400,  TC_TMOT,  R_FAIL, NULL,
+      2 },
     /* 2 ipv6, fails after ~400ms, reports COULDNT_CONNECT */
     { 5, TURL, "test.com:123:192.0.2.1,::1", CURL_IPRESOLVE_WHATEVER,
-      CNCT_TMOT, 150, 250, 250,     1,  1,      350,  TC_TMOT,  R_FAIL, "v6" },
+      CNCT_TMOT, 150, 250, 250,     1,  1,      350,  TC_TMOT,  R_FAIL, "v6",
+      2 },
     /* mixed ip4+6, v6 always first, v4 kicks in on HE, fails after ~350ms */
     { 6, TURL, "test.com:123:::1,192.0.2.1", CURL_IPRESOLVE_WHATEVER,
-      CNCT_TMOT, 150, 250, 250,     1,  1,      350,  TC_TMOT,  R_FAIL, "v6" },
+      CNCT_TMOT, 150, 250, 250,     1,  1,      350,  TC_TMOT,  R_FAIL, "v6",
+      2 },
     /* mixed ip6+4, v6 starts, v4 never starts due to high HE, TIMEOUT */
     { 7, TURL, "test.com:123:192.0.2.1,::1", CURL_IPRESOLVE_V4,
-      CNCT_TMOT, 150, 500, 500,     1,  0,      400,  TC_TMOT,  R_FAIL, NULL },
+      CNCT_TMOT, 150, 500, 500,     1,  0,      400,  TC_TMOT,  R_FAIL, NULL,
+      1 },
     /* mixed ip4+6, but only use v4, check it uses full connect timeout,
        although another address of the 'wrong' family is available */
     { 8, TURL, "test.com:123:::1,192.0.2.1", CURL_IPRESOLVE_V6,
-      CNCT_TMOT, 150, 500, 500,     0,  1,      400,  TC_TMOT,  R_FAIL, NULL },
+      CNCT_TMOT, 150, 500, 500,     0,  1,      400,  TC_TMOT,  R_FAIL, NULL,
+      1 },
     /* mixed ip4+6, but only use v6, check it uses full connect timeout,
        although another address of the 'wrong' family is available */
     { 9, TURL, "test.com:123:::1,192.0.2.1,::2,::3", CURL_IPRESOLVE_WHATEVER,
-      CNCT_TMOT, 50,  400,  400,    1,  3,      550,  TC_TMOT,  R_FAIL, NULL },
+      CNCT_TMOT, 50,  400,  400,    1,  3,      550,  TC_TMOT,  R_FAIL, NULL,
+      4 },
     /* 1 v4, 3 v6, fails after (3*HE)+400ms, ~550ms, COULDNT_CONNECT */
+    { 10, TURL, "test.com:123:::1,192.0.2.1,::2,::3,::4,::5,::6,::7,::8",
+      CURL_IPRESOLVE_WHATEVER,
+      CNCT_TMOT, 20,  500,  500,    1,  8,      550,  TC_TMOT,  R_FAIL, NULL,
+      6 },
+    /* 1 v4, 8 v6, MUST meet limit of 6 concurrent attempts */
 
 #endif
   };