From: Stefan Eissing Date: Sat, 20 May 2023 10:26:04 +0000 (+0200) Subject: http2: upload improvements X-Git-Tag: curl-8_1_1~16 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0cab1359a1ab9720afccbec6f84e22b409dc9886;p=thirdparty%2Fcurl.git 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 0ebe630abe..2dc032f233 100644 --- 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 6b8f3dbc4d..37fa45cb9e 100644 --- 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):