Skip to content

Commit

Permalink
http2: upload improvements
Browse files Browse the repository at this point in the history
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 curl#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 curl#11157 and checks success

Ref: curl#11157
Closes curl#11165
  • Loading branch information
icing authored and bch committed Jul 19, 2023
1 parent 8519fde commit adbe490
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 13 deletions.
33 changes: 21 additions & 12 deletions lib/http2.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)? \
Expand All @@ -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;
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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:
Expand Down
45 changes: 44 additions & 1 deletion tests/http/test_07_upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,50 @@ def test_07_32_issue_10591(self, env: Env, httpd, nghttpx, repeat, proto):
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):
Expand Down

0 comments on commit adbe490

Please sign in to comment.