conncache: fix several lock issues

If the lock is released before the dealings with the bundle is over, it may have changed by another thread in the mean time. Fixes #2132 Fixes #2151 Closes #2139
2024-12-21 15:48:49 -05:00 · 2017-12-02 14:27:00 +01:00 · 2017-12-02 14:27:00 +01:00 · 07cb27c98e
commit 07cb27c98e
parent 85f0133ea1
6 changed files with 315 additions and 183 deletions
--- a/lib/conncache.c
+++ b/lib/conncache.c
@ -40,11 +40,26 @@
 #include "curl_memory.h"
 #include "memdebug.h"

+#ifdef CURLDEBUG
+/* the debug versions of these macros make extra certain that the lock is
+   never doubly locked or unlocked */
+#define CONN_LOCK(x) if((x)->share) {                                   \
+    Curl_share_lock((x), CURL_LOCK_DATA_CONNECT, CURL_LOCK_ACCESS_SINGLE); \
+    DEBUGASSERT(!(x)->state.conncache_lock);                            \
+    (x)->state.conncache_lock = TRUE;                                   \
+  }
+
+#define CONN_UNLOCK(x) if((x)->share) {                                 \
+    DEBUGASSERT((x)->state.conncache_lock);                             \
+    (x)->state.conncache_lock = FALSE;                                  \
+    Curl_share_unlock((x), CURL_LOCK_DATA_CONNECT);                     \
+  }
+#else
 #define CONN_LOCK(x) if((x)->share)                                     \
    Curl_share_lock((x), CURL_LOCK_DATA_CONNECT, CURL_LOCK_ACCESS_SINGLE)
 #define CONN_UNLOCK(x) if((x)->share)                   \
    Curl_share_unlock((x), CURL_LOCK_DATA_CONNECT)
-
+#endif

 static void conn_llist_dtor(void *user, void *element)
 {
@ -165,18 +180,48 @@ static void hashkey(struct connectdata *conn, char *buf,
  snprintf(buf, len, "%ld%s", conn->port, hostname);
 }

+void Curl_conncache_unlock(struct connectdata *conn)
+{
+  CONN_UNLOCK(conn->data);
+}
+
+/* Returns number of connections currently held in the connection cache.
+   Locks/unlocks the cache itself!
+*/
+size_t Curl_conncache_size(struct Curl_easy *data)
+{
+  size_t num;
+  CONN_LOCK(data);
+  num = data->state.conn_cache->num_conn;
+  CONN_UNLOCK(data);
+  return num;
+}
+
+/* Returns number of connections currently held in the connections's bundle
+   Locks/unlocks the cache itself!
+*/
+size_t Curl_conncache_bundle_size(struct connectdata *conn)
+{
+  size_t num;
+  CONN_LOCK(conn->data);
+  num = conn->bundle->num_connections;
+  CONN_UNLOCK(conn->data);
+  return num;
+}
+
 /* Look up the bundle with all the connections to the same host this
-   connectdata struct is setup to use. */
+   connectdata struct is setup to use.
+
+   **NOTE**: When it returns, it holds the connection cache lock! */
 struct connectbundle *Curl_conncache_find_bundle(struct connectdata *conn,
                                                 struct conncache *connc)
 {
  struct connectbundle *bundle = NULL;
+  CONN_LOCK(conn->data);
  if(connc) {
    char key[128];
    hashkey(conn, key, sizeof(key));
-    CONN_LOCK(conn->data);
    bundle = Curl_hash_pick(&connc->hash, key, strlen(key));
-    CONN_UNLOCK(conn->data);
  }

  return bundle;
@ -223,77 +268,89 @@ CURLcode Curl_conncache_add_conn(struct conncache *connc,
  struct connectbundle *new_bundle = NULL;
  struct Curl_easy *data = conn->data;

+  /* *find_bundle() locks the connection cache */
  bundle = Curl_conncache_find_bundle(conn, data->state.conn_cache);
  if(!bundle) {
    int rc;
    char key[128];

    result = bundle_create(data, &new_bundle);
-    if(result)
-      return result;
+    if(result) {
+      goto unlock;
+    }

    hashkey(conn, key, sizeof(key));
-    CONN_LOCK(data);
    rc = conncache_add_bundle(data->state.conn_cache, key, new_bundle);
-    CONN_UNLOCK(data);

    if(!rc) {
      bundle_destroy(new_bundle);
-      return CURLE_OUT_OF_MEMORY;
+      result = CURLE_OUT_OF_MEMORY;
+      goto unlock;
    }
    bundle = new_bundle;
  }

-  CONN_LOCK(data);
  result = bundle_add_conn(bundle, conn);
  if(result) {
    if(new_bundle)
      conncache_remove_bundle(data->state.conn_cache, new_bundle);
-    CONN_UNLOCK(data);
-    return result;
+    goto unlock;
  }
-  CONN_UNLOCK(data);

  conn->connection_id = connc->next_connection_id++;
-  connc->num_connections++;
+  connc->num_conn++;

  DEBUGF(infof(conn->data, "Added connection %ld. "
               "The cache now contains %" CURL_FORMAT_CURL_OFF_TU " members\n",
-               conn->connection_id, (curl_off_t) connc->num_connections));
+               conn->connection_id, (curl_off_t) connc->num_conn));
+
+  unlock:
+  CONN_UNLOCK(data);

  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) {
+    if(lock) {
      CONN_LOCK(conn->data);
+    }
    bundle_remove_conn(bundle, conn);
    if(bundle->num_connections == 0)
      conncache_remove_bundle(connc, bundle);
-    CONN_UNLOCK(conn->data);
+    conn->bundle = NULL; /* removed from it */
    if(connc) {
-      connc->num_connections--;
-
+      connc->num_conn--;
      DEBUGF(infof(conn->data, "The cache now contains %"
                   CURL_FORMAT_CURL_OFF_TU " members\n",
-                   (curl_off_t) connc->num_connections));
+                   (curl_off_t) connc->num_conn));
+    }
+    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))
@ -303,7 +360,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);
@ -324,11 +381,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
@ -363,16 +421,104 @@ Curl_conncache_find_first_connection(struct conncache *connc)
 }

 /*
- * This function finds the connection in the connection
- * cache that has been unused for the longest time.
+ * Give ownership of a connection back to the connection cache. Might
+ * disconnect the oldest existing in there to make space.
+ *
+ * Return TRUE if stored, FALSE if closed.
+ */
+bool Curl_conncache_return_conn(struct connectdata *conn)
+{
+  struct Curl_easy *data = conn->data;
+
+  /* data->multi->maxconnects can be negative, deal with it. */
+  size_t maxconnects =
+    (data->multi->maxconnects < 0) ? data->multi->num_easy * 4:
+    data->multi->maxconnects;
+  struct connectdata *conn_candidate = NULL;
+
+  if(maxconnects > 0 &&
+     Curl_conncache_size(data) > maxconnects) {
+    infof(data, "Connection cache is full, closing the oldest one.\n");
+
+    conn_candidate = Curl_conncache_extract_oldest(data);
+
+    if(conn_candidate) {
+      /* Set the connection's owner correctly */
+      conn_candidate->data = data;
+
+      /* the winner gets the honour of being disconnected */
+      (void)Curl_disconnect(conn_candidate, /* dead_connection */ FALSE);
+    }
+  }
+  CONN_LOCK(data);
+  conn->inuse = FALSE; /* Mark the connection unused */
+  CONN_UNLOCK(data);
+
+  return (conn_candidate == conn) ? FALSE : TRUE;
+
+}
+
+/*
+ * This function finds the connection in the connection bundle that has been
+ * unused for the longest time.
+ *
+ * Does not lock the connection cache!
 *
 * Returns the pointer to the oldest idle connection, or NULL if none was
 * found.
 */
 struct connectdata *
-Curl_conncache_oldest_idle(struct Curl_easy *data)
+Curl_conncache_extract_bundle(struct Curl_easy *data,
+                              struct connectbundle *bundle)
 {
-  struct conncache *bc = data->state.conn_cache;
+  struct curl_llist_element *curr;
+  timediff_t highscore = -1;
+  timediff_t score;
+  struct curltime now;
+  struct connectdata *conn_candidate = NULL;
+  struct connectdata *conn;
+
+  (void)data;
+
+  now = Curl_now();
+
+  curr = bundle->conn_list.head;
+  while(curr) {
+    conn = curr->ptr;
+
+    if(!conn->inuse) {
+      /* Set higher score for the age passed since the connection was used */
+      score = Curl_timediff(now, conn->now);
+
+      if(score > highscore) {
+        highscore = score;
+        conn_candidate = conn;
+      }
+    }
+    curr = curr->next;
+  }
+  if(conn_candidate) {
+    /* remove it to prevent another thread from nicking it */
+    bundle_remove_conn(bundle, conn_candidate);
+    data->state.conn_cache->num_conn--;
+    DEBUGF(infof(data, "The cache now contains %"
+                 CURL_FORMAT_CURL_OFF_TU " members\n",
+                 (curl_off_t) data->state.conn_cache->num_conn));
+  }
+
+  return conn_candidate;
+}
+
+/*
+ * This function finds the connection in the connection cache that has been
+ * unused for the longest time and extracts that from the bundle.
+ *
+ * Returns the pointer to the connection, or NULL if none was found.
+ */
+struct connectdata *
+Curl_conncache_extract_oldest(struct Curl_easy *data)
+{
+  struct conncache *connc = data->state.conn_cache;
  struct curl_hash_iterator iter;
  struct curl_llist_element *curr;
  struct curl_hash_element *he;
@ -381,11 +527,12 @@ Curl_conncache_oldest_idle(struct Curl_easy *data)
  struct curltime now;
  struct connectdata *conn_candidate = NULL;
  struct connectbundle *bundle;
+  struct connectbundle *bundle_candidate = NULL;

  now = Curl_now();

  CONN_LOCK(data);
-  Curl_hash_start_iterate(&bc->hash, &iter);
+  Curl_hash_start_iterate(&connc->hash, &iter);

  he = Curl_hash_next_element(&iter);
  while(he) {
@ -404,6 +551,7 @@ Curl_conncache_oldest_idle(struct Curl_easy *data)
        if(score > highscore) {
          highscore = score;
          conn_candidate = conn;
+          bundle_candidate = bundle;
        }
      }
      curr = curr->next;
@ -411,6 +559,14 @@ Curl_conncache_oldest_idle(struct Curl_easy *data)

    he = Curl_hash_next_element(&iter);
  }
+  if(conn_candidate) {
+    /* remove it to prevent another thread from nicking it */
+    bundle_remove_conn(bundle_candidate, conn_candidate);
+    connc->num_conn--;
+    DEBUGF(infof(data, "The cache now contains %"
+                 CURL_FORMAT_CURL_OFF_TU " members\n",
+                 (curl_off_t) connc->num_conn));
+  }
  CONN_UNLOCK(data);

  return conn_candidate;
--- a/lib/conncache.h
+++ b/lib/conncache.h
@ -23,9 +23,15 @@
 *
 ***************************************************************************/

+/*
+ * All accesses to struct fields and changing of data in the connection cache
+ * and connectbundles must be done with the conncache LOCKED. The cache might
+ * be shared.
+ */
+
 struct conncache {
  struct curl_hash hash;
-  size_t num_connections;
+  size_t num_conn;
  long next_connection_id;
  struct curltime last_cleanup;
  /* handle used for closing cached connections */
@ -50,14 +56,17 @@ void Curl_conncache_destroy(struct conncache *connc);
 /* return the correct bundle, to a host or a proxy */
 struct connectbundle *Curl_conncache_find_bundle(struct connectdata *conn,
                                                 struct conncache *connc);
+void Curl_conncache_unlock(struct connectdata *conn);
+/* returns number of connections currently held in the connection cache */
+size_t Curl_conncache_size(struct Curl_easy *data);
+size_t Curl_conncache_bundle_size(struct connectdata *conn);

+bool Curl_conncache_return_conn(struct connectdata *conn);
 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,
@ -67,7 +76,10 @@ struct connectdata *
 Curl_conncache_find_first_connection(struct conncache *connc);

 struct connectdata *
-Curl_conncache_oldest_idle(struct Curl_easy *data);
+Curl_conncache_extract_bundle(struct Curl_easy *data,
+                              struct connectbundle *bundle);
+struct connectdata *
+Curl_conncache_extract_oldest(struct Curl_easy *data);
 void Curl_conncache_close_all_connections(struct conncache *connc);
 void Curl_conncache_print(struct conncache *connc);

--- a/lib/multi.c
+++ b/lib/multi.c
@ -479,38 +479,6 @@ static void debug_print_sock_hash(void *p)
 }
 #endif

-/* Mark the connection as 'idle', or close it if the cache is full.
-   Returns TRUE if the connection is kept, or FALSE if it was closed. */
-static bool
-ConnectionDone(struct Curl_easy *data, struct connectdata *conn)
-{
-  /* data->multi->maxconnects can be negative, deal with it. */
-  size_t maxconnects =
-    (data->multi->maxconnects < 0) ? data->multi->num_easy * 4:
-    data->multi->maxconnects;
-  struct connectdata *conn_candidate = NULL;
-
-  /* Mark the current connection as 'unused' */
-  conn->inuse = FALSE;
-
-  if(maxconnects > 0 &&
-     data->state.conn_cache->num_connections > maxconnects) {
-    infof(data, "Connection cache is full, closing the oldest one.\n");
-
-    conn_candidate = Curl_conncache_oldest_idle(data);
-
-    if(conn_candidate) {
-      /* Set the connection's owner correctly */
-      conn_candidate->data = data;
-
-      /* the winner gets the honour of being disconnected */
-      (void)Curl_disconnect(conn_candidate, /* dead_connection */ FALSE);
-    }
-  }
-
-  return (conn_candidate == conn) ? FALSE : TRUE;
-}
-
 static CURLcode multi_done(struct connectdata **connp,
                          CURLcode status,  /* an error if this is called
                                               after an error was detected */
@ -621,17 +589,21 @@ static CURLcode multi_done(struct connectdata **connp,
      result = res2;
  }
  else {
-    /* the connection is no longer in use */
-    if(ConnectionDone(data, conn)) {
-      /* remember the most recently used connection */
-      data->state.lastconnect = conn;
-
-      infof(data, "Connection #%ld to host %s left intact\n",
+    char buffer[256];
+    /* create string before returning the connection */
+    snprintf(buffer, sizeof(buffer),
+             "Connection #%ld to host %s left intact",
             conn->connection_id,
             conn->bits.socksproxy ? conn->socks_proxy.host.dispname :
             conn->bits.httpproxy ? conn->http_proxy.host.dispname :
             conn->bits.conn_to_host ? conn->conn_to_host.dispname :
             conn->host.dispname);
+
+    /* the connection is no longer in use */
+    if(Curl_conncache_return_conn(conn)) {
+      /* remember the most recently used connection */
+      data->state.lastconnect = conn;
+      infof(data, "%s\n", buffer);
    }
    else
      data->state.lastconnect = NULL;
@ -1357,16 +1329,16 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
    }

    if(data->easy_conn && data->mstate > CURLM_STATE_CONNECT &&
-       data->mstate < CURLM_STATE_COMPLETED)
+       data->mstate < CURLM_STATE_COMPLETED) {
      /* Make sure we set the connection's current owner */
      data->easy_conn->data = data;
+    }

    if(data->easy_conn &&
       (data->mstate >= CURLM_STATE_CONNECT) &&
       (data->mstate < CURLM_STATE_COMPLETED)) {
      /* we need to wait for the connect state as only then is the start time
         stored, but we must not check already completed handles */
-
      timeout_ms = Curl_timeleft(data, &now,
                                 (data->mstate <= CURLM_STATE_WAITDO)?
                                 TRUE:FALSE);
--- a/lib/url.c
+++ b/lib/url.c
@ -127,10 +127,6 @@ bool curl_win32_idn_to_ascii(const char *in, char **out);
 #include "curl_memory.h"
 #include "memdebug.h"

-/* Local static prototypes */
-static struct connectdata *
-find_oldest_idle_connection_in_bundle(struct Curl_easy *data,
-                                      struct connectbundle *bundle);
 static void conn_free(struct connectdata *conn);
 static void free_fixed_hostname(struct hostname *host);
 static void signalPipeClose(struct curl_llist *pipeline, bool pipe_broke);
@ -722,7 +718,6 @@ static void conn_free(struct connectdata *conn)
 #ifdef USE_SSL
  Curl_safefree(conn->ssl_extra);
 #endif
-
  free(conn); /* free all the connection oriented data */
 }

@ -777,7 +772,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);
@ -943,55 +938,16 @@ proxy_info_matches(const struct proxy_info* data,
  return FALSE;
 }

-
 /*
- * This function finds the connection in the connection
- * bundle that has been unused for the longest time.
+ * This function checks if the given connection is dead and extracts it from
+ * the connection cache if so.
 *
- * Returns the pointer to the oldest idle connection, or NULL if none was
- * found.
- */
-static struct connectdata *
-find_oldest_idle_connection_in_bundle(struct Curl_easy *data,
-                                      struct connectbundle *bundle)
-{
-  struct curl_llist_element *curr;
-  timediff_t highscore = -1;
-  timediff_t score;
-  struct curltime now;
-  struct connectdata *conn_candidate = NULL;
-  struct connectdata *conn;
-
-  (void)data;
-
-  now = Curl_now();
-
-  curr = bundle->conn_list.head;
-  while(curr) {
-    conn = curr->ptr;
-
-    if(!conn->inuse) {
-      /* Set higher score for the age passed since the connection was used */
-      score = Curl_timediff(now, conn->now);
-
-      if(score > highscore) {
-        highscore = score;
-        conn_candidate = conn;
-      }
-    }
-    curr = curr->next;
-  }
-
-  return conn_candidate;
-}
-
-/*
- * This function checks if given connection is dead and disconnects if so.
- * (That also removes it from the connection cache.)
+ * When this is called as a Curl_conncache_foreach() callback, the connection
+ * cache lock is held!
 *
- * Returns TRUE if the connection actually was dead and disconnected.
+ * Returns TRUE if the connection was dead and extracted.
 */
-static bool disconnect_if_dead(struct connectdata *conn,
+static bool extract_if_dead(struct connectdata *conn,
                            struct Curl_easy *data)
 {
  size_t pipeLen = conn->send_pipe.size + conn->recv_pipe.size;
@ -1017,25 +973,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 +1011,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;
  }
 }
@ -1106,8 +1073,8 @@ ConnectionExists(struct Curl_easy *data,
     Curl_pipeline_site_blacklisted(data, needle))
    canpipe &= ~ CURLPIPE_HTTP1;

-  /* Look up the bundle with all the connections to this
-     particular host */
+  /* Look up the bundle with all the connections to this particular host.
+     Locks the connection cache, beware of early returns! */
  bundle = Curl_conncache_find_bundle(needle, data->state.conn_cache);
  if(bundle) {
    /* Max pipe length is zero (unlimited) for multiplexed connections */
@ -1130,6 +1097,7 @@ ConnectionExists(struct Curl_easy *data,
        if((bundle->multiuse == BUNDLE_UNKNOWN) && data->set.pipewait) {
          infof(data, "Server doesn't support multi-use yet, wait\n");
          *waitpipe = TRUE;
+          Curl_conncache_unlock(needle);
          return FALSE; /* no re-use */
        }

@ -1161,8 +1129,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;

@ -1479,9 +1450,13 @@ ConnectionExists(struct Curl_easy *data,
  }

  if(chosen) {
+    /* mark it as used before releasing the lock */
+    chosen->inuse = TRUE;
+    Curl_conncache_unlock(needle);
    *usethis = chosen;
    return TRUE; /* yes, we found one to use! */
  }
+  Curl_conncache_unlock(needle);

  if(foundPendingCandidate && data->set.pipewait) {
    infof(data,
@ -4409,20 +4384,21 @@ static CURLcode create_conn(struct Curl_easy *data,
  else
    reuse = ConnectionExists(data, conn, &conn_temp, &force_reuse, &waitpipe);

-  /* If we found a reusable connection, we may still want to
-     open a new connection if we are pipelining. */
+  /* If we found a reusable connection that is now marked as in use, we may
+     still want to open a new connection if we are pipelining. */
  if(reuse && !force_reuse && IsPipeliningPossible(data, conn_temp)) {
    size_t pipelen = conn_temp->send_pipe.size + conn_temp->recv_pipe.size;
    if(pipelen > 0) {
      infof(data, "Found connection %ld, with requests in the pipe (%zu)\n",
            conn_temp->connection_id, pipelen);

-      if(conn_temp->bundle->num_connections < max_host_connections &&
-         data->state.conn_cache->num_connections < max_total_connections) {
+      if(Curl_conncache_bundle_size(conn_temp) < max_host_connections &&
+         Curl_conncache_size(data) < max_total_connections) {
        /* We want a new connection anyway */
        reuse = FALSE;

        infof(data, "We can reuse, but we want a new connection anyway\n");
+        Curl_conncache_return_conn(conn_temp);
      }
    }
  }
@ -4434,8 +4410,6 @@ static CURLcode create_conn(struct Curl_easy *data,
     * just allocated before we can move along and use the previously
     * existing one.
     */
-    conn_temp->inuse = TRUE; /* mark this as being in use so that no other
-                                handle in a multi stack may nick it */
    reuse_conn(conn, conn_temp);
 #ifdef USE_SSL
    free(conn->ssl_extra);
@ -4455,7 +4429,6 @@ static CURLcode create_conn(struct Curl_easy *data,
    /* We have decided that we want a new connection. However, we may not
       be able to do that if we have reached the limit of how many
       connections we are allowed to open. */
-    struct connectbundle *bundle = NULL;

    if(conn->handler->flags & PROTOPT_ALPN_NPN) {
      /* The protocol wants it, so set the bits if enabled in the easy handle
@ -4470,15 +4443,18 @@ static CURLcode create_conn(struct Curl_easy *data,
      /* There is a connection that *might* become usable for pipelining
         "soon", and we wait for that */
      connections_available = FALSE;
-    else
-      bundle = Curl_conncache_find_bundle(conn, data->state.conn_cache);
+    else {
+      /* this gets a lock on the conncache */
+      struct connectbundle *bundle =
+        Curl_conncache_find_bundle(conn, data->state.conn_cache);

      if(max_host_connections > 0 && bundle &&
         (bundle->num_connections >= max_host_connections)) {
        struct connectdata *conn_candidate;

-      /* The bundle is full. Let's see if we can kill a connection. */
-      conn_candidate = find_oldest_idle_connection_in_bundle(data, bundle);
+        /* The bundle is full. Extract the oldest connection. */
+        conn_candidate = Curl_conncache_extract_bundle(data, bundle);
+        Curl_conncache_unlock(conn);

        if(conn_candidate) {
          /* Set the connection's owner correctly, then kill it */
@ -4491,14 +4467,18 @@ static CURLcode create_conn(struct Curl_easy *data,
          connections_available = FALSE;
        }
      }
+      else
+        Curl_conncache_unlock(conn);
+
+    }

    if(connections_available &&
       (max_total_connections > 0) &&
-       (data->state.conn_cache->num_connections >= max_total_connections)) {
+       (Curl_conncache_size(data) >= max_total_connections)) {
      struct connectdata *conn_candidate;

      /* The cache is full. Let's see if we can kill a connection. */
-      conn_candidate = Curl_conncache_oldest_idle(data);
+      conn_candidate = Curl_conncache_extract_oldest(data);

      if(conn_candidate) {
        /* Set the connection's owner correctly, then kill it */
@ -4521,6 +4501,9 @@ static CURLcode create_conn(struct Curl_easy *data,
      goto out;
    }
    else {
+      /* Mark the connection as used, before we add it */
+      conn->inuse = TRUE;
+
      /*
       * This is a brand new connection, so let's store it in the connection
       * cache of ours!
@ -4548,9 +4531,6 @@ static CURLcode create_conn(struct Curl_easy *data,
 #endif
  }

-  /* Mark the connection as used */
-  conn->inuse = TRUE;
-
  /* Setup and init stuff before DO starts, in preparing for the transfer. */
  Curl_init_do(data, conn);

--- a/lib/urldata.h
+++ b/lib/urldata.h
@ -775,9 +775,10 @@ struct connectdata {
  void *closesocket_client;

  bool inuse; /* This is a marker for the connection cache logic. If this is
-                 TRUE this handle is being used by an easy handle and cannot
-                 be used by any other easy handle without careful
-                 consideration (== only for pipelining). */
+                 TRUE this handle is being used by one or more easy handles
+                 and can only used by any other easy handle without careful
+                 consideration (== only for pipelining/multiplexing) and it
+                 cannot be used by another multi handle! */

  /**** Fields set when inited and not modified again */
  long connection_id; /* Contains a unique number to make it easier to
@ -1325,6 +1326,9 @@ struct UrlState {
  struct Curl_easy *stream_depends_on;
  bool stream_depends_e; /* set or don't set the Exclusive bit */
  int stream_weight;
+#ifdef CURLDEBUG
+  bool conncache_lock;
+#endif
 };


--- a/tests/data/test1554
+++ b/tests/data/test1554
@ -29,10 +29,6 @@ run 1: foobar and so on fun!
 <- Mutex unlock
 -> Mutex lock
 <- Mutex unlock
-> Mutex lock
-<- Mutex unlock
-> Mutex lock
-<- Mutex unlock
 run 1: foobar and so on fun!
 -> Mutex lock
 <- Mutex unlock
@ -40,6 +36,10 @@ run 1: foobar and so on fun!
 <- Mutex unlock
 -> Mutex lock
 <- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
 run 1: foobar and so on fun!
 -> Mutex lock
 <- Mutex unlock
@ -47,11 +47,19 @@ run 1: foobar and so on fun!
 <- Mutex unlock
 -> Mutex lock
 <- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
 run 1: foobar and so on fun!
 -> Mutex lock
 <- Mutex unlock
 -> Mutex lock
 <- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
 </datacheck>
 </reply>