From: Daniel Stenberg Date: Fri, 5 Apr 2024 11:07:16 +0000 (+0200) Subject: multi: introduce SETUP state for better timeouts X-Git-Tag: curl-8_8_0~227 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=be659030ba078d6aa0b74a806da77dfb123159c6;p=thirdparty%2Fcurl.git multi: introduce SETUP state for better timeouts Since we can go to the CONNECT state from PENDING, potentially multiple times for a single transfer, this change introdues a SETUP state that happens before CONNECT when doing a new transfer. Now, doing a redirect on a handle goes back to SETUP (not CONNECT like before) and we initilize the connect timeout etc in SETUP. Previously, we would do it in CONNECT but that would make it unreliable in cases where a transfer goes in and out between CONNECT and PENDING multiple times. SETUP is transient, so the handle never actually stays in that state. Additionally: take care of timeouts of PENDING transfers in curl_multi_perform() Ref: #13227 Closes #13371 --- diff --git a/lib/multi.c b/lib/multi.c index 8387c60654..63cdd11af2 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -86,6 +86,8 @@ ((x) && (x)->magic == CURL_MULTI_HANDLE) #endif +static void move_pending_to_connect(struct Curl_multi *multi, + struct Curl_easy *data); static CURLMcode singlesocket(struct Curl_multi *multi, struct Curl_easy *data); static CURLMcode add_next_timeout(struct curltime now, @@ -100,6 +102,7 @@ static void multi_xfer_bufs_free(struct Curl_multi *multi); static const char * const multi_statename[]={ "INIT", "PENDING", + "SETUP", "CONNECT", "RESOLVING", "CONNECTING", @@ -149,6 +152,7 @@ static void mstate(struct Curl_easy *data, CURLMstate state static const init_multistate_func finit[MSTATE_LAST] = { NULL, /* INIT */ NULL, /* PENDING */ + NULL, /* SETUP */ Curl_init_CONNECT, /* CONNECT */ NULL, /* RESOLVING */ NULL, /* CONNECTING */ @@ -1110,6 +1114,7 @@ static void multi_getsock(struct Curl_easy *data, switch(data->mstate) { case MSTATE_INIT: case MSTATE_PENDING: + case MSTATE_SETUP: case MSTATE_CONNECT: /* nothing to poll for yet */ break; @@ -1745,7 +1750,6 @@ static bool multi_handle_timeout(struct Curl_easy *data, bool connect_timeout) { timediff_t timeout_ms = Curl_timeleft(data, now, connect_timeout); - if(timeout_ms < 0) { /* Handle timed out */ struct curltime since; @@ -1952,31 +1956,39 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, switch(data->mstate) { case MSTATE_INIT: - /* init this transfer. */ + /* Transitional state. init this transfer. A handle never comes + back to this state. */ result = Curl_pretransfer(data); - - if(!result) { - /* after init, go CONNECT */ - multistate(data, MSTATE_CONNECT); - *nowp = Curl_pgrsTime(data, TIMER_STARTOP); - rc = CURLM_CALL_MULTI_PERFORM; - } - break; - - case MSTATE_CONNECT: - /* Connect. We want to get a connection identifier filled in. */ - /* init this transfer. */ - result = Curl_preconnect(data); if(result) break; + /* after init, go SETUP */ + multistate(data, MSTATE_SETUP); + (void)Curl_pgrsTime(data, TIMER_STARTOP); + FALLTHROUGH(); + + case MSTATE_SETUP: + /* Transitional state. Setup things for a new transfer. The handle + can come back to this state on a redirect. */ *nowp = Curl_pgrsTime(data, TIMER_STARTSINGLE); if(data->set.timeout) Curl_expire(data, data->set.timeout, EXPIRE_TIMEOUT); - if(data->set.connecttimeout) + /* Since a connection might go to pending and back to CONNECT several + times before it actually takes off, we need to set the timeout once + in SETUP before we enter CONNECT the first time. */ Curl_expire(data, data->set.connecttimeout, EXPIRE_CONNECTTIMEOUT); + multistate(data, MSTATE_CONNECT); + FALLTHROUGH(); + + case MSTATE_CONNECT: + /* Connect. We want to get a connection identifier filled in. This state + can be entered from SETUP and from PENDING. */ + result = Curl_preconnect(data); + if(result) + break; + result = Curl_connect(data, &async, &connected); if(CURLE_NO_CONNECTION_AVAILABLE == result) { /* There was no connection available. We will go to the pending @@ -1990,11 +2002,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, result = CURLE_OK; break; } - else if(data->state.previouslypending) { - /* this transfer comes from the pending queue so try move another */ - infof(data, "Transfer was pending, now try another"); + else process_pending_handles(data->multi); - } if(!result) { *nowp = Curl_pgrsTime(data, TIMER_POSTQUEUE); @@ -2275,7 +2284,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, follow = FOLLOW_RETRY; drc = Curl_follow(data, newurl, follow); if(!drc) { - multistate(data, MSTATE_CONNECT); + multistate(data, MSTATE_SETUP); rc = CURLM_CALL_MULTI_PERFORM; result = CURLE_OK; } @@ -2535,7 +2544,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* multi_done() might return CURLE_GOT_NOTHING */ result = Curl_follow(data, newurl, follow); if(!result) { - multistate(data, MSTATE_CONNECT); + multistate(data, MSTATE_SETUP); rc = CURLM_CALL_MULTI_PERFORM; } free(newurl); @@ -2629,7 +2638,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, * (i.e. CURLM_CALL_MULTI_PERFORM == TRUE) then we should do that before * declaring the connection timed out as we may almost have a completed * connection. */ - multi_handle_timeout(data, nowp, &stream_error, &result, TRUE); + multi_handle_timeout(data, nowp, &stream_error, &result, FALSE); } statemachine_end: @@ -2768,10 +2777,20 @@ CURLMcode curl_multi_perform(struct Curl_multi *multi, int *running_handles) */ do { multi->timetree = Curl_splaygetbest(now, multi->timetree, &t); - if(t) + if(t) { /* the removed may have another timeout in queue */ + data = t->payload; + if(data->mstate == MSTATE_PENDING) { + bool stream_unused; + CURLcode result_unused; + if(multi_handle_timeout(data, &now, &stream_unused, &result_unused, + FALSE)) { + infof(data, "PENDING handle timeout"); + move_pending_to_connect(multi, data); + } + } (void)add_next_timeout(now, multi, t->payload); - + } } while(t); *running_handles = multi->num_alive; @@ -3725,29 +3744,43 @@ void Curl_multiuse_state(struct Curl_easy *data, process_pending_handles(data->multi); } -/* process_pending_handles() moves all handles from PENDING - back into the main list and change state to CONNECT */ -static void process_pending_handles(struct Curl_multi *multi) +static void move_pending_to_connect(struct Curl_multi *multi, + struct Curl_easy *data) { - struct Curl_llist_element *e = multi->pending.head; - if(e) { - struct Curl_easy *data = e->ptr; + DEBUGASSERT(data->mstate == MSTATE_PENDING); + + /* put it back into the main list */ + link_easy(multi, data); - DEBUGASSERT(data->mstate == MSTATE_PENDING); + multistate(data, MSTATE_CONNECT); - /* put it back into the main list */ - link_easy(multi, data); + /* Remove this node from the pending list */ + Curl_llist_remove(&multi->pending, &data->connect_queue, NULL); - multistate(data, MSTATE_CONNECT); + /* Make sure that the handle will be processed soonish. */ + Curl_expire(data, 0, EXPIRE_RUN_NOW); +} - /* Remove this node from the list */ - Curl_llist_remove(&multi->pending, e, NULL); +/* process_pending_handles() moves a handle from PENDING back into the main + list and change state to CONNECT. - /* Make sure that the handle will be processed soonish. */ - Curl_expire(data, 0, EXPIRE_RUN_NOW); + We do not move all transfers because that can be a significant amount. + Since this is tried every now and then doing too many too often becomes a + performance problem. - /* mark this as having been in the pending queue */ - data->state.previouslypending = TRUE; + When there is a change for connection limits like max host connections etc, + this likely only allows one new transfer. When there is a pipewait change, + it can potentially allow hundreds of new transfers. + + We could consider an improvement where we store the queue reason and allow + more pipewait rechecks than others. +*/ +static void process_pending_handles(struct Curl_multi *multi) +{ + struct Curl_llist_element *e = multi->pending.head; + if(e) { + struct Curl_easy *data = e->ptr; + move_pending_to_connect(multi, data); } } diff --git a/lib/multihandle.h b/lib/multihandle.h index 122f8f6a7c..9126801511 100644 --- a/lib/multihandle.h +++ b/lib/multihandle.h @@ -44,24 +44,25 @@ struct Curl_message { typedef enum { MSTATE_INIT, /* 0 - start in this state */ MSTATE_PENDING, /* 1 - no connections, waiting for one */ - MSTATE_CONNECT, /* 2 - resolve/connect has been sent off */ - MSTATE_RESOLVING, /* 3 - awaiting the resolve to finalize */ - MSTATE_CONNECTING, /* 4 - awaiting the TCP connect to finalize */ - MSTATE_TUNNELING, /* 5 - awaiting HTTPS proxy SSL initialization to + MSTATE_SETUP, /* 2 - start a new transfer */ + MSTATE_CONNECT, /* 3 - resolve/connect has been sent off */ + MSTATE_RESOLVING, /* 4 - awaiting the resolve to finalize */ + MSTATE_CONNECTING, /* 5 - awaiting the TCP connect to finalize */ + MSTATE_TUNNELING, /* 6 - awaiting HTTPS proxy SSL initialization to complete and/or proxy CONNECT to finalize */ - MSTATE_PROTOCONNECT, /* 6 - initiate protocol connect procedure */ - MSTATE_PROTOCONNECTING, /* 7 - completing the protocol-specific connect + MSTATE_PROTOCONNECT, /* 7 - initiate protocol connect procedure */ + MSTATE_PROTOCONNECTING, /* 8 - completing the protocol-specific connect phase */ - MSTATE_DO, /* 8 - start send off the request (part 1) */ - MSTATE_DOING, /* 9 - sending off the request (part 1) */ - MSTATE_DOING_MORE, /* 10 - send off the request (part 2) */ - MSTATE_DID, /* 11 - done sending off request */ - MSTATE_PERFORMING, /* 12 - transfer data */ - MSTATE_RATELIMITING, /* 13 - wait because limit-rate exceeded */ - MSTATE_DONE, /* 14 - post data transfer operation */ - MSTATE_COMPLETED, /* 15 - operation complete */ - MSTATE_MSGSENT, /* 16 - the operation complete message is sent */ - MSTATE_LAST /* 17 - not a true state, never use this */ + MSTATE_DO, /* 9 - start send off the request (part 1) */ + MSTATE_DOING, /* 10 - sending off the request (part 1) */ + MSTATE_DOING_MORE, /* 11 - send off the request (part 2) */ + MSTATE_DID, /* 12 - done sending off request */ + MSTATE_PERFORMING, /* 13 - transfer data */ + MSTATE_RATELIMITING, /* 14 - wait because limit-rate exceeded */ + MSTATE_DONE, /* 15 - post data transfer operation */ + MSTATE_COMPLETED, /* 16 - operation complete */ + MSTATE_MSGSENT, /* 17 - the operation complete message is sent */ + MSTATE_LAST /* 18 - not a true state, never use this */ } CURLMstate; /* we support N sockets per easy handle. Set the corresponding bit to what diff --git a/lib/urldata.h b/lib/urldata.h index ce16503fd1..aa3f44fc2f 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -1386,7 +1386,6 @@ struct UrlState { BIT(done); /* set to FALSE when Curl_init_do() is called and set to TRUE when multi_done() is called, to prevent multi_done() to get invoked twice when the multi interface is used. */ - BIT(previouslypending); /* this transfer WAS in the multi->pending queue */ #ifndef CURL_DISABLE_COOKIES BIT(cookie_engine); #endif