conncache: avoid double-locking

... by not calling Curl_disconnect directly from within the foreach
callback, but instead have that return an extracted "dead" connection
that the caller can then disconnect.

3 files changed, 60 insertions, 33 deletions

diff --git a/lib/conncache.c b/lib/conncache.c
index a5cd10cad..cacfa9abc 100644
--- a/lib/conncache.c
+++ b/lib/conncache.c
@@ -300,15 +300,18 @@ CURLcode Curl_conncache_add_conn(struct conncache *connc,
   return CURLE_OK;
 }
 
-void Curl_conncache_remove_conn(struct conncache *connc,
-                                struct connectdata *conn)
+void Curl_conncache_remove_conn(struct connectdata *conn, bool lock)
 {
+  struct Curl_easy *data = conn->data;
   struct connectbundle *bundle = conn->bundle;
+  struct conncache *connc = data->state.conn_cache;
 
   /* The bundle pointer can be NULL, since this function can be called
      due to a failed connection attempt, before being added to a bundle */
   if(bundle) {
-    CONN_LOCK(conn->data);
+    if(lock) {
+      CONN_LOCK(conn->data);
+    }
     bundle_remove_conn(bundle, conn);
     if(bundle->num_connections == 0)
       conncache_remove_bundle(connc, bundle);
@@ -318,17 +321,25 @@ void Curl_conncache_remove_conn(struct conncache *connc,
                    CURL_FORMAT_CURL_OFF_TU " members\n",
                    (curl_off_t) connc->num_conn));
     }
-    CONN_UNLOCK(conn->data);
+    if(lock) {
+      CONN_UNLOCK(conn->data);
+    }
   }
 }
 
-/* This function iterates the entire connection cache and calls the
-   function func() with the connection pointer as the first argument
-   and the supplied 'param' argument as the other,
+/* This function iterates the entire connection cache and calls the function
+   func() with the connection pointer as the first argument and the supplied
+   'param' argument as the other.
+
+   The conncache lock is still held when the callback is called. It needs it,
+   so that it can safely continue traversing the lists once the callback
+   returns.
+
+   Returns 1 if the loop was aborted due to the callback's return code.
 
    Return 0 from func() to continue the loop, return 1 to abort it.
  */
-void Curl_conncache_foreach(struct Curl_easy *data,
+bool Curl_conncache_foreach(struct Curl_easy *data,
                             struct conncache *connc,
                             void *param,
                             int (*func)(struct connectdata *conn, void *param))
@@ -338,7 +349,7 @@ void Curl_conncache_foreach(struct Curl_easy *data,
   struct curl_hash_element *he;
 
   if(!connc)
-    return;
+    return FALSE;
 
   CONN_LOCK(data);
   Curl_hash_start_iterate(&connc->hash, &iter);
@@ -359,11 +370,12 @@ void Curl_conncache_foreach(struct Curl_easy *data,
 
       if(1 == func(conn, param)) {
         CONN_UNLOCK(data);
-        return;
+        return TRUE;
       }
     }
   }
   CONN_UNLOCK(data);
+  return FALSE;
 }
 
 /* Return the first connection found in the cache. Used when closing all
diff --git a/lib/conncache.h b/lib/conncache.h
index 36c18ecbf..7c046a016 100644
--- a/lib/conncache.h
+++ b/lib/conncache.h
@@ -62,11 +62,9 @@ size_t Curl_conncache_size(struct Curl_easy *data);
 
 CURLcode Curl_conncache_add_conn(struct conncache *connc,
                                  struct connectdata *conn);
-
-void Curl_conncache_remove_conn(struct conncache *connc,
-                                struct connectdata *conn);
-
-void Curl_conncache_foreach(struct Curl_easy *data,
+void Curl_conncache_remove_conn(struct connectdata *conn,
+                                bool lock);
+bool Curl_conncache_foreach(struct Curl_easy *data,
                             struct conncache *connc,
                             void *param,
                             int (*func)(struct connectdata *conn,
diff --git a/lib/url.c b/lib/url.c
index e66464c8a..9c613f236 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -777,7 +777,7 @@ CURLcode Curl_disconnect(struct connectdata *conn, bool dead_connection)
 
     /* unlink ourselves! */
   infof(data, "Closing connection %ld\n", conn->connection_id);
-  Curl_conncache_remove_conn(data->state.conn_cache, conn);
+  Curl_conncache_remove_conn(conn, TRUE);
 
   free_fixed_hostname(&conn->host);
   free_fixed_hostname(&conn->conn_to_host);
@@ -986,13 +986,16 @@ find_oldest_idle_connection_in_bundle(struct Curl_easy *data,
 }
 
 /*
- * This function checks if given connection is dead and disconnects if so.
- * (That also removes it from the connection cache.)
+ * This function checks if the given connection is dead and extracts it from
+ * the connection cache if so.
  *
- * Returns TRUE if the connection actually was dead and disconnected.
+ * When this is called as a Curl_conncache_foreach() callback, the connection
+ * cache lock is held!
+ *
+ * Returns TRUE if the connection was dead and extracted.
  */
-static bool disconnect_if_dead(struct connectdata *conn,
-                               struct Curl_easy *data)
+static bool extract_if_dead(struct connectdata *conn,
+                            struct Curl_easy *data)
 {
   size_t pipeLen = conn->send_pipe.size + conn->recv_pipe.size;
   if(!pipeLen && !conn->inuse) {
@@ -1017,25 +1020,30 @@ static bool disconnect_if_dead(struct connectdata *conn,
     if(dead) {
       conn->data = data;
       infof(data, "Connection %ld seems to be dead!\n", conn->connection_id);
-
-      /* disconnect resources */
-      Curl_disconnect(conn, /* dead_connection */TRUE);
+      Curl_conncache_remove_conn(conn, FALSE);
       return TRUE;
     }
   }
   return FALSE;
 }
 
+struct prunedead {
+  struct Curl_easy *data;
+  struct connectdata *extracted;
+};
+
 /*
- * Wrapper to use disconnect_if_dead() function in Curl_conncache_foreach()
+ * Wrapper to use extract_if_dead() function in Curl_conncache_foreach()
  *
- * Returns always 0.
  */
-static int call_disconnect_if_dead(struct connectdata *conn,
-                                      void *param)
+static int call_extract_if_dead(struct connectdata *conn, void *param)
 {
-  struct Curl_easy* data = (struct Curl_easy*)param;
-  disconnect_if_dead(conn, data);
+  struct prunedead *p = (struct prunedead *)param;
+  if(extract_if_dead(conn, p->data)) {
+    /* stop the iteration here, pass back the connection that was extracted */
+    p->extracted = conn;
+    return 1;
+  }
   return 0; /* continue iteration */
 }
 
@@ -1050,8 +1058,14 @@ static void prune_dead_connections(struct Curl_easy *data)
   time_t elapsed = Curl_timediff(now, data->state.conn_cache->last_cleanup);
 
   if(elapsed >= 1000L) {
-    Curl_conncache_foreach(data, data->state.conn_cache, data,
-                           call_disconnect_if_dead);
+    struct prunedead prune;
+    prune.data = data;
+    prune.extracted = NULL;
+    while(Curl_conncache_foreach(data, data->state.conn_cache, &prune,
+                                 call_extract_if_dead)) {
+      /* disconnect it */
+      (void)Curl_disconnect(prune.extracted, /* dead_connection */TRUE);
+    }
     data->state.conn_cache->last_cleanup = now;
   }
 }
@@ -1162,8 +1176,11 @@ ConnectionExists(struct Curl_easy *data,
       check = curr->ptr;
       curr = curr->next;
 
-      if(disconnect_if_dead(check, data))
+      if(extract_if_dead(check, data)) {
+        /* disconnect it */
+        (void)Curl_disconnect(check, /* dead_connection */TRUE);
         continue;
+      }
 
       pipeLen = check->send_pipe.size + check->recv_pipe.size;