From db9b6fa82e9062d5c1e1bfb9e9faf898075fbb99 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Tue, 7 Apr 2026 11:57:37 +0200 Subject: [PATCH] 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 --- .../opts/CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.md | 6 ++ lib/cf-ip-happy.c | 73 +++++++++++++++---- tests/unit/unit2600.c | 54 ++++++++++---- 3 files changed, 104 insertions(+), 29 deletions(-) diff --git a/docs/libcurl/opts/CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.md b/docs/libcurl/opts/CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS.md index e38a7c3be5..c4b29d876f 100644 --- 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 0eb35d467a..0659f56aed 100644 --- 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 b3d6160039..e0da5da756 100644 --- 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 = ¤t_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 };