Michael Wallner's test program again help me track down a problem. This time

it basically was that we didn't remove the current connection from the pipe
list when following a redirect. Also in this commit: several cases of
additional debug code for debug builds helping to check and track down some
signs of run-time trouble.

2 files changed, 80 insertions, 19 deletions

diff --git a/lib/multi.c b/lib/multi.c
index 2f7f32870..2da17a3f0 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -424,12 +424,12 @@ CURLMcode curl_multi_add_handle(CURLM *multi_handle,
   /* increase the node-counter */
   multi->num_easy++;
 
-  if((multi->num_easy+5) > multi->connc->num) {
-    /* we want the connection cache to have room for all easy transfers, and
-       some more so we have a margin of 5 for now, but we add the new amount
-       plus 10 to not have to do it for every new handle added */
+  if((multi->num_easy * 4) > multi->connc->num) {
+    /* We want the connection cache to have plenty room. Before we supported
+       the shared cache every single easy handle had 5 entries in their cache
+       by default. */
     CURLcode res = Curl_ch_connc(easy_handle, multi->connc,
-                                 multi->num_easy + 10);
+                                 multi->connc->num*4);
     if(res)
       /* TODO: we need to do some cleaning up here! */
       return res;
@@ -1111,13 +1111,10 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* call this even if the readwrite function returned error */
         Curl_posttransfer(easy->easy_handle);
 
-        if (retry) {
-          Curl_removeHandleFromPipeline(easy->easy_handle,
-                                        easy->easy_conn->recv_pipe);
-        }
-
         /* When we follow redirects, must to go back to the CONNECT state */
         if(easy->easy_handle->reqdata.newurl || retry) {
+          Curl_removeHandleFromPipeline(easy->easy_handle,
+                                        easy->easy_conn->recv_pipe);
           if(!retry) {
             /* if the URL is a follow-location and not just a retried request
                then figure out the URL here */
diff --git a/lib/url.c b/lib/url.c
index 3f59299be..71a827e7a 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -216,6 +216,49 @@ CURLcode Curl_close(struct SessionHandle *data)
 {
   struct Curl_multi *m = data->multi;
 
+#ifdef CURLDEBUG
+  /* only for debugging, scan through all connections and see if there's a
+     pipe reference still identifying this handle */
+
+  if(data->state.is_in_pipeline)
+    fprintf(stderr, "CLOSED when in pipeline!\n");
+
+  if(data->state.connc && data->state.connc->type == CONNCACHE_MULTI) {
+    struct conncache *c = data->state.connc;
+    int i;
+    struct curl_llist *pipe;
+    struct curl_llist_element *curr;
+    struct connectdata *connptr;
+
+    for(i=0; i< c->num; i++) {
+      connptr = c->connects[i];
+      if(!connptr)
+        continue;
+
+      pipe = connptr->send_pipe;
+      if(pipe) {
+        for (curr = pipe->head; curr; curr=curr->next) {
+          if(data == (struct SessionHandle *) curr->ptr) {
+            fprintf(stderr,
+                    "MAJOR problem we %p are still in send pipe for %p done %d\n",
+                    data, connptr, connptr->bits.done);
+          }
+        }
+      }
+      pipe = connptr->recv_pipe;
+      if(pipe) {
+        for (curr = pipe->head; curr; curr=curr->next) {
+          if(data == (struct SessionHandle *) curr->ptr) {
+            fprintf(stderr,
+                    "MAJOR problem we %p are still in recv pipe for %p done %d\n",
+                    data, connptr, connptr->bits.done);
+          }
+        }
+      }
+    }
+  }
+#endif
+
   if(m)
     /* This handle is still part of a multi handle, take care of this first
        and detach this handle from there. */
@@ -1707,6 +1750,11 @@ CURLcode Curl_disconnect(struct connectdata *conn)
     return CURLE_OK; /* this is closed and fine already */
   data = conn->data;
 
+  if(!data) {
+    DEBUGF(infof(data, "DISCONNECT without easy handle, ignoring\n"));
+    return CURLE_OK;
+  }
+
 #if defined(CURLDEBUG) && defined(AGGRESIVE_TEST)
   /* scan for DNS cache entries still marked as in use */
   Curl_hash_apply(data->hostcache,
@@ -1805,12 +1853,17 @@ static bool IsPipeliningPossible(struct SessionHandle *handle)
   return FALSE;
 }
 
-void Curl_addHandleToPipeline(struct SessionHandle *handle,
+void Curl_addHandleToPipeline(struct SessionHandle *data,
                               struct curl_llist *pipe)
 {
-  Curl_llist_insert_next(pipe,
-                         pipe->tail,
-                         handle);
+#ifdef CURLDEBUG
+  if(!IsPipeliningPossible(data)) {
+    /* when not pipelined, there MUST be no handle in the list already */
+    if(pipe->head)
+      infof(data, "PIPE when no PIPE supposed!\n");
+  }
+#endif
+  Curl_llist_insert_next(pipe, pipe->tail, data);
 }
 
 
@@ -1864,7 +1917,14 @@ static void signalPipeClose(struct curl_llist *pipe)
     struct curl_llist_element *next = curr->next;
     struct SessionHandle *data = (struct SessionHandle *) curr->ptr;
 
-    data->state.pipe_broke = TRUE;
+#ifdef CURLDEBUG /* debug-only code */
+    if(data->magic != CURLEASY_MAGIC_NUMBER) {
+      /* MAJOR BADNESS */
+      fprintf(stderr, "signalPipeClose() found BAAD easy handle\n");
+    }
+    else
+#endif
+      data->state.pipe_broke = TRUE;
 
     Curl_llist_remove(pipe, curr, NULL);
     curr = next;
@@ -2062,7 +2122,7 @@ ConnectionDone(struct connectdata *conn)
 
   if (conn->send_pipe == 0 &&
       conn->recv_pipe == 0)
-      conn->is_in_pipeline = FALSE;
+    conn->is_in_pipeline = FALSE;
 }
 
 /*
@@ -2086,7 +2146,8 @@ ConnectionStore(struct SessionHandle *data,
     /* there was no room available, kill one */
     i = ConnectionKillOne(data);
     if(-1 != i)
-      infof(data, "Connection (#%d) was killed to make room\n", i);
+      infof(data, "Connection (#%d) was killed to make room (holds %d)\n",
+            i, data->state.connc->num);
     else
       infof(data, "This connection did not fit in the connection cache\n");
   }
@@ -3541,7 +3602,10 @@ static CURLcode CreateConnection(struct SessionHandle *data,
 
     /* Setup a "faked" transfer that'll do nothing */
     if(CURLE_OK == result) {
+      conn->data = data;
       conn->bits.tcpconnect = TRUE; /* we are "connected */
+      ConnectionStore(data, conn);
+
       result = Curl_setup_transfer(conn, -1, -1, FALSE, NULL, /* no download */
                                    -1, NULL); /* no upload */
     }
@@ -4438,6 +4502,8 @@ CURLcode Curl_done(struct connectdata **connp,
      cancelled before we proceed */
   ares_cancel(data->state.areschannel);
 
+  ConnectionDone(conn); /* the connection is no longer in use */
+
   /* if data->set.reuse_forbid is TRUE, it means the libcurl client has
      forced us to close this no matter what we think.
 
@@ -4463,8 +4529,6 @@ CURLcode Curl_done(struct connectdata **connp,
     infof(data, "Connection #%ld to host %s left intact\n",
           conn->connectindex,
           conn->bits.httpproxy?conn->proxy.dispname:conn->host.dispname);
-
-    ConnectionDone(conn); /* the connection is no longer in use */
   }
 
   return result;