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 8387c60654cb0a1b1582efc571e7e459adae5218..63cdd11af2a1b183223a8ba2bc4403997a75b3a1 100644 (file)
--- 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 122f8f6a7cb8122d54e5b8adf4a3bac0e704e478..9126801511d7cc85b613ec6fcfa70d8798df49fe 100644 (file)
--- 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 ce16503fd11c9e20b5547d87070831bc765badcc..aa3f44fc2f0720c7aff54842523fd6c3f9df6450 100644 (file)
--- 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