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
curl · Aug 28, 2016 · 3533def · 3533def · captain-caveman2k · Aug 30, 2016
1 parent a6ddd65
commit 3533def
Show file tree

Hide file tree

Showing 11 changed files with 152 additions and 88 deletions.
diff --git a/docs/KNOWN_BUGS b/docs/KNOWN_BUGS
@@ -63,6 +63,7 @@ problems may have been fixed or changed somewhat since this was written!
  7.5 ASCII FTP
  7.6 FTP with NULs in URL parts
  7.7 FTP and empty path parts in the URL
+ 7.8 Premature transfer end but healthy control channel
 
  8. TELNET
  8.1 TELNET and time limtiations don't work
@@ -441,6 +442,17 @@ problems may have been fixed or changed somewhat since this was written!
  the user wants to reach the root dir (this exception SHALL remain even when
  this bug is fixed).
 
+7.8 Premature transfer end but healthy control channel
+
+ When 'multi_done' is called before the transfer has been completed the normal
+ way, it is considered a "premature" transfer end. In this situation, libcurl
+ closes the connection assuming it doesn't know the state of the connection so
+ it can't be reused for subsequent requests.
+
+ With FTP however, this isn't necessarily true but there are a bunch of
+ situations (listed in the ftp_done code) where it *could* keep the connection
+ alive even in this situation - but the current code doesn't. Fixing this would
+ allow libcurl to reuse FTP connections better.
 
 8. TELNET
 

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
     }