http2: make sure stream errors don't needlessly close the connection

With HTTP/2 each transfer is made in an indivial logical stream over the
connection, making most previous errors that caused the connection to get
forced-closed now instead just kill the stream and not the connection.

Fixes #941

9 files changed, 138 insertions, 85 deletions

diff --git a/lib/connect.c b/lib/connect.c
index 7f8c0870a..eca817c35 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -1371,25 +1371,26 @@ CURLcode Curl_socket(struct connectdata *conn,
 
 }
 
-#ifdef CURLDEBUG
 /*
- * Curl_conncontrol() is used to set the conn->bits.close bit on or off. It
- * MUST be called with the connclose() or connkeep() macros with a stated
- * reason. The reason is only shown in debug builds but helps to figure out
- * decision paths when connections are or aren't re-used as expected.
+ * Curl_conncontrol() marks streams or connection for closure.
  */
-void Curl_conncontrol(struct connectdata *conn, bool closeit,
-                      const char *reason)
-{
-#if defined(CURL_DISABLE_VERBOSE_STRINGS)
-  (void) reason;
+void Curl_conncontrol(struct connectdata *conn,
+                      int ctrl /* see defines in header */
+#ifdef CURLDEBUG
+                      , const char *reason
 #endif
-  if(closeit != conn->bits.close) {
-    infof(conn->data, "Marked for [%s]: %s\n", closeit?"closure":"keep alive",
-          reason);
-
+  )
+{
+  /* close if a connection, or a stream that isn't multiplexed */
+  bool closeit = (ctrl == CONNCTRL_CONNECTION) ||
+    ((ctrl == CONNCTRL_STREAM) && !(conn->handler->flags & PROTOPT_STREAM));
+  if((ctrl == CONNCTRL_STREAM) &&
+     (conn->handler->flags & PROTOPT_STREAM))
+    DEBUGF(infof(conn->data, "Kill stream: %s\n", reason));
+  else if(closeit != conn->bits.close) {
+    DEBUGF(infof(conn->data, "Marked for [%s]: %s\n",
+                 closeit?"closure":"keep alive", reason));
     conn->bits.close = closeit; /* the only place in the source code that
                                    should assign this bit */
   }
 }
-#endif
diff --git a/lib/connect.h b/lib/connect.h
index 6d60e0d81..c3e43cf66 100644
--- a/lib/connect.h
+++ b/lib/connect.h
@@ -7,7 +7,7 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2015, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2016, 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
@@ -104,21 +104,37 @@ CURLcode Curl_socket(struct connectdata *conn,
 
 void Curl_tcpnodelay(struct connectdata *conn, curl_socket_t sockfd);
 
-#ifdef CURLDEBUG
 /*
- * Curl_connclose() sets the bit.close bit to TRUE with an explanation.
- * Nothing else.
+ * Curl_conncontrol() marks the end of a connection/stream. The 'closeit'
+ * argument specifies if it is the end of a connection or a stream.
+ *
+ * For stream-based protocols (such as HTTP/2), a stream close will not cause
+ * a connection close. Other protocols will close the connection for both
+ * cases.
+ *
+ * It sets the bit.close bit to TRUE (with an explanation for debug builds),
+ * when the connection will close.
  */
-void Curl_conncontrol(struct connectdata *conn,
-                      bool closeit,
-                      const char *reason);
-#define connclose(x,y) Curl_conncontrol(x,TRUE, y)
-#define connkeep(x,y) Curl_conncontrol(x, FALSE, y)
-#else /* if !CURLDEBUG */
 
-#define connclose(x,y) (x)->bits.close = TRUE
-#define connkeep(x,y) (x)->bits.close = FALSE
+#define CONNCTRL_KEEP 0 /* undo a marked closure */
+#define CONNCTRL_CONNECTION 1
+#define CONNCTRL_STREAM 2
 
+void Curl_conncontrol(struct connectdata *conn,
+                      int closeit
+#ifdef CURLDEBUG
+                      , const char *reason
+#endif
+  );
+
+#ifdef CURLDEBUG
+#define streamclose(x,y) Curl_conncontrol(x, CONNCTRL_STREAM, y)
+#define connclose(x,y) Curl_conncontrol(x, CONNCTRL_CONNECTION, y)
+#define connkeep(x,y) Curl_conncontrol(x, CONNCTRL_KEEP, y)
+#else /* if !CURLDEBUG */
+#define streamclose(x,y) Curl_conncontrol(x, CONNCTRL_STREAM)
+#define connclose(x,y) Curl_conncontrol(x, CONNCTRL_CONNECTION)
+#define connkeep(x,y) Curl_conncontrol(x, CONNCTRL_KEEP)
 #endif
 
 #endif /* HEADER_CURL_CONNECT_H */
diff --git a/lib/http.c b/lib/http.c
index 087d1af02..e4b9d8b4b 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -462,7 +462,7 @@ static CURLcode http_perhapsrewind(struct connectdata *conn)
 #endif
 
     /* This is not NTLM or many bytes left to send: close */
-    connclose(conn, "Mid-auth HTTP and much data left to send");
+    streamclose(conn, "Mid-auth HTTP and much data left to send");
     data->req.size = 0; /* don't download any more than 0 bytes */
 
     /* There still is data left to send, but this connection is marked for
@@ -1452,9 +1452,8 @@ CURLcode Curl_http_done(struct connectdata *conn,
 {
   struct Curl_easy *data = conn->data;
   struct HTTP *http = data->req.protop;
-#ifdef USE_NGHTTP2
-  struct http_conn *httpc = &conn->proto.httpc;
-#endif
+
+  infof(data, "Curl_http_done: called premature == %d\n", premature);
 
   Curl_unencode_cleanup(conn);
 
@@ -1467,7 +1466,7 @@ CURLcode Curl_http_done(struct connectdata *conn,
      * Do not close CONNECT_ONLY connections. */
     if((data->req.httpcode != 401) && (data->req.httpcode != 407) &&
        !data->set.connect_only)
-      connclose(conn, "Negotiate transfer completed");
+      streamclose(conn, "Negotiate transfer completed");
     Curl_cleanup_negotiate(data);
   }
 #endif
@@ -1484,27 +1483,7 @@ CURLcode Curl_http_done(struct connectdata *conn,
     http->send_buffer = NULL; /* clear the pointer */
   }
 
-#ifdef USE_NGHTTP2
-  if(http->header_recvbuf) {
-    DEBUGF(infof(data, "free header_recvbuf!!\n"));
-    Curl_add_buffer_free(http->header_recvbuf);
-    http->header_recvbuf = NULL; /* clear the pointer */
-    Curl_add_buffer_free(http->trailer_recvbuf);
-    http->trailer_recvbuf = NULL; /* clear the pointer */
-    if(http->push_headers) {
-      /* if they weren't used and then freed before */
-      for(; http->push_headers_used > 0; --http->push_headers_used) {
-        free(http->push_headers[http->push_headers_used - 1]);
-      }
-      free(http->push_headers);
-      http->push_headers = NULL;
-    }
-  }
-  if(http->stream_id) {
-    nghttp2_session_set_stream_user_data(httpc->h2, http->stream_id, 0);
-    http->stream_id = 0;
-  }
-#endif
+  Curl_http2_done(conn, premature);
 
   if(HTTPREQ_POST_FORM == data->set.httpreq) {
     data->req.bytecount = http->readbytecount + http->writebytecount;
@@ -3118,7 +3097,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
              signal the end of the document. */
           infof(data, "no chunk, no close, no size. Assume close to "
                 "signal end\n");
-          connclose(conn, "HTTP: No end-of-message indicator");
+          streamclose(conn, "HTTP: No end-of-message indicator");
         }
       }
 
@@ -3199,7 +3178,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
              */
             if(!k->upload_done) {
               infof(data, "HTTP error before end of send, stop sending\n");
-              connclose(conn, "Stop sending data before everything sent");
+              streamclose(conn, "Stop sending data before everything sent");
               k->upload_done = TRUE;
               k->keepon &= ~KEEP_SEND; /* don't send */
               if(data->state.expect100header)
@@ -3503,7 +3482,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
         /* Negative Content-Length is really odd, and we know it
            happens for example when older Apache servers send large
            files */
-        connclose(conn, "negative content-length");
+        streamclose(conn, "negative content-length");
         infof(data, "Negative content-length: %" CURL_FORMAT_CURL_OFF_T
               ", closing after transfer\n", contentlength);
       }
@@ -3576,7 +3555,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
        * the connection will close when this request has been
        * served.
        */
-      connclose(conn, "Connection: close used");
+      streamclose(conn, "Connection: close used");
     }
     else if(checkprefix("Transfer-Encoding:", k->p)) {
       /* One or more encodings. We check for chunked and/or a compression
diff --git a/lib/http2.c b/lib/http2.c
index a14f75e62..e51e72ab4 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -185,7 +185,7 @@ const struct Curl_handler Curl_handler_http2 = {
   ZERO_NULL,                            /* readwrite */
   PORT_HTTP,                            /* defport */
   CURLPROTO_HTTP,                       /* protocol */
-  PROTOPT_NONE                          /* flags */
+  PROTOPT_STREAM                        /* flags */
 };
 
 const struct Curl_handler Curl_handler_http2_ssl = {
@@ -205,7 +205,7 @@ const struct Curl_handler Curl_handler_http2_ssl = {
   ZERO_NULL,                            /* readwrite */
   PORT_HTTP,                            /* defport */
   CURLPROTO_HTTPS,                      /* protocol */
-  PROTOPT_SSL                           /* flags */
+  PROTOPT_SSL | PROTOPT_STREAM          /* flags */
 };
 
 /*
@@ -489,8 +489,11 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame,
   }
 
   stream = data_s->req.protop;
-  if(!stream)
+  if(!stream) {
+    DEBUGF(infof(conn->data, "No proto pointer for stream: %x\n",
+                 stream_id));
     return NGHTTP2_ERR_CALLBACK_FAILURE;
+  }
 
   DEBUGF(infof(data_s, "on_frame_recv() header %x stream %x\n",
                frame->hd.type, stream_id));
@@ -979,6 +982,43 @@ static int error_callback(nghttp2_session *session,
 }
 #endif
 
+void Curl_http2_done(struct connectdata *conn, bool premature)
+{
+  struct Curl_easy *data = conn->data;
+  struct HTTP *http = data->req.protop;
+  struct http_conn *httpc = &conn->proto.httpc;
+
+  if(http->header_recvbuf) {
+    DEBUGF(infof(data, "free header_recvbuf!!\n"));
+    Curl_add_buffer_free(http->header_recvbuf);
+    http->header_recvbuf = NULL; /* clear the pointer */
+    Curl_add_buffer_free(http->trailer_recvbuf);
+    http->trailer_recvbuf = NULL; /* clear the pointer */
+    if(http->push_headers) {
+      /* if they weren't used and then freed before */
+      for(; http->push_headers_used > 0; --http->push_headers_used) {
+        free(http->push_headers[http->push_headers_used - 1]);
+      }
+      free(http->push_headers);
+      http->push_headers = NULL;
+    }
+  }
+
+  if(premature) {
+    /* RST_STREAM */
+    nghttp2_submit_rst_stream(httpc->h2, NGHTTP2_FLAG_NONE, http->stream_id,
+                              NGHTTP2_STREAM_CLOSED);
+    if(http->stream_id == httpc->pause_stream_id) {
+      infof(data, "stopped the pause stream!\n");
+      httpc->pause_stream_id = 0;
+    }
+  }
+  if(http->stream_id) {
+    nghttp2_session_set_stream_user_data(httpc->h2, http->stream_id, 0);
+    http->stream_id = 0;
+  }
+}
+
 /*
  * Initialize nghttp2 for a Curl connection
  */
@@ -1378,6 +1418,8 @@ static ssize_t http2_recv(struct connectdata *conn, int sockindex,
        socket is not read.  But it seems that usually streams are
        notified with its drain property, and socket is read again
        quickly. */
+    DEBUGF(infof(data, "stream %x is paused, pause id: %x\n",
+                 stream->stream_id, httpc->pause_stream_id));
     *err = CURLE_AGAIN;
     return -1;
   }
diff --git a/lib/http2.h b/lib/http2.h
index bedbebf16..cad578ca1 100644
--- a/lib/http2.h
+++ b/lib/http2.h
@@ -7,7 +7,7 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2015, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2016, 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
@@ -51,6 +51,7 @@ CURLcode Curl_http2_switched(struct connectdata *conn,
 /* called from Curl_http_setup_conn */
 void Curl_http2_setup_conn(struct connectdata *conn);
 void Curl_http2_setup_req(struct Curl_easy *data);
+void Curl_http2_done(struct connectdata *conn, bool premature);
 #else /* USE_NGHTTP2 */
 #define Curl_http2_init(x) CURLE_UNSUPPORTED_PROTOCOL
 #define Curl_http2_send_request(x) CURLE_UNSUPPORTED_PROTOCOL
@@ -61,6 +62,7 @@ void Curl_http2_setup_req(struct Curl_easy *data);
 #define Curl_http2_setup_req(x)
 #define Curl_http2_init_state(x)
 #define Curl_http2_init_userset(x)
+#define Curl_http2_done(x,y)
 #endif
 
 #endif /* HEADER_CURL_HTTP2_H */
diff --git a/lib/http_proxy.c b/lib/http_proxy.c
index 87f86b0c4..082b73ad7 100644
--- a/lib/http_proxy.c
+++ b/lib/http_proxy.c
@@ -574,7 +574,7 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn,
       free(data->req.newurl);
       data->req.newurl = NULL;
       /* failure, close this connection to avoid re-use */
-      connclose(conn, "proxy CONNECT failure");
+      streamclose(conn, "proxy CONNECT failure");
       Curl_closesocket(conn, conn->sock[sockindex]);
       conn->sock[sockindex] = CURL_SOCKET_BAD;
     }
diff --git a/lib/multi.c b/lib/multi.c
index 7a25103de..e1325f029 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -569,10 +569,9 @@ static CURLcode multi_done(struct connectdata **connp,
       result = CURLE_ABORTED_BY_CALLBACK;
   }
 
-  if((!premature &&
-      conn->send_pipe->size + conn->recv_pipe->size != 0 &&
-      !data->set.reuse_forbid &&
-      !conn->bits.close)) {
+  if(conn->send_pipe->size + conn->recv_pipe->size != 0 &&
+     !data->set.reuse_forbid &&
+     !conn->bits.close) {
     /* Stop if pipeline is not empty and we do not have to close
        connection. */
     DEBUGF(infof(data, "Connection still in use, no more multi_done now!\n"));
@@ -685,7 +684,7 @@ CURLMcode curl_multi_remove_handle(struct Curl_multi *multi,
     /* If the handle is in a pipeline and has started sending off its
        request but not received its response yet, we need to close
        connection. */
-    connclose(data->easy_conn, "Removed with partial response");
+    streamclose(data->easy_conn, "Removed with partial response");
     /* Set connection owner so that the DONE function closes it.  We can
        safely do this here since connection is killed. */
     data->easy_conn->data = easy;
@@ -1298,7 +1297,9 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
     return CURLM_BAD_EASY_HANDLE;
 
   do {
-    bool disconnect_conn = FALSE;
+    /* A "stream" here is a logical stream if the protocol can handle that
+       (HTTP/2), or the full connection for older protocols */
+    bool stream_error = FALSE;
     rc = CURLM_OK;
 
     /* Handle the case when the pipe breaks, i.e., the connection
@@ -1376,8 +1377,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
         /* Force connection closed if the connection has indeed been used */
         if(data->mstate > CURLM_STATE_DO) {
-          connclose(data->easy_conn, "Disconnected with pending data");
-          disconnect_conn = TRUE;
+          streamclose(data->easy_conn, "Disconnected with pending data");
+          stream_error = TRUE;
         }
         result = CURLE_OPERATION_TIMEDOUT;
         (void)multi_done(&data->easy_conn, result, TRUE);
@@ -1426,7 +1427,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* Add this handle to the send or pend pipeline */
         result = Curl_add_handle_to_pipeline(data, data->easy_conn);
         if(result)
-          disconnect_conn = TRUE;
+          stream_error = TRUE;
         else {
           if(async)
             /* We're now waiting for an asynchronous name lookup */
@@ -1518,7 +1519,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
       if(result) {
         /* failure detected */
-        disconnect_conn = TRUE;
+        stream_error = TRUE;
         break;
       }
     }
@@ -1558,7 +1559,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
       else if(result) {
         /* failure detected */
         /* Just break, the cleaning up is handled all in one place */
-        disconnect_conn = TRUE;
+        stream_error = TRUE;
         break;
       }
       break;
@@ -1578,7 +1579,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* failure detected */
         Curl_posttransfer(data);
         multi_done(&data->easy_conn, result, TRUE);
-        disconnect_conn = TRUE;
+        stream_error = TRUE;
       }
       break;
 
@@ -1595,7 +1596,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* failure detected */
         Curl_posttransfer(data);
         multi_done(&data->easy_conn, result, TRUE);
-        disconnect_conn = TRUE;
+        stream_error = TRUE;
       }
       break;
 
@@ -1670,7 +1671,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
           if(drc) {
             /* a failure here pretty much implies an out of memory */
             result = drc;
-            disconnect_conn = TRUE;
+            stream_error = TRUE;
           }
           else
             retry = (newurl)?TRUE:FALSE;
@@ -1703,7 +1704,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
           }
           else {
             /* Have error handler disconnect conn if we can't retry */
-            disconnect_conn = TRUE;
+            stream_error = TRUE;
             free(newurl);
           }
         }
@@ -1712,7 +1713,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
           Curl_posttransfer(data);
           if(data->easy_conn)
             multi_done(&data->easy_conn, result, FALSE);
-          disconnect_conn = TRUE;
+          stream_error = TRUE;
         }
       }
       break;
@@ -1734,7 +1735,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* failure detected */
         Curl_posttransfer(data);
         multi_done(&data->easy_conn, result, FALSE);
-        disconnect_conn = TRUE;
+        stream_error = TRUE;
       }
       break;
 
@@ -1763,7 +1764,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* failure detected */
         Curl_posttransfer(data);
         multi_done(&data->easy_conn, result, FALSE);
-        disconnect_conn = TRUE;
+        stream_error = TRUE;
       }
       break;
 
@@ -1885,10 +1886,10 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
 
         if(!(data->easy_conn->handler->flags & PROTOPT_DUAL) &&
            result != CURLE_HTTP2_STREAM)
-          connclose(data->easy_conn, "Transfer returned error");
+          streamclose(data->easy_conn, "Transfer returned error");
 
         Curl_posttransfer(data);
-        multi_done(&data->easy_conn, result, FALSE);
+        multi_done(&data->easy_conn, result, TRUE);
       }
       else if(done) {
         followtype follow=FOLLOW_NONE;
@@ -1944,7 +1945,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
             if(!result)
               newurl = NULL; /* allocation was handed over Curl_follow() */
             else
-              disconnect_conn = TRUE;
+              stream_error = TRUE;
           }
 
           multistate(data, CURLM_STATE_DONE);
@@ -2045,7 +2046,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
           Curl_removeHandleFromPipeline(data, data->easy_conn->send_pipe);
           Curl_removeHandleFromPipeline(data, data->easy_conn->recv_pipe);
 
-          if(disconnect_conn) {
+          if(stream_error) {
             /* Don't attempt to send data over a connection that timed out */
             bool dead_connection = result == CURLE_OPERATION_TIMEDOUT;
             /* disconnect properly */
@@ -2069,7 +2070,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
         /* aborted due to progress callback return code must close the
            connection */
         result = CURLE_ABORTED_BY_CALLBACK;
-        connclose(data->easy_conn, "Aborted by callback");
+        streamclose(data->easy_conn, "Aborted by callback");
 
         /* if not yet in DONE state, go there, otherwise COMPLETED */
         multistate(data, (data->mstate < CURLM_STATE_DONE)?
diff --git a/lib/url.c b/lib/url.c
index 29c32cfba..f355c7a22 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -2830,6 +2830,17 @@ CURLcode Curl_disconnect(struct connectdata *conn, bool dead_connection)
     return CURLE_OK;
   }
 
+  /*
+   * If this connection isn't marked to force-close, leave it open if there
+   * are other users of it
+   */
+  if(!conn->bits.close &&
+     (conn->send_pipe->size + conn->recv_pipe->size)) {
+    DEBUGF(infof(data, "Curl_disconnect, usecounter: %d\n",
+                 conn->send_pipe->size + conn->recv_pipe->size));
+    return CURLE_OK;
+  }
+
   if(conn->dns_entry != NULL) {
     Curl_resolv_unlock(data, conn->dns_entry);
     conn->dns_entry = NULL;
diff --git a/lib/urldata.h b/lib/urldata.h
index 44f8dc5c0..1f4d2551e 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -818,6 +818,7 @@ struct Curl_handler {
 #define PROTOPT_CREDSPERREQUEST (1<<7) /* requires login credentials per
                                           request instead of per connection */
 #define PROTOPT_ALPN_NPN (1<<8) /* set ALPN and/or NPN for this */
+#define PROTOPT_STREAM (1<<9) /* a protocol with individual logical streams */
 
 /* return the count of bytes sent, or -1 on error */
 typedef ssize_t (Curl_send)(struct connectdata *conn, /* connection data */