http2: Ensure that http2_handle_stream_close is called

This commit ensures that streams which was closed in on_stream_close
callback gets passed to http2_handle_stream_close.  Previously, this
might not happen.  To achieve this, we increment drain property to
forcibly call recv function for that stream.

To more accurately check that we have no pending event before shutting
down HTTP/2 session, we sum up drain property into
http_conn.drain_total.  We only shutdown session if that value is 0.

With this commit, when stream was closed before reading response
header fields, error code CURLE_HTTP2_STREAM is returned even if
HTTP/2 level error is NO_ERROR.  This signals the upper layer that
stream was closed by error just like TCP connection close in HTTP/1.

Ref: https://github.com/curl/curl/issues/659
Ref: https://github.com/curl/curl/pull/663

2 files changed, 39 insertions, 19 deletions

diff --git a/lib/http.h b/lib/http.h
index 5000df448..8aae35206 100644
--- a/lib/http.h
+++ b/lib/http.h
@@ -214,6 +214,7 @@ struct http_conn {
      them for both cases. */
   int32_t pause_stream_id; /* stream ID which paused
                               nghttp2_session_mem_recv */
+  int drain_total; /* sum of all stream's UrlState.drain */
 
   /* this is a hash of all individual streams (SessionHandle structs) */
   struct h2settings settings;
diff --git a/lib/http2.c b/lib/http2.c
index 206d9592d..5e9adf240 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -507,6 +507,7 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame,
     stream->memlen += ncopy;
 
     data_s->state.drain++;
+    httpc->drain_total++;
     {
       /* get the pointer from userp again since it was re-assigned above */
       struct connectdata *conn_s = (struct connectdata *)userp;
@@ -583,6 +584,7 @@ static int on_data_chunk_recv(nghttp2_session *session, uint8_t flags,
   stream->memlen += nread;
 
   data_s->state.drain++;
+  conn->proto.httpc.drain_total++;
 
   /* if we receive data for another handle, wake that up */
   if(conn->data != data_s)
@@ -665,9 +667,9 @@ static int on_stream_close(nghttp2_session *session, int32_t stream_id,
 {
   struct SessionHandle *data_s;
   struct HTTP *stream;
+  struct connectdata *conn = (struct connectdata *)userp;
   (void)session;
   (void)stream_id;
-  (void)userp;
 
   if(stream_id) {
     /* get the stream from the hash based on Stream ID, stream ID zero is for
@@ -686,6 +688,8 @@ static int on_stream_close(nghttp2_session *session, int32_t stream_id,
 
     stream->error_code = error_code;
     stream->closed = TRUE;
+    data_s->state.drain++;
+    conn->proto.httpc.drain_total++;
 
     /* remove the entry from the hash as the stream is now gone */
     nghttp2_session_set_stream_user_data(session, stream_id, 0);
@@ -845,6 +849,7 @@ static int on_header(nghttp2_session *session, const nghttp2_frame *frame,
     /* the space character after the status code is mandatory */
     Curl_add_buffer(stream->header_recvbuf, " \r\n", 3);
     data_s->state.drain++;
+    conn->proto.httpc.drain_total++;
     /* if we receive data for another handle, wake that up */
     if(conn->data != data_s)
       Curl_expire(data_s, 1);
@@ -862,6 +867,7 @@ static int on_header(nghttp2_session *session, const nghttp2_frame *frame,
   Curl_add_buffer(stream->header_recvbuf, value, valuelen);
   Curl_add_buffer(stream->header_recvbuf, "\r\n", 2);
   data_s->state.drain++;
+  conn->proto.httpc.drain_total++;
   /* if we receive data for another handle, wake that up */
   if(conn->data != data_s)
     Curl_expire(data_s, 1);
@@ -1053,6 +1059,14 @@ CURLcode Curl_http2_request_upgrade(Curl_send_buffer *req,
   return result;
 }
 
+/*
+ * Returns nonzero if current HTTP/2 session should be closed.
+ */
+static int should_close_session(struct http_conn *httpc) {
+  return httpc->drain_total == 0 && !nghttp2_session_want_read(httpc->h2) &&
+         !nghttp2_session_want_write(httpc->h2);
+}
+
 static int h2_session_send(struct SessionHandle *data,
                            nghttp2_session *h2);
 
@@ -1102,9 +1116,7 @@ static int h2_process_pending_input(struct SessionHandle *data,
     return -1;
   }
 
-  if(httpc->pause_stream_id == 0 &&
-     !nghttp2_session_want_read(httpc->h2) &&
-     !nghttp2_session_want_write(httpc->h2)) {
+  if(should_close_session(httpc)) {
     DEBUGF(infof(data,
                  "h2_process_pending_input: nothing to do in this session\n"));
     *err = CURLE_HTTP2;
@@ -1125,12 +1137,17 @@ static ssize_t http2_handle_stream_close(struct connectdata *conn,
     httpc->pause_stream_id = 0;
   }
 
+  httpc->drain_total -= data->state.drain;
+  data->state.drain = 0;
+
   if(httpc->pause_stream_id == 0) {
     if(h2_process_pending_input(data, httpc, err) != 0) {
       return -1;
     }
   }
 
+  DEBUGASSERT(data->state.drain == 0);
+
   /* Reset to FALSE to prevent infinite loop in readwrite_data
    function. */
   stream->closed = FALSE;
@@ -1141,6 +1158,14 @@ static ssize_t http2_handle_stream_close(struct connectdata *conn,
     return -1;
   }
 
+  if(!stream->bodystarted) {
+    failf(data, "HTTP/2 stream %u was closed cleanly, but before getting "
+          " all response header fields, teated as error",
+          stream->stream_id);
+    *err = CURLE_HTTP2_STREAM;
+    return -1;
+  }
+
   if(stream->trailer_recvbuf && stream->trailer_recvbuf->buffer) {
     trailer_pos = stream->trailer_recvbuf->buffer;
     trailer_end = trailer_pos + stream->trailer_recvbuf->size_used;
@@ -1228,9 +1253,7 @@ static ssize_t http2_recv(struct connectdata *conn, int sockindex,
 
   (void)sockindex; /* we always do HTTP2 on sockindex 0 */
 
-  if(httpc->pause_stream_id == 0 &&
-     !nghttp2_session_want_read(httpc->h2) &&
-     !nghttp2_session_want_write(httpc->h2)) {
+  if(should_close_session(httpc)) {
     DEBUGF(infof(data,
                  "http2_recv: nothing to do in this session\n"));
     *err = CURLE_HTTP2;
@@ -1399,9 +1422,7 @@ static ssize_t http2_recv(struct connectdata *conn, int sockindex,
       return 0;
     }
 
-    if(httpc->pause_stream_id == 0 &&
-       !nghttp2_session_want_read(httpc->h2) &&
-       !nghttp2_session_want_write(httpc->h2)) {
+    if(should_close_session(httpc)) {
       DEBUGF(infof(data, "http2_recv: nothing to do in this session\n"));
       *err = CURLE_HTTP2;
       return -1;
@@ -1419,8 +1440,10 @@ static ssize_t http2_recv(struct connectdata *conn, int sockindex,
       DEBUGF(infof(data, "Data returned for PAUSED stream %u\n",
                    stream->stream_id));
     }
-    else
+    else if(!stream->closed) {
+      httpc->drain_total -= data->state.drain;
       data->state.drain = 0; /* this stream is hereby drained */
+    }
 
     return retlen;
   }
@@ -1484,9 +1507,7 @@ static ssize_t http2_send(struct connectdata *conn, int sockindex,
     stream->upload_mem = NULL;
     stream->upload_len = 0;
 
-    if(httpc->pause_stream_id == 0 &&
-       !nghttp2_session_want_read(httpc->h2) &&
-       !nghttp2_session_want_write(httpc->h2)) {
+    if(should_close_session(httpc)) {
       DEBUGF(infof(conn->data, "http2_send: nothing to do in this session\n"));
       *err = CURLE_HTTP2;
       return -1;
@@ -1658,9 +1679,7 @@ static ssize_t http2_send(struct connectdata *conn, int sockindex,
     return -1;
   }
 
-  if(httpc->pause_stream_id == 0 &&
-     !nghttp2_session_want_read(httpc->h2) &&
-     !nghttp2_session_want_write(httpc->h2)) {
+  if(should_close_session(httpc)) {
     DEBUGF(infof(conn->data, "http2_send: nothing to do in this session\n"));
     *err = CURLE_HTTP2;
     return -1;
@@ -1719,6 +1738,7 @@ CURLcode Curl_http2_setup(struct connectdata *conn)
   httpc->nread_inbuf = 0;
 
   httpc->pause_stream_id = 0;
+  httpc->drain_total = 0;
 
   conn->bits.multiplex = TRUE; /* at least potentially multiplexed */
   conn->httpversion = 20;
@@ -1826,8 +1846,7 @@ CURLcode Curl_http2_switched(struct connectdata *conn,
     return CURLE_HTTP2;
   }
 
-  if(!nghttp2_session_want_read(httpc->h2) &&
-     !nghttp2_session_want_write(httpc->h2)) {
+  if(should_close_session(httpc)) {
     DEBUGF(infof(data,
                  "nghttp2_session_send(): nothing to do in this session\n"));
     return CURLE_HTTP2;