HTTP/2: curl hangs with upload if server sends response early and resets stream #986

Closed
tatsuhiro-t opened this Issue Aug 28, 2016 · 4 comments

Projects

None yet

3 participants

@tatsuhiro-t
Contributor
tatsuhiro-t commented Aug 28, 2016 edited

I did this

Run h2 server. With --early-response option, this server returns HTTP response without waiting for upload completion. Additionally, it sends RST_STREAM if it expects request body from client:

$ nghttpd 8443 --no-tls -v --early-response

Then do some large upload from curl:

$ curl http://localhost:8443 --http2-prior-knowledge -T 100m  -v
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8443 (#0)
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0xf91f30)
> PUT /100m HTTP/1.1
> Host: localhost:8443
> User-Agent: curl/7.50.2-DEV
> Accept: */*
> Content-Length: 104857600
> 
* Connection state changed (MAX_CONCURRENT_STREAMS updated)!
< HTTP/2 200 
< server: nghttpd nghttp2/1.15.0-DEV
< cache-control: max-age=3600
< date: Sun, 28 Aug 2016 08:49:30 GMT
< content-length: 0
< last-modified: Sun, 28 Aug 2016 08:32:37 GMT
< 

And it stuck.

Background info: nghttp2/nghttp2#669

I expected the following

curl should detect stream closure, and exit. Since upload was not completed, with some error code (not sure which one we should use here).

curl/libcurl version

I build curl from git master

curl 7.50.2-DEV (x86_64-pc-linux-gnu) libcurl/7.50.2-DEV OpenSSL/1.0.2h zlib/1.2.8 libidn/1.32 libssh2/1.5.0 nghttp2/1.8.0-DEV
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets 

operating system

Linux

@tatsuhiro-t
Contributor

I attempted to fix this. The following patch seems to work, but I have not checked side effects.

diff --git a/lib/http.h b/lib/http.h
index 6529005..9fb669c 100644
--- a/lib/http.h
+++ b/lib/http.h
@@ -168,6 +168,7 @@ struct HTTP {
   const uint8_t *pausedata; /* pointer to data received in on_data_chunk */
   size_t pauselen; /* the number of bytes left in data */
   bool closed; /* TRUE on HTTP2 stream close */
+  bool close_handled; /* TRUE if stream closure is handled by libcurl */
   uint32_t error_code; /* HTTP/2 error code */

   char *mem;     /* points to a buffer in memory to store received data */
diff --git a/lib/http2.c b/lib/http2.c
index a14f75e..e611058 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -151,6 +151,7 @@ void Curl_http2_setup_req(struct Curl_easy *data)
   http->pauselen = 0;
   http->error_code = NGHTTP2_NO_ERROR;
   http->closed = FALSE;
+  http->close_handled = FALSE;
   http->mem = data->state.buffer;
   http->len = BUFSIZE;
   http->memlen = 0;
@@ -1216,6 +1217,8 @@ static ssize_t http2_handle_stream_close(struct connectdata *conn,
     }
   }

+  stream->close_handled = TRUE;
+
   DEBUGF(infof(data, "http2_recv returns 0, http2_handle_stream_close\n"));
   return 0;
 }
@@ -1520,6 +1523,14 @@ static ssize_t http2_send(struct connectdata *conn, int sockindex,
   DEBUGF(infof(conn->data, "http2_send len=%zu\n", len));

   if(stream->stream_id != -1) {
+    if(stream->close_handled) {
+      infof(conn->data, "stream %d closed\n", stream->stream_id);
+      *err = CURLE_HTTP2;
+      return -1;
+    }
+    else if(stream->closed) {
+      return http2_handle_stream_close(conn, conn->data, stream, err);
+    }
     /* If stream_id != -1, we have dispatched request HEADERS, and now
        are going to send or sending request body in DATA frame */
     stream->upload_mem = mem;

Here, we use error code CURLE_HTTP2. Not sure it is the best code in this situation.

@bagder bagder added the HTTP/2 label Aug 28, 2016
@bagder
Member
bagder commented Aug 28, 2016

Thanks, I'll give this a closer look and test out a little before I merge. Regarding the error code, I think it is time for us to go through and check if we shouldn't add a few more error codes for h2 specific situations that would be helpful if we exposed better to users instead of shoveling everything under a generic "http2 error" code.

@bagder bagder closed this in c3e906e Aug 28, 2016
@jay
Member
jay commented Aug 28, 2016

There is a new error code we added CURLE_HTTP2_STREAM a while ago, but I know you are aware of this, does it not apply?

@bagder
Member
bagder commented Aug 28, 2016

Oh right. forgot about that one. Yes it seems suitable for this case. Thanks, will fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment