http2: several cleanups

- separate easy handle from connections better
- added asserts on a number of places
- added sanity check of pipelines for debug builds

Closes #2751

lib/hostasyn.c

@@ -131,7 +131,7 @@ CURLcode Curl_async_resolved(struct connectdata *conn,
   if(result)
     /* We're not allowed to return failure with memory left allocated
        in the connectdata struct, free those here */
-    Curl_disconnect(conn->data, conn, FALSE); /* close the connection */
+    Curl_disconnect(conn->data, conn, TRUE); /* close the connection */
 
   return result;
 }

lib/http.c

@@ -158,18 +158,20 @@ CURLcode Curl_http_setup_conn(struct connectdata *conn)
   /* allocate the HTTP-specific struct for the Curl_easy, only to survive
      during this request */
   struct HTTP *http;
-  DEBUGASSERT(conn->data->req.protop == NULL);
+  struct Curl_easy *data = conn->data;
+  DEBUGASSERT(data->req.protop == NULL);
 
   http = calloc(1, sizeof(struct HTTP));
   if(!http)
     return CURLE_OUT_OF_MEMORY;
 
   Curl_mime_initpart(&http->form, conn->data);
-  conn->data->req.protop = http;
-
-  Curl_http2_setup_conn(conn);
-  Curl_http2_setup_req(conn->data);
+  data->req.protop = http;
 
+  if(!CONN_INUSE(conn))
+    /* if not alredy multi-using, setup connection details */
+    Curl_http2_setup_conn(conn);
+  Curl_http2_setup_req(data);
   return CURLE_OK;
 }
 
@@ -1913,6 +1915,7 @@ CURLcode Curl_http(struct connectdata *conn, bool *done)
   }
 
   http = data->req.protop;
+  DEBUGASSERT(http);
 
   if(!data->state.this_is_a_follow) {
     /* Free to avoid leaking memory on multiple requests*/

lib/http2.c

@@ -154,6 +154,11 @@ static void http2_stream_free(struct HTTP *http)
   }
 }
 
+/*
+ * Disconnects *a* connection used for HTTP/2. It might be an old one from the
+ * connection cache and not the "main" one. Don't touch the easy handle!
+ */
+
 static CURLcode http2_disconnect(struct connectdata *conn,
                                  bool dead_connection)
 {
@@ -164,8 +169,6 @@ static CURLcode http2_disconnect(struct connectdata *conn,
 
   nghttp2_session_del(c->h2);
   Curl_safefree(c->inbuf);
-  http2_stream_free(conn->data->req.protop);
-  conn->data->state.drain = 0;
 
   H2BUGF(infof(conn->data, "HTTP/2 DISCONNECT done\n"));
 
@@ -520,6 +523,7 @@ static int push_promise(struct Curl_easy *data,
     if(rv) {
       /* denied, kill off the new handle again */
       http2_stream_free(newhandle->req.protop);
+      newhandle->req.protop = NULL;
       (void)Curl_close(newhandle);
       goto fail;
     }
@@ -535,6 +539,7 @@ static int push_promise(struct Curl_easy *data,
     if(rc) {
       infof(data, "failed to add handle to multi\n");
       http2_stream_free(newhandle->req.protop);
+      newhandle->req.protop = NULL;
       Curl_close(newhandle);
       rv = 1;
       goto fail;

lib/http2.h

@@ -7,7 +7,7 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2017, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
@@ -65,7 +65,7 @@ void Curl_http2_cleanup_dependencies(struct Curl_easy *data);
 #define Curl_http2_request_upgrade(x,y) CURLE_UNSUPPORTED_PROTOCOL
 #define Curl_http2_setup(x) CURLE_UNSUPPORTED_PROTOCOL
 #define Curl_http2_switched(x,y,z) CURLE_UNSUPPORTED_PROTOCOL
-#define Curl_http2_setup_conn(x)
+#define Curl_http2_setup_conn(x) Curl_nop_stmt
 #define Curl_http2_setup_req(x)
 #define Curl_http2_init_state(x)
 #define Curl_http2_init_userset(x)

lib/pipeline.c

@@ -6,7 +6,7 @@
  *                             \___|\___/|_| \_\_____|
  *
  * Copyright (C) 2013, Linus Nielsen Feltzing, <linus@haxx.se>
- * Copyright (C) 2013 - 2017, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 2013 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
@@ -110,8 +110,8 @@ CURLcode Curl_add_handle_to_pipeline(struct Curl_easy *handle,
   pipeline = &conn->send_pipe;
 
   result = addHandleToPipeline(handle, pipeline);
-
-  if(pipeline == &conn->send_pipe && sendhead != conn->send_pipe.head) {
+  if((conn->bundle->multiuse == BUNDLE_PIPELINING) &&
+     (pipeline == &conn->send_pipe && sendhead != conn->send_pipe.head)) {
     /* this is a new one as head, expire it */
     Curl_pipeline_leave_write(conn); /* not in use yet */
     Curl_expire(conn->send_pipe.head->ptr, 0, EXPIRE_RUN_NOW);

lib/url.c

@@ -127,7 +127,6 @@ bool curl_win32_idn_to_ascii(const char *in, char **out);
 
 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);
 static CURLcode parse_url_login(struct Curl_easy *data,
                                 struct connectdata *conn,
                                 char **userptr, char **passwdptr,
@@ -750,7 +749,7 @@ CURLcode Curl_disconnect(struct Curl_easy *data,
     return CURLE_OK; /* this is closed and fine already */
 
   if(!data) {
-    DEBUGF(fprintf(stderr, "DISCONNECT without easy handle, ignoring\n"));
+    DEBUGF(infof(data, "DISCONNECT without easy handle, ignoring\n"));
     return CURLE_OK;
   }
 
@@ -758,9 +757,8 @@ CURLcode Curl_disconnect(struct Curl_easy *data,
    * If this connection isn't marked to force-close, leave it open if there
    * are other users of it
    */
-  if(!conn->bits.close && CONN_INUSE(conn)) {
-    DEBUGF(fprintf(stderr, "Curl_disconnect when inuse: %d\n",
-                   CONN_INUSE(conn)));
+  if(CONN_INUSE(conn) && !dead_connection) {
+    DEBUGF(infof(data, "Curl_disconnect when inuse: %d\n", CONN_INUSE(conn)));
     return CURLE_OK;
   }
 
@@ -792,14 +790,7 @@ CURLcode Curl_disconnect(struct Curl_easy *data,
 
   Curl_ssl_close(conn, FIRSTSOCKET);
 
-  /* Indicate to all handles on the pipe that we're dead */
-  if(Curl_pipeline_wanted(data->multi, CURLPIPE_ANY)) {
-    signalPipeClose(&conn->send_pipe, TRUE);
-    signalPipeClose(&conn->recv_pipe, TRUE);
-  }
-
   conn_free(conn);
-
   return CURLE_OK;
 }
 
@@ -888,6 +879,16 @@ static void Curl_printPipeline(struct curl_llist *pipeline)
 static struct Curl_easy* gethandleathead(struct curl_llist *pipeline)
 {
   struct curl_llist_element *curr = pipeline->head;
+#ifdef DEBUGBUILD
+  {
+    struct curl_llist_element *p = pipeline->head;
+    while(p) {
+      struct Curl_easy *e = p->ptr;
+      DEBUGASSERT(GOOD_EASY_HANDLE(e));
+      p = p->next;
+    }
+  }
+#endif
   if(curr) {
     return (struct Curl_easy *) curr->ptr;
   }
@@ -920,33 +921,6 @@ void Curl_getoff_all_pipelines(struct Curl_easy *data,
   }
 }
 
-static void signalPipeClose(struct curl_llist *pipeline, bool pipe_broke)
-{
-  struct curl_llist_element *curr;
-
-  if(!pipeline)
-    return;
-
-  curr = pipeline->head;
-  while(curr) {
-    struct curl_llist_element *next = curr->next;
-    struct Curl_easy *data = (struct Curl_easy *) curr->ptr;
-
-#ifdef DEBUGBUILD /* debug-only code */
-    if(data->magic != CURLEASY_MAGIC_NUMBER) {
-      /* MAJOR BADNESS */
-      infof(data, "signalPipeClose() found BAAD easy handle\n");
-    }
-#endif
-
-    if(pipe_broke)
-      data->state.pipe_broke = TRUE;
-    Curl_multi_handlePipeBreak(data);
-    Curl_llist_remove(pipeline, curr, NULL);
-    curr = next;
-  }
-}
-
 static bool
 proxy_info_matches(const struct proxy_info* data,
                    const struct proxy_info* needle)
@@ -2498,18 +2472,6 @@ static CURLcode setup_connection_internals(struct connectdata *conn)
 {
   const struct Curl_handler * p;
   CURLcode result;
-  struct Curl_easy *data = conn->data;
-
-  /* in some case in the multi state-machine, we go back to the CONNECT state
-     and then a second (or third or...) call to this function will be made
-     without doing a DISCONNECT or DONE in between (since the connection is
-     yet in place) and therefore this function needs to first make sure
-     there's no lingering previous data allocated. */
-  Curl_free_request_state(data);
-
-  memset(&data->req, 0, sizeof(struct SingleRequest));
-  data->req.maxdownload = -1;
-
   conn->socktype = SOCK_STREAM; /* most of them are TCP streams */
 
   /* Perform setup complement if some. */
@@ -4675,12 +4637,16 @@ CURLcode Curl_connect(struct Curl_easy *data,
 
   *asyncp = FALSE; /* assume synchronous resolves by default */
 
+  /* init the single-transfer specific data */
+  Curl_free_request_state(data);
+  memset(&data->req, 0, sizeof(struct SingleRequest));
+  data->req.maxdownload = -1;
+
   /* call the stuff that needs to be called */
   result = create_conn(data, in_connect, asyncp);
 
   if(!result) {
-    /* no error */
-    if((*in_connect)->send_pipe.size || (*in_connect)->recv_pipe.size)
+    if(CONN_INUSE(*in_connect))
       /* pipelining */
       *protocol_done = TRUE;
     else if(!*asyncp) {
@@ -4695,11 +4661,10 @@ CURLcode Curl_connect(struct Curl_easy *data,
     *in_connect = NULL;
     return result;
   }
-
-  if(result && *in_connect) {
+  else if(result && *in_connect) {
     /* We're not allowed to return failure with memory left allocated in the
        connectdata struct, free those here */
-    Curl_disconnect(data, *in_connect, FALSE);
+    Curl_disconnect(data, *in_connect, TRUE);
     *in_connect = NULL; /* return a NULL */
   }
 

lib/urldata.h

@@ -786,7 +786,7 @@ struct connectdata {
      other easy handle without careful consideration (== only for
      pipelining/multiplexing) and it cannot be used by another multi
      handle! */
-#define CONN_INUSE(c) ((c)->send_pipe.size || (c)->recv_pipe.size)
+#define CONN_INUSE(c) ((c)->send_pipe.size + (c)->recv_pipe.size)
 
   /**** Fields set when inited and not modified again */
   long connection_id; /* Contains a unique number to make it easier to