Ravi Pratap provided fixes for HTTP pipelining

7 files changed, 97 insertions, 53 deletions

diff --git a/lib/ftp.c b/lib/ftp.c
index 4bc91b9f6..4007bb091 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -331,7 +331,7 @@ static CURLcode ftp_readresp(curl_socket_t sockfd,
        * line */
       int i;
 
-      conn->headerbytecount += gotbytes;
+      data->reqdata.keep.headerbytecount += gotbytes;
 
       ftpc->nread_resp += gotbytes;
       for(i = 0; i < gotbytes; ptr++, i++) {
@@ -564,7 +564,7 @@ CURLcode Curl_GetFTPResponse(ssize_t *nreadp, /* return number of bytes read */
          * line */
         int i;
 
-        conn->headerbytecount += gotbytes;
+        data->reqdata.keep.headerbytecount += gotbytes;
 
         *nreadp += gotbytes;
         for(i = 0; i < gotbytes; ptr++, i++) {
diff --git a/lib/http.c b/lib/http.c
index 8e4d322af..c412df5bb 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -1630,8 +1630,8 @@ CURLcode Curl_http_done(struct connectdata *conn,
 
   if(!conn->bits.retry &&
      ((http->readbytecount +
-       conn->headerbytecount -
-       conn->deductheadercount)) <= 0) {
+       data->reqdata.keep.headerbytecount -
+       data->reqdata.keep.deductheadercount)) <= 0) {
     /* If this connection isn't simply closed to be retried, AND nothing was
        read from the HTTP server (that counts), this can't be right so we
        return an error here */
diff --git a/lib/multi.c b/lib/multi.c
index 17003f47b..3245b520d 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -207,7 +207,7 @@ void curl_multi_dump(CURLM *multi_handle);
 static void multistate(struct Curl_one_easy *easy, CURLMstate state)
 {
 #ifdef CURLDEBUG
-  long index = -1;
+  long index = -5000;
 #endif
   CURLMstate oldstate = easy->state;
 
@@ -534,10 +534,12 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
       multi->num_alive--;
 
     if (easy->easy_handle->state.is_in_pipeline &&
-        easy->state > CURLM_STATE_DO) {
+        easy->state > CURLM_STATE_DO &&
+        easy->state < CURLM_STATE_COMPLETED) {
       /* If the handle is in a pipeline and has finished sending off its
-         request, we need to remember the fact that we want to remove this
-         handle but do the actual removal at a later time */
+         request but not received its reponse yet, we need to remember the
+         fact that we want to remove this handle but do the actual removal at
+         a later time */
       easy->easy_handle->state.cancelled = TRUE;
       return CURLM_OK;
     }
@@ -603,7 +605,9 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle,
 
       /* and modify the connectindex since this handle can't point to the
          connection cache anymore */
-      if(easy->easy_conn)
+      if(easy->easy_conn &&
+         (easy->easy_conn->send_pipe->size +
+          easy->easy_conn->recv_pipe->size == 0))
         easy->easy_conn->connectindex = -1;
     }
 
@@ -648,6 +652,14 @@ bool Curl_multi_canPipeline(struct Curl_multi* multi)
   return multi->pipelining_enabled;
 }
 
+void Curl_multi_handlePipeBreak(struct SessionHandle *data)
+{
+  struct Curl_one_easy *one_easy = data->set.one_easy;
+
+  if (one_easy)
+    one_easy->easy_conn = NULL;
+}
+
 static int waitconnect_getsock(struct connectdata *conn,
                                curl_socket_t *sock,
                                int numsocks)
@@ -791,13 +803,17 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
   struct Curl_transfer_keeper *k;
 
   do {
+    bool disconnect_conn = FALSE;
 
     if(!GOOD_EASY_HANDLE(easy->easy_handle))
       return CURLM_BAD_EASY_HANDLE;
 
+    /* Handle the case when the pipe breaks, i.e., the connection
+       we're using gets cleaned up and we're left with nothing. */
     if (easy->easy_handle->state.pipe_broke) {
       infof(easy->easy_handle, "Pipe broke: handle 0x%x, url = %s\n",
             easy, easy->easy_handle->reqdata.path);
+
       if(easy->easy_handle->state.is_in_pipeline) {
         /* Head back to the CONNECT state */
         multistate(easy, CURLM_STATE_CONNECT);
@@ -920,7 +936,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
           /* call again please so that we get the next socket setup */
           result = CURLM_CALL_MULTI_PERFORM;
           if(protocol_connect)
-            multistate(easy, CURLM_STATE_DO);
+            multistate(easy, CURLM_STATE_WAITDO);
           else {
 #ifndef CURL_DISABLE_HTTP
             if (easy->easy_conn->bits.tunnel_connecting)
@@ -934,8 +950,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
       if(CURLE_OK != easy->result) {
         /* failure detected */
-        Curl_disconnect(easy->easy_conn); /* disconnect properly */
-        easy->easy_conn = NULL;           /* no more connection */
+        disconnect_conn = TRUE;
         break;
       }
     }
@@ -964,8 +979,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
       if(CURLE_OK != easy->result) {
         /* failure detected */
-        Curl_disconnect(easy->easy_conn); /* close the connection */
-        easy->easy_conn = NULL;           /* no more connection */
+        /* Just break, the cleaning up is handled all in one place */
+        disconnect_conn = TRUE;
         break;
       }
 
@@ -998,8 +1013,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* failure detected */
         Curl_posttransfer(easy->easy_handle);
         Curl_done(&easy->easy_conn, easy->result, FALSE);
-        Curl_disconnect(easy->easy_conn); /* close the connection */
-        easy->easy_conn = NULL;           /* no more connection */
+        disconnect_conn = TRUE;
       }
       break;
 
@@ -1065,8 +1079,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
           /* failure detected */
           Curl_posttransfer(easy->easy_handle);
           Curl_done(&easy->easy_conn, easy->result, FALSE);
-          Curl_disconnect(easy->easy_conn); /* close the connection */
-          easy->easy_conn = NULL;           /* no more connection */
+          disconnect_conn = TRUE;
         }
       }
       break;
@@ -1098,8 +1111,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* failure detected */
         Curl_posttransfer(easy->easy_handle);
         Curl_done(&easy->easy_conn, easy->result, FALSE);
-        Curl_disconnect(easy->easy_conn); /* close the connection */
-        easy->easy_conn = NULL;           /* no more connection */
+        disconnect_conn = TRUE;
       }
       break;
 
@@ -1208,9 +1220,11 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
       if(easy->result)  {
         /* The transfer phase returned error, we mark the connection to get
-         * closed to prevent being re-used. This is becasue we can't
+         * closed to prevent being re-used. This is because we can't
          * possibly know if the connection is in a good shape or not now. */
         easy->easy_conn->bits.close = TRUE;
+        Curl_removeHandleFromPipeline(easy->easy_handle,
+                                      easy->easy_conn->recv_pipe);
 
         if(CURL_SOCKET_BAD != easy->easy_conn->sock[SECONDARYSOCKET]) {
           /* if we failed anywhere, we must clean up the secondary socket if
@@ -1292,10 +1306,17 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
       /* This node should be delinked from the list now and we should post
          an information message that we are complete. */
+
+      /* Important: reset the conn pointer so that we don't point to memory
+         that could be freed anytime */
+      easy->easy_conn = NULL;
       break;
 
     case CURLM_STATE_CANCELLED:
       /* Cancelled transfer, wait to be cleaned up */
+
+      /* Reset the conn pointer so we don't leave it dangling */
+      easy->easy_conn = NULL;
       break;
 
     default:
@@ -1308,6 +1329,10 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
          * If an error was returned, and we aren't in completed state now,
          * then we go to completed and consider this transfer aborted.
          */
+
+        /* NOTE: no attempt to disconnect connections must be made
+           in the case blocks above - cleanup happens only here */
+
         easy->easy_handle->state.is_in_pipeline = FALSE;
         easy->easy_handle->state.pipe_broke = FALSE;
 
@@ -1315,7 +1340,21 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
           /* if this has a connection, unsubscribe from the pipelines */
           easy->easy_conn->writechannel_inuse = FALSE;
           easy->easy_conn->readchannel_inuse = FALSE;
+          Curl_removeHandleFromPipeline(easy->easy_handle,
+                                        easy->easy_conn->send_pipe);
+          Curl_removeHandleFromPipeline(easy->easy_handle,
+                                        easy->easy_conn->recv_pipe);
         }
+
+        if (disconnect_conn) {
+          Curl_disconnect(easy->easy_conn); /* disconnect properly */
+
+          /* This is where we make sure that the easy_conn pointer is reset.
+             We don't have to do this in every case block above where a
+             failure is detected */
+          easy->easy_conn = NULL;
+        }
+
         multistate(easy, CURLM_STATE_COMPLETED);
       }
     }
diff --git a/lib/multiif.h b/lib/multiif.h
index 800aa0f5d..2a8b06c27 100644
--- a/lib/multiif.h
+++ b/lib/multiif.h
@@ -31,6 +31,7 @@ void Curl_expire(struct SessionHandle *data, long milli);
 void Curl_multi_rmeasy(void *multi, CURL *data);
 
 bool Curl_multi_canPipeline(struct Curl_multi* multi);
+void Curl_multi_handlePipeBreak(struct SessionHandle *data);
 
 /* the write bits start at bit 16 for the *getsock() bitmap */
 #define GETSOCK_WRITEBITSTART 16
diff --git a/lib/transfer.c b/lib/transfer.c
index f4b0380d3..b70f3b509 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -366,6 +366,7 @@ CURLcode Curl_readwrite(struct connectdata *conn,
         /* receive data from the network! */
         readrc = Curl_read(conn, conn->sockfd, k->buf, bytestoread, &nread);
 
+        DEBUGF(infof(data, "Read %ld bytes from stream (readrc = %d)\n", nread, readrc));
         /* subzero, this would've blocked */
         if(0 > readrc)
           break; /* get out of loop */
@@ -394,7 +395,7 @@ CURLcode Curl_readwrite(struct connectdata *conn,
         else if (0 >= nread) {
           /* if we receive 0 or less here, the server closed the connection
              and we bail out from this! */
-
+          DEBUGF(infof(data, "nread <= 0, server closed connection, bailing\n"));
           k->keepon &= ~KEEP_READ;
           break;
         }
@@ -608,10 +609,10 @@ CURLcode Curl_readwrite(struct connectdata *conn,
                 return result;
 
               data->info.header_size += (long)headerlen;
-              conn->headerbytecount += (long)headerlen;
+              data->reqdata.keep.headerbytecount += (long)headerlen;
 
-              conn->deductheadercount =
-                (100 == k->httpcode)?conn->headerbytecount:0;
+              data->reqdata.keep.deductheadercount =
+                (100 == k->httpcode)?data->reqdata.keep.headerbytecount:0;
 
               if (data->reqdata.resume_from &&
                   (data->set.httpreq==HTTPREQ_GET) &&
@@ -1106,7 +1107,7 @@ CURLcode Curl_readwrite(struct connectdata *conn,
               return result;
 
             data->info.header_size += (long)k->hbuflen;
-            conn->headerbytecount += (long)k->hbuflen;
+            data->reqdata.keep.headerbytecount += (long)k->hbuflen;
 
             /* reset hbufp pointer && hbuflen */
             k->hbufp = data->state.headerbuff;
@@ -2341,7 +2342,8 @@ bool Curl_retry_request(struct connectdata *conn,
   bool retry = FALSE;
   struct SessionHandle *data = conn->data;
 
-  if((data->reqdata.keep.bytecount+conn->headerbytecount == 0) &&
+  if((data->reqdata.keep.bytecount +
+      data->reqdata.keep.headerbytecount == 0) &&
      conn->bits.reuse &&
      !conn->bits.no_body) {
     /* We got no data, we attempted to re-use a connection and yet we want a
diff --git a/lib/url.c b/lib/url.c
index 3b6e56642..ddb214d20 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1970,8 +1970,7 @@ static void Curl_printPipeline(struct curl_llist *pipe)
   curr = pipe->head;
   while (curr) {
     struct SessionHandle *data = (struct SessionHandle *) curr->ptr;
-    infof(data, "Handle in pipeline: %s\n",
-          data->reqdata.path);
+    infof(data, "Handle in pipeline: %s\n", data->reqdata.path);
     curr = curr->next;
   }
 }
@@ -2010,12 +2009,12 @@ static void signalPipeClose(struct curl_llist *pipe)
 #ifdef CURLDEBUG /* debug-only code */
     if(data->magic != CURLEASY_MAGIC_NUMBER) {
       /* MAJOR BADNESS */
-      fprintf(stderr, "signalPipeClose() found BAAD easy handle\n");
+      infof(data, "signalPipeClose() found BAAD easy handle\n");
     }
-    else
 #endif
 
     data->state.pipe_broke = TRUE;
+    Curl_multi_handlePipeBreak(data);
     Curl_llist_remove(pipe, curr, NULL);
     curr = next;
   }
@@ -2060,8 +2059,8 @@ ConnectionExists(struct SessionHandle *data,
                                   from the multi */
     }
 
-    DEBUGF(infof(data, "Examining connection #%ld for reuse, "
-                 "(pipeLen = %ld)\n", check->connectindex, pipeLen));
+    DEBUGF(infof(data, "Examining connection #%ld for reuse"
+                 " (pipeLen = %ld)\n", check->connectindex, pipeLen));
 
     if(pipeLen > 0 && !canPipeline) {
       /* can only happen within multi handles, and means that another easy
@@ -2074,12 +2073,26 @@ ConnectionExists(struct SessionHandle *data,
        yet and until then we don't re-use this connection */
     if (!check->ip_addr_str) {
       infof(data,
-            "Connection #%ld has not finished name resolve, can't reuse\n",
+            "Connection #%ld hasn't finished name resolve, can't reuse\n",
             check->connectindex);
       continue;
     }
 #endif
 
+    if ((check->sock[FIRSTSOCKET] == CURL_SOCKET_BAD) || check->bits.close) {
+      /* Don't pick a connection that hasn't connected yet or that is going to
+         get closed. */
+      infof(data, "Connection #%ld isn't open enough, can't reuse\n",
+            check->connectindex);
+#ifdef CURLDEBUG
+      if (check->recv_pipe->size > 0) {
+        infof(data, "BAD! Unconnected #%ld has a non-empty recv pipeline!\n",
+              check->connectindex);
+      }
+#endif
+      continue;
+    }
+
     if (pipeLen >= MAX_PIPELINE_LENGTH) {
       infof(data, "Connection #%ld has its pipeline full, can't reuse\n",
             check->connectindex);
@@ -2100,13 +2113,6 @@ ConnectionExists(struct SessionHandle *data,
       }
     }
 
-    if (check->bits.close) {
-      /* Don't pick a connection that is going to be closed. */
-      infof(data, "Connection #%ld has been marked for close, can't reuse\n",
-            check->connectindex);
-      continue;
-    }
-
     if((needle->protocol&PROT_SSL) != (check->protocol&PROT_SSL))
       /* don't do mixed SSL and non-SSL connections */
       continue;
@@ -2178,15 +2184,11 @@ ConnectionExists(struct SessionHandle *data,
 
       check->inuse = TRUE; /* mark this as being in use so that no other
                               handle in a multi stack may nick it */
-
       if (canPipeline) {
         /* Mark the connection as being in a pipeline */
         check->is_in_pipeline = TRUE;
       }
 
-      check->connectindex = i; /* Set this appropriately since it might have
-                                  been set to -1 when the easy was removed
-                                  from the multi */
       *usethis = check;
       return TRUE; /* yes, we found one to use! */
     }
@@ -4069,7 +4071,7 @@ static CURLcode SetupConnection(struct connectdata *conn,
       return CURLE_OUT_OF_MEMORY;
   }
 
-  conn->headerbytecount = 0;
+  data->reqdata.keep.headerbytecount = 0;
 
 #ifdef CURL_DO_LINEEND_CONV
   data->state.crlf_conversions = 0; /* reset CRLF conversion counter */
diff --git a/lib/urldata.h b/lib/urldata.h
index 879f36902..419915569 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -555,6 +555,13 @@ struct Curl_transfer_keeper {
   curl_off_t bytecount;         /* total number of bytes read */
   curl_off_t writebytecount;    /* number of bytes written */
 
+  long headerbytecount;  /* only count received headers */
+  long deductheadercount; /* this amount of bytes doesn't count when we check
+                          if anything has been transfered at the end of
+                          a connection. We use this counter to make only
+                          a 100 reply (without a following second response
+                          code) result in a CURLE_GOT_NOTHING error code */
+
   struct timeval start;         /* transfer started at this time */
   struct timeval now;           /* current time */
   bool header;                  /* incoming data has HTTP header */
@@ -756,13 +763,6 @@ struct connectdata {
   unsigned short remote_port; /* what remote port to connect to,
                                  not the proxy port! */
 
-  long headerbytecount;  /* only count received headers */
-  long deductheadercount; /* this amount of bytes doesn't count when we check
-                             if anything has been transfered at the end of
-                             a connection. We use this counter to make only
-                             a 100 reply (without a following second response
-                             code) result in a CURLE_GOT_NOTHING error code */
-
   char *user;    /* user name string, allocated */
   char *passwd;  /* password string, allocated */