From e85418f04512f3181df41fc559a6a76621912cf0 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 12 Oct 2013 17:02:37 +0900 Subject: [PATCH] Fix local window size adjustments Now shrinking local window size properly limits the amount of WINDOW_UPDATE value so that shrinked window is honored. --- lib/includes/nghttp2/nghttp2.h | 5 +-- lib/nghttp2_helper.c | 43 ++++++++++++++++++++---- lib/nghttp2_helper.h | 14 ++++---- lib/nghttp2_session.c | 1 + lib/nghttp2_session.h | 3 ++ lib/nghttp2_stream.c | 1 + lib/nghttp2_stream.h | 3 ++ lib/nghttp2_submit.c | 8 +++-- src/nghttp.cc | 5 +-- tests/nghttp2_helper_test.c | 61 ++++++++++++++++++++++++++++++---- tests/nghttp2_session_test.c | 27 ++++++++++++--- 11 files changed, 139 insertions(+), 32 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 088f19ef..ead216c6 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -1990,11 +1990,12 @@ int nghttp2_submit_goaway(nghttp2_session *session, * should be submitted, then WINDOW_UPDATE is queued with the current * received bytes count. * + * If the |window_size_increment| is 0, the function does nothing and + * returns 0. + * * This function returns 0 if it succeeds, or one of the following * negative error codes: * - * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` - * The |delta_window_size| is 0. * :enum:`NGHTTP2_ERR_FLOW_CONTROL` * The local window size overflow or gets negative. * :enum:`NGHTTP2_ERR_STREAM_CLOSED` diff --git a/lib/nghttp2_helper.c b/lib/nghttp2_helper.c index 90ac00f3..c9c6ba12 100644 --- a/lib/nghttp2_helper.c +++ b/lib/nghttp2_helper.c @@ -94,25 +94,54 @@ void nghttp2_downcase(uint8_t *s, size_t len) int nghttp2_adjust_local_window_size(int32_t *local_window_size_ptr, int32_t *recv_window_size_ptr, - int32_t delta) + int32_t *recv_reduction_ptr, + int32_t *delta_ptr) { - if(delta > 0) { - int32_t new_recv_window_size = *recv_window_size_ptr - delta; + if(*delta_ptr > 0) { + int32_t new_recv_window_size = + nghttp2_max(0, *recv_window_size_ptr) - *delta_ptr; if(new_recv_window_size < 0) { + /* The delta size is strictly more than received bytes. Increase + local_window_size by that difference. */ + int32_t recv_reduction_diff; if(*local_window_size_ptr > NGHTTP2_MAX_WINDOW_SIZE + new_recv_window_size) { return NGHTTP2_ERR_FLOW_CONTROL; } *local_window_size_ptr -= new_recv_window_size; - new_recv_window_size = 0; + /* If there is recv_reduction due to earlier window_size + reduction, we have to adjust it too. */ + recv_reduction_diff = nghttp2_min(*recv_reduction_ptr, + -new_recv_window_size); + *recv_reduction_ptr -= recv_reduction_diff; + if(*recv_window_size_ptr < 0) { + *recv_window_size_ptr += recv_reduction_diff; + } else { + /* If *recv_window_size_ptr > 0, then those bytes are + considered to be backed to the remote peer, so it is + effectively 0 now. */ + *recv_window_size_ptr = recv_reduction_diff; + } + /* recv_reduction_diff must be paied from *delta_ptr, since it + was added in window size reduction (see below). */ + *delta_ptr -= recv_reduction_diff; + } else { + *recv_window_size_ptr = new_recv_window_size; } - *recv_window_size_ptr = new_recv_window_size; return 0; } else { - if(*local_window_size_ptr + delta < 0) { + if(*local_window_size_ptr + *delta_ptr < 0 || + *recv_window_size_ptr < INT32_MIN - *delta_ptr || + *recv_reduction_ptr > INT32_MAX + *delta_ptr) { return NGHTTP2_ERR_FLOW_CONTROL; } - *local_window_size_ptr += delta; + /* Decreasing local window size. Note that we achieve this without + noticing to the remote peer. To do this, we cut + recv_window_size by -delta. This means that we don't send + WINDOW_UPDATE for -delta bytes. */ + *local_window_size_ptr += *delta_ptr; + *recv_window_size_ptr += *delta_ptr; + *recv_reduction_ptr -= *delta_ptr; } return 0; } diff --git a/lib/nghttp2_helper.h b/lib/nghttp2_helper.h index b9343c5a..8559a12a 100644 --- a/lib/nghttp2_helper.h +++ b/lib/nghttp2_helper.h @@ -92,11 +92,12 @@ void* nghttp2_memdup(const void* src, size_t n); void nghttp2_downcase(uint8_t *s, size_t len); /* - * Adjusts |*local_window_size_ptr| and |*recv_window_size_ptr| with - * |delta| which is the WINDOW_UPDATE's window_size_increment sent - * from local side. If |delta| is strictly larger than - * |*recv_window_size_ptr|, |*local_window_size_ptr| is increased by - * delta - *recv_window_size_ptr. If |delta| is negative, + * Adjusts |*local_window_size_ptr|, |*recv_window_size_ptr|, + * |*recv_reduction_ptr| with |*delta_ptr| which is the + * WINDOW_UPDATE's window_size_increment sent from local side. If + * |delta| is strictly larger than |*recv_window_size_ptr|, + * |*local_window_size_ptr| is increased by delta - + * *recv_window_size_ptr. If |delta| is negative, * |*local_window_size_ptr| is decreased by delta. * * This function returns 0 if it succeeds, or one of the following @@ -107,7 +108,8 @@ void nghttp2_downcase(uint8_t *s, size_t len); */ int nghttp2_adjust_local_window_size(int32_t *local_window_size_ptr, int32_t *recv_window_size_ptr, - int32_t delta); + int32_t *recv_reduction_ptr, + int32_t *delta_ptr); /* * Returns non-zero if the function decided that WINDOW_UPDATE should diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 95508bf2..7eb6b642 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -183,6 +183,7 @@ static int nghttp2_session_new(nghttp2_session **session_ptr, (*session_ptr)->local_flow_control = 1; (*session_ptr)->remote_window_size = NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE; (*session_ptr)->recv_window_size = 0; + (*session_ptr)->recv_reduction = 0; (*session_ptr)->local_window_size = NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE; (*session_ptr)->goaway_flags = NGHTTP2_GOAWAY_NONE; diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index f7bda56f..67ca600e 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -173,6 +173,9 @@ struct nghttp2_session { /* Keep track of the number of bytes received without WINDOW_UPDATE. */ int32_t recv_window_size; + /* The amount of recv_window_size cut using submitting negative + value to WINDOW_UPDATE */ + int32_t recv_reduction; /* window size for local flow control. It is initially set to NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE and could be increased/decreased by submitting WINDOW_UPDATE. See diff --git a/lib/nghttp2_stream.c b/lib/nghttp2_stream.c index 048205fe..b212bf4d 100644 --- a/lib/nghttp2_stream.c +++ b/lib/nghttp2_stream.c @@ -49,6 +49,7 @@ void nghttp2_stream_init(nghttp2_stream *stream, int32_t stream_id, stream->remote_window_size = remote_initial_window_size; stream->local_window_size = local_initial_window_size; stream->recv_window_size = 0; + stream->recv_reduction = 0; } void nghttp2_stream_free(nghttp2_stream *stream) diff --git a/lib/nghttp2_stream.h b/lib/nghttp2_stream.h index 12877c6a..73cce35d 100644 --- a/lib/nghttp2_stream.h +++ b/lib/nghttp2_stream.h @@ -115,6 +115,9 @@ typedef struct { /* Keep track of the number of bytes received without WINDOW_UPDATE. */ int32_t recv_window_size; + /* The amount of recv_window_size cut using submitting negative + value to WINDOW_UPDATE */ + int32_t recv_reduction; /* window size for local flow control. It is initially set to NGHTTP2_INITIAL_WINDOW_SIZE and could be increased/decreased by submitting WINDOW_UPDATE. See nghttp2_submit_window_update(). */ diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index 9745802e..ac248e2b 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -285,7 +285,7 @@ int nghttp2_submit_window_update(nghttp2_session *session, uint8_t flags, int rv; nghttp2_stream *stream; if(window_size_increment == 0) { - return NGHTTP2_ERR_INVALID_ARGUMENT; + return 0; } flags = 0; if(stream_id == 0) { @@ -294,7 +294,8 @@ int nghttp2_submit_window_update(nghttp2_session *session, uint8_t flags, } rv = nghttp2_adjust_local_window_size(&session->local_window_size, &session->recv_window_size, - window_size_increment); + &session->recv_reduction, + &window_size_increment); if(rv != 0) { return rv; } @@ -314,7 +315,8 @@ int nghttp2_submit_window_update(nghttp2_session *session, uint8_t flags, } rv = nghttp2_adjust_local_window_size(&stream->local_window_size, &stream->recv_window_size, - window_size_increment); + &stream->recv_reduction, + &window_size_increment); if(rv != 0) { return rv; } diff --git a/src/nghttp.cc b/src/nghttp.cc index e364e547..938c7ea1 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -691,9 +691,10 @@ struct HttpClient { } } if(config.connection_window_bits != -1) { + int32_t wininc = (1 << config.connection_window_bits) - 1 + - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE; rv = nghttp2_submit_window_update - (session, NGHTTP2_FLAG_NONE, 0, - (1 << config.connection_window_bits) - 1); + (session, NGHTTP2_FLAG_NONE, 0, wininc); if(rv != 0) { return -1; } diff --git a/tests/nghttp2_helper_test.c b/tests/nghttp2_helper_test.c index f038809b..80159fc1 100644 --- a/tests/nghttp2_helper_test.c +++ b/tests/nghttp2_helper_test.c @@ -32,46 +32,93 @@ void test_nghttp2_adjust_local_window_size(void) { int32_t local_window_size = 100; int32_t recv_window_size = 50; + int32_t recv_reduction = 0; + int32_t delta; + delta = 0; CU_ASSERT(0 == nghttp2_adjust_local_window_size(&local_window_size, - &recv_window_size, 0)); + &recv_window_size, + &recv_reduction, + &delta)); CU_ASSERT(100 == local_window_size); CU_ASSERT(50 == recv_window_size); + CU_ASSERT(0 == recv_reduction); + CU_ASSERT(0 == delta); + delta = 49; CU_ASSERT(0 == nghttp2_adjust_local_window_size(&local_window_size, - &recv_window_size, 49)); + &recv_window_size, + &recv_reduction, + &delta)); CU_ASSERT(100 == local_window_size); CU_ASSERT(1 == recv_window_size); + CU_ASSERT(0 == recv_reduction); + CU_ASSERT(49 == delta); + delta = 1; CU_ASSERT(0 == nghttp2_adjust_local_window_size(&local_window_size, - &recv_window_size, 1)); + &recv_window_size, + &recv_reduction, + &delta)); CU_ASSERT(100 == local_window_size); CU_ASSERT(0 == recv_window_size); + CU_ASSERT(0 == recv_reduction); + CU_ASSERT(1 == delta); + delta = 1; CU_ASSERT(0 == nghttp2_adjust_local_window_size(&local_window_size, - &recv_window_size, 1)); + &recv_window_size, + &recv_reduction, + &delta)); CU_ASSERT(101 == local_window_size); CU_ASSERT(0 == recv_window_size); + CU_ASSERT(0 == recv_reduction); + CU_ASSERT(1 == delta); + delta = -1; CU_ASSERT(0 == nghttp2_adjust_local_window_size(&local_window_size, - &recv_window_size, -1)); + &recv_window_size, + &recv_reduction, + &delta)); CU_ASSERT(100 == local_window_size); + CU_ASSERT(-1 == recv_window_size); + CU_ASSERT(1 == recv_reduction); + CU_ASSERT(-1 == delta); + + delta = 1; + CU_ASSERT(0 == nghttp2_adjust_local_window_size(&local_window_size, + &recv_window_size, + &recv_reduction, + &delta)); + CU_ASSERT(101 == local_window_size); CU_ASSERT(0 == recv_window_size); + CU_ASSERT(0 == recv_reduction); + CU_ASSERT(0 == delta); + local_window_size = 100; recv_window_size = 50; + recv_reduction = 0; + delta = INT32_MAX; CU_ASSERT(NGHTTP2_ERR_FLOW_CONTROL == nghttp2_adjust_local_window_size(&local_window_size, &recv_window_size, - INT32_MAX)); + &recv_reduction, + &delta)); CU_ASSERT(100 == local_window_size); CU_ASSERT(50 == recv_window_size); + CU_ASSERT(0 == recv_reduction); + CU_ASSERT(INT32_MAX == delta); + delta = INT32_MIN; CU_ASSERT(NGHTTP2_ERR_FLOW_CONTROL == nghttp2_adjust_local_window_size(&local_window_size, &recv_window_size, - INT32_MIN)); + &recv_reduction, + &delta)); CU_ASSERT(100 == local_window_size); CU_ASSERT(50 == recv_window_size); + CU_ASSERT(0 == recv_reduction); + CU_ASSERT(INT32_MIN == delta); } static int check_header_name(const char *s) diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 8886de1d..8dd3be2f 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -2432,7 +2432,7 @@ void test_nghttp2_submit_window_update(void) CU_ASSERT(0 == nghttp2_session_send(session)); CU_ASSERT(0 == stream->recv_window_size); - CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + CU_ASSERT(0 == nghttp2_submit_window_update(session, NGHTTP2_FLAG_NONE, 2, 0)); /* Disable local flow control */ stream->local_flow_control = 0; @@ -2472,17 +2472,26 @@ void test_nghttp2_submit_window_update_local_window_size(void) CU_ASSERT(0 == nghttp2_session_send(session)); /* Let's decrement local window size */ - stream->recv_window_size = 32768; + stream->recv_window_size = 65536; CU_ASSERT(0 == nghttp2_submit_window_update(session, NGHTTP2_FLAG_NONE, 2, -stream->local_window_size / 2)); CU_ASSERT(32768 == stream->local_window_size); CU_ASSERT(0 == stream->recv_window_size); + CU_ASSERT(32768 == stream->recv_reduction); + item = nghttp2_session_get_next_ob_item(session); CU_ASSERT(NGHTTP2_WINDOW_UPDATE == OB_CTRL_TYPE(item)); CU_ASSERT(32768 == OB_CTRL(item)->window_update.window_size_increment); - CU_ASSERT(0 == nghttp2_session_send(session)); + /* Increase local window size */ + CU_ASSERT(0 == nghttp2_submit_window_update(session, NGHTTP2_FLAG_NONE, 2, + 16384)); + CU_ASSERT(49152 == stream->local_window_size); + CU_ASSERT(16384 == stream->recv_window_size); + CU_ASSERT(16384 == stream->recv_reduction); + CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(session)); + CU_ASSERT(NGHTTP2_ERR_FLOW_CONTROL == nghttp2_submit_window_update(session, NGHTTP2_FLAG_NONE, 2, NGHTTP2_MAX_WINDOW_SIZE)); @@ -2503,17 +2512,25 @@ void test_nghttp2_submit_window_update_local_window_size(void) CU_ASSERT(0 == nghttp2_session_send(session)); /* Go decrement part */ - session->recv_window_size = 32768; + session->recv_window_size = 65536; CU_ASSERT(0 == nghttp2_submit_window_update(session, NGHTTP2_FLAG_NONE, 0, -session->local_window_size/2)); CU_ASSERT(32768 == session->local_window_size); CU_ASSERT(0 == session->recv_window_size); + CU_ASSERT(32768 == session->recv_reduction); item = nghttp2_session_get_next_ob_item(session); CU_ASSERT(NGHTTP2_WINDOW_UPDATE == OB_CTRL_TYPE(item)); CU_ASSERT(32768 == OB_CTRL(item)->window_update.window_size_increment); - CU_ASSERT(0 == nghttp2_session_send(session)); + /* Increase local window size */ + CU_ASSERT(0 == nghttp2_submit_window_update(session, NGHTTP2_FLAG_NONE, 0, + 16384)); + CU_ASSERT(49152 == session->local_window_size); + CU_ASSERT(16384 == session->recv_window_size); + CU_ASSERT(16384 == session->recv_reduction); + CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(session)); + CU_ASSERT(NGHTTP2_ERR_FLOW_CONTROL == nghttp2_submit_window_update(session, NGHTTP2_FLAG_NONE, 0, NGHTTP2_MAX_WINDOW_SIZE));