From 2ec585518e83bec7de8deb0d429cd744d99fc72b Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 19 Feb 2020 00:16:09 +0900 Subject: [PATCH] Fix receiving stream data stall Previously, if automatic window update is enabled (which is default), after window size is set to 0 by nghttp2_session_set_local_window_size, once the receiving window is exhausted, even after window size is increased by nghttp2_session_set_local_window_size, no more data cannot be received. This is because nghttp2_session_set_local_window_size does not submit WINDOW_UPDATE. It is only triggered when new data arrives but since window is filled up, no more data cannot be received, thus dead lock happens. This commit fixes this issue. nghttp2_session_set_local_window_size submits WINDOW_UPDATE if necessary. https://github.com/curl/curl/issues/4939 --- lib/nghttp2_session.c | 61 ++++++++++-------------------------- lib/nghttp2_session.h | 32 +++++++++++++++++++ lib/nghttp2_submit.c | 18 ++++++++--- tests/nghttp2_session_test.c | 56 +++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 49 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 9df3d6f3..563ccd7d 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -2494,14 +2494,6 @@ static int session_update_stream_consumed_size(nghttp2_session *session, static int session_update_connection_consumed_size(nghttp2_session *session, size_t delta_size); -static int session_update_recv_connection_window_size(nghttp2_session *session, - size_t delta_size); - -static int session_update_recv_stream_window_size(nghttp2_session *session, - nghttp2_stream *stream, - size_t delta_size, - int send_window_update); - /* * Called after a frame is sent. This function runs * on_frame_send_callback and handles stream closure upon END_STREAM @@ -2735,7 +2727,7 @@ static int session_after_frame_sent1(nghttp2_session *session) { if (session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE) { rv = session_update_connection_consumed_size(session, 0); } else { - rv = session_update_recv_connection_window_size(session, 0); + rv = nghttp2_session_update_recv_connection_window_size(session, 0); } if (nghttp2_is_fatal(rv)) { @@ -2761,7 +2753,8 @@ static int session_after_frame_sent1(nghttp2_session *session) { if (session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE) { rv = session_update_stream_consumed_size(session, stream, 0); } else { - rv = session_update_recv_stream_window_size(session, stream, 0, 1); + rv = + nghttp2_session_update_recv_stream_window_size(session, stream, 0, 1); } if (nghttp2_is_fatal(rv)) { @@ -5019,22 +5012,10 @@ static int adjust_recv_window_size(int32_t *recv_window_size_ptr, size_t delta, return 0; } -/* - * Accumulates received bytes |delta_size| for stream-level flow - * control and decides whether to send WINDOW_UPDATE to that stream. - * If NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE is set, WINDOW_UPDATE will not - * be sent. - * - * This function returns 0 if it succeeds, or one of the following - * negative error codes: - * - * NGHTTP2_ERR_NOMEM - * Out of memory. - */ -static int session_update_recv_stream_window_size(nghttp2_session *session, - nghttp2_stream *stream, - size_t delta_size, - int send_window_update) { +int nghttp2_session_update_recv_stream_window_size(nghttp2_session *session, + nghttp2_stream *stream, + size_t delta_size, + int send_window_update) { int rv; rv = adjust_recv_window_size(&stream->recv_window_size, delta_size, stream->local_window_size); @@ -5063,20 +5044,8 @@ static int session_update_recv_stream_window_size(nghttp2_session *session, return 0; } -/* - * Accumulates received bytes |delta_size| for connection-level flow - * control and decides whether to send WINDOW_UPDATE to the - * connection. If NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE is set, - * WINDOW_UPDATE will not be sent. - * - * This function returns 0 if it succeeds, or one of the following - * negative error codes: - * - * NGHTTP2_ERR_NOMEM - * Out of memory. - */ -static int session_update_recv_connection_window_size(nghttp2_session *session, - size_t delta_size) { +int nghttp2_session_update_recv_connection_window_size(nghttp2_session *session, + size_t delta_size) { int rv; rv = adjust_recv_window_size(&session->recv_window_size, delta_size, session->local_window_size); @@ -6454,7 +6423,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, } /* Pad Length field is subject to flow control */ - rv = session_update_recv_connection_window_size(session, readlen); + rv = nghttp2_session_update_recv_connection_window_size(session, readlen); if (nghttp2_is_fatal(rv)) { return rv; } @@ -6477,7 +6446,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, stream = nghttp2_session_get_stream(session, iframe->frame.hd.stream_id); if (stream) { - rv = session_update_recv_stream_window_size( + rv = nghttp2_session_update_recv_stream_window_size( session, stream, readlen, iframe->payloadleft || (iframe->frame.hd.flags & NGHTTP2_FLAG_END_STREAM) == 0); @@ -6524,7 +6493,8 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (readlen > 0) { ssize_t data_readlen; - rv = session_update_recv_connection_window_size(session, readlen); + rv = nghttp2_session_update_recv_connection_window_size(session, + readlen); if (nghttp2_is_fatal(rv)) { return rv; } @@ -6533,7 +6503,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return (ssize_t)inlen; } - rv = session_update_recv_stream_window_size( + rv = nghttp2_session_update_recv_stream_window_size( session, stream, readlen, iframe->payloadleft || (iframe->frame.hd.flags & NGHTTP2_FLAG_END_STREAM) == 0); @@ -6634,7 +6604,8 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (readlen > 0) { /* Update connection-level flow control window for ignored DATA frame too */ - rv = session_update_recv_connection_window_size(session, readlen); + rv = nghttp2_session_update_recv_connection_window_size(session, + readlen); if (nghttp2_is_fatal(rv)) { return rv; } diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index 90ead9c0..d2082731 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -898,4 +898,36 @@ int nghttp2_session_terminate_session_with_reason(nghttp2_session *session, uint32_t error_code, const char *reason); +/* + * Accumulates received bytes |delta_size| for connection-level flow + * control and decides whether to send WINDOW_UPDATE to the + * connection. If NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE is set, + * WINDOW_UPDATE will not be sent. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * NGHTTP2_ERR_NOMEM + * Out of memory. + */ +int nghttp2_session_update_recv_connection_window_size(nghttp2_session *session, + size_t delta_size); + +/* + * Accumulates received bytes |delta_size| for stream-level flow + * control and decides whether to send WINDOW_UPDATE to that stream. + * If NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE is set, WINDOW_UPDATE will not + * be sent. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * NGHTTP2_ERR_NOMEM + * Out of memory. + */ +int nghttp2_session_update_recv_stream_window_size(nghttp2_session *session, + nghttp2_stream *stream, + size_t delta_size, + int send_window_update); + #endif /* NGHTTP2_SESSION_H */ diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index f604eff5..744a49cf 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -450,6 +450,13 @@ int nghttp2_session_set_local_window_size(nghttp2_session *session, if (rv != 0) { return rv; } + + if (window_size_increment > 0) { + return nghttp2_session_add_window_update(session, 0, stream_id, + window_size_increment); + } + + return nghttp2_session_update_recv_connection_window_size(session, 0); } else { stream = nghttp2_session_get_stream(session, stream_id); @@ -476,11 +483,14 @@ int nghttp2_session_set_local_window_size(nghttp2_session *session, if (rv != 0) { return rv; } - } - if (window_size_increment > 0) { - return nghttp2_session_add_window_update(session, 0, stream_id, - window_size_increment); + if (window_size_increment > 0) { + return nghttp2_session_add_window_update(session, 0, stream_id, + window_size_increment); + } + + return nghttp2_session_update_recv_stream_window_size(session, stream, 0, + 1); } return 0; diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index b366a6aa..53a0a9f0 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -10468,6 +10468,62 @@ void test_nghttp2_session_set_local_window_size(void) { CU_ASSERT(0 == nghttp2_session_send(session)); nghttp2_session_del(session); + + /* Make sure that nghttp2_session_set_local_window_size submits + WINDOW_UPDATE if necessary to increase stream-level window. */ + nghttp2_session_client_new(&session, &callbacks, NULL); + stream = open_sent_stream(session, 1); + stream->recv_window_size = NGHTTP2_INITIAL_WINDOW_SIZE; + + CU_ASSERT(0 == nghttp2_session_set_local_window_size( + session, NGHTTP2_FLAG_NONE, 1, 0)); + CU_ASSERT(0 == stream->recv_window_size); + CU_ASSERT(0 == nghttp2_session_get_stream_local_window_size(session, 1)); + /* This should submit WINDOW_UPDATE frame because stream-level + receiving window is now full. */ + CU_ASSERT(0 == + nghttp2_session_set_local_window_size(session, NGHTTP2_FLAG_NONE, 1, + NGHTTP2_INITIAL_WINDOW_SIZE)); + CU_ASSERT(0 == stream->recv_window_size); + CU_ASSERT(NGHTTP2_INITIAL_WINDOW_SIZE == + nghttp2_session_get_stream_local_window_size(session, 1)); + + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NGHTTP2_WINDOW_UPDATE == item->frame.hd.type); + CU_ASSERT(1 == item->frame.hd.stream_id); + CU_ASSERT(NGHTTP2_INITIAL_WINDOW_SIZE == + item->frame.window_update.window_size_increment); + + nghttp2_session_del(session); + + /* Make sure that nghttp2_session_set_local_window_size submits + WINDOW_UPDATE if necessary to increase connection-level + window. */ + nghttp2_session_client_new(&session, &callbacks, NULL); + session->recv_window_size = NGHTTP2_INITIAL_WINDOW_SIZE; + + CU_ASSERT(0 == nghttp2_session_set_local_window_size( + session, NGHTTP2_FLAG_NONE, 0, 0)); + CU_ASSERT(0 == session->recv_window_size); + CU_ASSERT(0 == nghttp2_session_get_local_window_size(session)); + /* This should submit WINDOW_UPDATE frame because connection-level + receiving window is now full. */ + CU_ASSERT(0 == + nghttp2_session_set_local_window_size(session, NGHTTP2_FLAG_NONE, 0, + NGHTTP2_INITIAL_WINDOW_SIZE)); + CU_ASSERT(0 == session->recv_window_size); + CU_ASSERT(NGHTTP2_INITIAL_WINDOW_SIZE == + nghttp2_session_get_local_window_size(session)); + + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NGHTTP2_WINDOW_UPDATE == item->frame.hd.type); + CU_ASSERT(0 == item->frame.hd.stream_id); + CU_ASSERT(NGHTTP2_INITIAL_WINDOW_SIZE == + item->frame.window_update.window_size_increment); + + nghttp2_session_del(session); } void test_nghttp2_session_cancel_from_before_frame_send(void) {