From: Alex Snast Date: Wed, 17 Jul 2024 11:06:06 +0000 (+0300) Subject: http/3: resume upload on ack if we have more data to send X-Git-Tag: curl-8_9_0~28 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f504db89282d81ffc387e2e5e4449fb97b4665d7;p=thirdparty%2Fcurl.git http/3: resume upload on ack if we have more data to send Currently we're waiting for sendbuf_len_in_flight to hit zero before resuming upload which means we're blocking and waiting for _all_ acks to arrive before sending more data. This causes significant delays especially when ack delay is used on the server side. The fix addresses several issues in h3 over ngtcp2: - On ack we now call nghttp3_conn_resume_stream() when we have more data to send. - upload_left was incorrectly computed on CF_CTRL_DATA_DONE_SEND as we need to subtract the ammount of data we have in flight. - Remove upload_blocked_len as we Curl_bufq_write call will do the right thing when called from cf_ngtcp2_send. Fixes #14198 Closes #14209 --- diff --git a/lib/vquic/curl_ngtcp2.c b/lib/vquic/curl_ngtcp2.c index d8a55ee048..1f67f2362e 100644 --- a/lib/vquic/curl_ngtcp2.c +++ b/lib/vquic/curl_ngtcp2.c @@ -162,7 +162,6 @@ struct h3_stream_ctx { struct bufq sendbuf; /* h3 request body */ struct h1_req_parser h1; /* h1 request parsing */ size_t sendbuf_len_in_flight; /* sendbuf amount "in flight" */ - size_t upload_blocked_len; /* the amount written last and EGAINed */ curl_uint64_t error3; /* HTTP/3 stream error code */ curl_off_t upload_left; /* number of request bytes left to upload */ int status_code; /* HTTP status code */ @@ -1272,8 +1271,8 @@ static int cb_h3_acked_req_body(nghttp3_conn *conn, int64_t stream_id, Curl_bufq_skip(&stream->sendbuf, skiplen); stream->sendbuf_len_in_flight -= skiplen; - /* Everything ACKed, we resume upload processing */ - if(!stream->sendbuf_len_in_flight) { + /* Resume upload processing if we have more data to send */ + if(stream->sendbuf_len_in_flight < Curl_bufq_len(&stream->sendbuf)) { int rv = nghttp3_conn_resume_stream(conn, stream_id); if(rv && rv != NGHTTP3_ERR_STREAM_NOT_FOUND) { return NGTCP2_ERR_CALLBACK_FAILURE; @@ -1528,21 +1527,6 @@ static ssize_t cf_ngtcp2_send(struct Curl_cfilter *cf, struct Curl_easy *data, sent = -1; goto out; } - else if(stream->upload_blocked_len) { - /* the data in `buf` has already been submitted or added to the - * buffers, but have been EAGAINed on the last invocation. */ - DEBUGASSERT(len >= stream->upload_blocked_len); - if(len < stream->upload_blocked_len) { - /* Did we get called again with a smaller `len`? This should not - * happen. We are not prepared to handle that. */ - failf(data, "HTTP/3 send again with decreased length"); - *err = CURLE_HTTP3; - sent = -1; - goto out; - } - sent = (ssize_t)stream->upload_blocked_len; - stream->upload_blocked_len = 0; - } else if(stream->closed) { if(stream->resp_hds_complete) { /* Server decided to close the stream after having sent us a final @@ -1586,18 +1570,6 @@ static ssize_t cf_ngtcp2_send(struct Curl_cfilter *cf, struct Curl_easy *data, sent = -1; } - if(stream && sent > 0 && stream->sendbuf_len_in_flight) { - /* We have unacknowledged DATA and cannot report success to our - * caller. Instead we EAGAIN and remember how much we have already - * "written" into our various internal connection buffers. */ - stream->upload_blocked_len = sent; - CURL_TRC_CF(data, cf, "[%" CURL_PRId64 "] cf_send(len=%zu), " - "%zu bytes in flight -> EGAIN", stream->id, len, - stream->sendbuf_len_in_flight); - *err = CURLE_AGAIN; - sent = -1; - } - out: result = check_and_set_expiry(cf, data, &pktx); if(result) { @@ -1965,7 +1937,8 @@ static CURLcode cf_ngtcp2_data_event(struct Curl_cfilter *cf, struct h3_stream_ctx *stream = H3_STREAM_CTX(ctx, data); if(stream && !stream->send_closed) { stream->send_closed = TRUE; - stream->upload_left = Curl_bufq_len(&stream->sendbuf); + stream->upload_left = Curl_bufq_len(&stream->sendbuf) - + stream->sendbuf_len_in_flight; (void)nghttp3_conn_resume_stream(ctx->h3conn, stream->id); } break; diff --git a/lib/vquic/curl_osslq.c b/lib/vquic/curl_osslq.c index 8a6f6e45a7..dafde44f26 100644 --- a/lib/vquic/curl_osslq.c +++ b/lib/vquic/curl_osslq.c @@ -560,7 +560,6 @@ struct h3_stream_ctx { struct bufq recvbuf; /* h3 response body */ struct h1_req_parser h1; /* h1 request parsing */ size_t sendbuf_len_in_flight; /* sendbuf amount "in flight" */ - size_t upload_blocked_len; /* the amount written last and EGAINed */ size_t recv_buf_nonflow; /* buffered bytes, not counting for flow control */ curl_uint64_t error3; /* HTTP/3 stream error code */ curl_off_t upload_left; /* number of request bytes left to upload */ @@ -1053,8 +1052,8 @@ static int cb_h3_acked_stream_data(nghttp3_conn *conn, int64_t stream_id, Curl_bufq_skip(&stream->sendbuf, skiplen); stream->sendbuf_len_in_flight -= skiplen; - /* Everything ACKed, we resume upload processing */ - if(!stream->sendbuf_len_in_flight) { + /* Resume upload processing if we have more data to send */ + if(stream->sendbuf_len_in_flight < Curl_bufq_len(&stream->sendbuf)) { int rv = nghttp3_conn_resume_stream(conn, stream_id); if(rv && rv != NGHTTP3_ERR_STREAM_NOT_FOUND) { return NGHTTP3_ERR_CALLBACK_FAILURE; @@ -1936,21 +1935,6 @@ static ssize_t cf_osslq_send(struct Curl_cfilter *cf, struct Curl_easy *data, } stream = H3_STREAM_CTX(ctx, data); } - else if(stream->upload_blocked_len) { - /* the data in `buf` has already been submitted or added to the - * buffers, but have been EAGAINed on the last invocation. */ - DEBUGASSERT(len >= stream->upload_blocked_len); - if(len < stream->upload_blocked_len) { - /* Did we get called again with a smaller `len`? This should not - * happen. We are not prepared to handle that. */ - failf(data, "HTTP/3 send again with decreased length"); - *err = CURLE_HTTP3; - nwritten = -1; - goto out; - } - nwritten = (ssize_t)stream->upload_blocked_len; - stream->upload_blocked_len = 0; - } else if(stream->closed) { if(stream->resp_hds_complete) { /* Server decided to close the stream after having sent us a final @@ -1988,18 +1972,6 @@ static ssize_t cf_osslq_send(struct Curl_cfilter *cf, struct Curl_easy *data, nwritten = -1; } - if(stream && nwritten > 0 && stream->sendbuf_len_in_flight) { - /* We have unacknowledged DATA and cannot report success to our - * caller. Instead we EAGAIN and remember how much we have already - * "written" into our various internal connection buffers. */ - stream->upload_blocked_len = nwritten; - CURL_TRC_CF(data, cf, "[%" CURL_PRId64 "] cf_send(len=%zu), " - "%zu bytes in flight -> EGAIN", stream->s.id, len, - stream->sendbuf_len_in_flight); - *err = CURLE_AGAIN; - nwritten = -1; - } - out: result = check_and_set_expiry(cf, data); CURL_TRC_CF(data, cf, "[%" CURL_PRId64 "] cf_send(len=%zu) -> %zd, %d", @@ -2159,7 +2131,8 @@ static CURLcode cf_osslq_data_event(struct Curl_cfilter *cf, struct h3_stream_ctx *stream = H3_STREAM_CTX(ctx, data); if(stream && !stream->send_closed) { stream->send_closed = TRUE; - stream->upload_left = Curl_bufq_len(&stream->sendbuf); + stream->upload_left = Curl_bufq_len(&stream->sendbuf) - + stream->sendbuf_len_in_flight; (void)nghttp3_conn_resume_stream(ctx->h3.conn, stream->s.id); } break;