http2: upload improvements

Make send buffer smaller to have progress and "upload done" reporting
closer to reality. Fix handling of send "drain" condition to no longer
trigger once the transfer loop reports it is done sending. Also do not
trigger the send "drain" on RST streams.

Background:
- a upload stall was reported in #11157 that timed out
- test_07_33a reproduces a problem with such a stall if the
  server 404s the request and RSTs the stream.
- test_07_33b verifies a successful PUT, using the parameters
  from #11157 and checks success

Ref: #11157
Closes #11165

diff --git a/lib/http2.c b/lib/http2.c
index 0ebe630abee047ff578a5ff6cbd2078ee283986e..2dc032f233c3f89e709b28b3cce2f17476123e70 100644 (file)
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -75,7 +75,9 @@
 #define H2_NW_SEND_CHUNKS       1
 /* stream recv/send chunks are a result of window / chunk sizes */
 #define H2_STREAM_RECV_CHUNKS   (H2_STREAM_WINDOW_SIZE / H2_CHUNK_SIZE)
-#define H2_STREAM_SEND_CHUNKS   (H2_STREAM_WINDOW_SIZE / H2_CHUNK_SIZE)
+/* keep smaller stream upload buffer (default h2 window size) to have
+ * our progress bars and "upload done" reporting closer to reality */
+#define H2_STREAM_SEND_CHUNKS   ((64 * 1024) / H2_CHUNK_SIZE)
 /* spare chunks we keep for a full window */
 #define H2_STREAM_POOL_SPARES   (H2_STREAM_WINDOW_SIZE / H2_CHUNK_SIZE)
 
@@ -185,6 +187,8 @@ struct stream_ctx {
   bool reset;  /* TRUE on stream reset */
   bool close_handled; /* TRUE if stream closure is handled by libcurl */
   bool bodystarted;
+  bool send_closed; /* transfer is done sending, we might have still
+                        buffered data in stream->sendbuf to upload. */
 };
 
 #define H2_STREAM_CTX(d)    ((struct stream_ctx *)(((d) && (d)->req.p.http)? \
@@ -205,7 +209,7 @@ static void drain_stream(struct Curl_cfilter *cf,
 
   (void)cf;
   bits = CURL_CSELECT_IN;
-  if(stream->upload_left)
+  if(!stream->send_closed && stream->upload_left)
     bits |= CURL_CSELECT_OUT;
   if(data->state.dselect_bits != bits) {
     data->state.dselect_bits = bits;
@@ -1039,6 +1043,8 @@ static CURLcode on_stream_frame(struct Curl_cfilter *cf,
     DEBUGF(LOG_CF(data, cf, "[h2sid=%d] FRAME[RST]", stream_id));
     stream->closed = TRUE;
     stream->reset = TRUE;
+    stream->send_closed = TRUE;
+    data->req.keepon &= ~KEEP_SEND_HOLD;
     drain_stream(cf, data, stream);
     break;
   case NGHTTP2_WINDOW_UPDATE:
@@ -1438,7 +1444,7 @@ static ssize_t req_body_read_callback(nghttp2_session *session,
     nread = 0;
   }
 
-  if(nread > 0 && data_s->state.infilesize != -1)
+  if(nread > 0 && stream->upload_left != -1)
     stream->upload_left -= nread;
 
   DEBUGF(LOG_CF(data_s, cf, "[h2sid=%d] req_body_read(len=%zu) left=%zd"
@@ -1517,15 +1523,18 @@ static CURLcode http2_data_done_send(struct Curl_cfilter *cf,
     goto out;
 
   DEBUGF(LOG_CF(data, cf, "[h2sid=%d] data done send", stream->id));
-  if(stream->upload_left) {
-    /* If the stream still thinks there's data left to upload. */
-    if(stream->upload_left == -1)
-      stream->upload_left = 0; /* DONE! */
-
-    /* resume sending here to trigger the callback to get called again so
-       that it can signal EOF to nghttp2 */
-    (void)nghttp2_session_resume_data(ctx->h2, stream->id);
-    drain_stream(cf, data, stream);
+  if(!stream->send_closed) {
+    stream->send_closed = TRUE;
+    if(stream->upload_left) {
+      /* If we operated with unknown length, we now know that everything
+       * that is buffered is all we have to send. */
+      if(stream->upload_left == -1)
+        stream->upload_left = Curl_bufq_len(&stream->sendbuf);
+      /* resume sending here to trigger the callback to get called again so
+         that it can signal EOF to nghttp2 */
+      (void)nghttp2_session_resume_data(ctx->h2, stream->id);
+      drain_stream(cf, data, stream);
+    }
   }
 
 out:

diff --git a/tests/http/test_07_upload.py b/tests/http/test_07_upload.py
index 6b8f3dbc4d996e97c921cd968477974e28fa0285..37fa45cb9e404a789f47bc92a4dd6937e93c5fec 100644 (file)
--- a/tests/http/test_07_upload.py
+++ b/tests/http/test_07_upload.py
@@ -240,7 +240,50 @@ class TestUpload:
         url = f'https://{env.authority_for(env.domain1, proto)}/curltest/put?id=[0-{count-1}]'
         r = curl.http_put(urls=[url], fdata=fdata, alpn_proto=proto)
         r.check_response(count=count, http_status=200)
-        r.check_response(count=count, http_status=200)
+
+    # issue #11157, upload that is 404'ed by server, needs to terminate
+    # correctly and not time out on sending
+    def test_07_33_issue_11157a(self, env: Env, httpd, nghttpx, repeat):
+        proto = 'h2'
+        fdata = os.path.join(env.gen_dir, 'data-10m')
+        # send a POST to our PUT handler which will send immediately a 404 back
+        url = f'https://{env.authority_for(env.domain1, proto)}/curltest/put'
+        curl = CurlClient(env=env)
+        r = curl.run_direct(with_stats=True, args=[
+            '--resolve', f'{env.authority_for(env.domain1, proto)}:127.0.0.1',
+            '--cacert', env.ca.cert_file,
+            '--request', 'POST',
+            '--max-time', '5', '-v',
+            '--url', url,
+            '--form', 'idList=12345678',
+            '--form', 'pos=top',
+            '--form', 'name=mr_test',
+            '--form', f'fileSource=@{fdata};type=application/pdf',
+        ])
+        assert r.exit_code == 0, f'{r}'
+        r.check_stats(1, 404)
+
+    # issue #11157, send upload that is slowly read in
+    def test_07_33_issue_11157b(self, env: Env, httpd, nghttpx, repeat):
+        proto = 'h2'
+        fdata = os.path.join(env.gen_dir, 'data-10m')
+        # tell our test PUT handler to read the upload more slowly, so
+        # that the send buffering and transfer loop needs to wait
+        url = f'https://{env.authority_for(env.domain1, proto)}/curltest/put?chunk_delay=2ms'
+        curl = CurlClient(env=env)
+        r = curl.run_direct(with_stats=True, args=[
+            '--resolve', f'{env.authority_for(env.domain1, proto)}:127.0.0.1',
+            '--cacert', env.ca.cert_file,
+            '--request', 'PUT',
+            '--max-time', '5', '-v',
+            '--url', url,
+            '--form', 'idList=12345678',
+            '--form', 'pos=top',
+            '--form', 'name=mr_test',
+            '--form', f'fileSource=@{fdata};type=application/pdf',
+        ])
+        assert r.exit_code == 0, f'{r}'
+        r.check_stats(1, 200)
 
     def check_download(self, count, srcfile, curl):
         for i in range(count):