From dcfa421d6f56dfdde0f31faa75bb50a2f48ee720 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 7 Aug 2013 22:02:30 +0900 Subject: [PATCH] Fix connection-level flow control (local) Fix the bug that connection-level local window is not updated for the data is the last part of the stream. For the stream level window may ignore this, connection-level window must be updated. Also this change fixes the bug that connection-level window is not updated for the ignored DATA frames. --- lib/nghttp2_session.c | 145 +++++++++++++++++++++-------------- tests/main.c | 2 + tests/nghttp2_session_test.c | 59 ++++++++++++++ tests/nghttp2_session_test.h | 1 + 4 files changed, 149 insertions(+), 58 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 55d3e30f..6a41d7ad 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -2556,8 +2556,49 @@ static int32_t adjust_recv_window_size(int32_t recv_window_size, int32_t delta) } /* - * Accumulates received bytes |delta_size| and decides whether to send - * WINDOW_UPDATE. If NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE is set, + * 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 nghttp2_session_update_recv_stream_window_size +(nghttp2_session *session, + nghttp2_stream *stream, + int32_t delta_size) +{ + stream->recv_window_size = adjust_recv_window_size + (stream->recv_window_size, delta_size); + if(!(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE)) { + /* This is just a heuristics. */ + /* We have to use local_settings here because it is the constraint + the remote endpoint should honor. */ + if((size_t)stream->recv_window_size*2 >= + session->local_settings[NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE]) { + int r; + r = nghttp2_session_add_window_update(session, + NGHTTP2_FLAG_NONE, + stream->stream_id, + stream->recv_window_size); + if(r == 0) { + stream->recv_window_size = 0; + } else { + return r; + } + } + } + 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 @@ -2566,51 +2607,27 @@ static int32_t adjust_recv_window_size(int32_t recv_window_size, int32_t delta) * NGHTTP2_ERR_NOMEM * Out of memory. */ -static int nghttp2_session_update_recv_window_size(nghttp2_session *session, - nghttp2_stream *stream, - int32_t delta_size) +static int nghttp2_session_update_recv_connection_window_size +(nghttp2_session *session, + int32_t delta_size) { - if(stream && stream->local_flow_control) { - stream->recv_window_size = adjust_recv_window_size - (stream->recv_window_size, delta_size); - if(!(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE)) { - /* This is just a heuristics. */ - /* We have to use local_settings here because it is the constraint - the remote endpoint should honor. */ - if((size_t)stream->recv_window_size*2 >= - session->local_settings[NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE]) { - int r; - r = nghttp2_session_add_window_update(session, - NGHTTP2_FLAG_NONE, - stream->stream_id, - stream->recv_window_size); - if(r == 0) { - stream->recv_window_size = 0; - } else { - return r; - } - } - } - } - if(session->local_flow_control) { - session->recv_window_size = adjust_recv_window_size - (session->recv_window_size, delta_size); - if(!(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE)) { - /* Same heuristics above */ - if((size_t)session->recv_window_size*2 >= - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { - int r; - /* Use stream ID 0 to update connection-level flow control - window */ - r = nghttp2_session_add_window_update(session, - NGHTTP2_FLAG_NONE, - 0, - session->recv_window_size); - if(r == 0) { - session->recv_window_size = 0; - } else { - return r; - } + session->recv_window_size = adjust_recv_window_size + (session->recv_window_size, delta_size); + if(!(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE)) { + /* Same heuristics above */ + if((size_t)session->recv_window_size*2 >= + NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE) { + int r; + /* Use stream ID 0 to update connection-level flow control + window */ + r = nghttp2_session_add_window_update(session, + NGHTTP2_FLAG_NONE, + 0, + session->recv_window_size); + if(r == 0) { + session->recv_window_size = 0; + } else { + return r; } } } @@ -2727,28 +2744,40 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, session->user_data); } } + /* TODO We need on_ignored_data_chunk_recv_callback, for + ingored DATA frame, so that the application can calculate + connection-level window size. */ } session->iframe.off += readlen; inmark += readlen; - if(session->iframe.state != NGHTTP2_RECV_PAYLOAD_IGN && - nghttp2_frame_is_data_frame(session->iframe.headbuf) && - readlen > 0 && - (session->iframe.payloadlen != session->iframe.off || - (data_flags & NGHTTP2_FLAG_END_STREAM) == 0)) { - nghttp2_stream *stream; - stream = nghttp2_session_get_stream(session, data_stream_id); - if(session->local_flow_control || - (stream && stream->local_flow_control)) { - r = nghttp2_session_update_recv_window_size(session, - stream, - readlen); + if(nghttp2_frame_is_data_frame(session->iframe.headbuf) && readlen > 0) { + if(session->local_flow_control) { + /* Update connection-level flow control window for ignored + DATA frame too */ + r = nghttp2_session_update_recv_connection_window_size + (session, readlen); if(r < 0) { /* FATAL */ assert(r < NGHTTP2_ERR_FATAL); return r; } } + if(session->iframe.state != NGHTTP2_RECV_PAYLOAD_IGN && + (session->iframe.payloadlen != session->iframe.off || + (data_flags & NGHTTP2_FLAG_END_STREAM) == 0)) { + nghttp2_stream *stream; + stream = nghttp2_session_get_stream(session, data_stream_id); + if(stream && stream->local_flow_control) { + r = nghttp2_session_update_recv_stream_window_size + (session, stream, readlen); + if(r < 0) { + /* FATAL */ + assert(r < NGHTTP2_ERR_FATAL); + return r; + } + } + } } if(session->iframe.payloadlen == session->iframe.off) { if(!nghttp2_frame_is_data_frame(session->iframe.headbuf)) { diff --git a/tests/main.c b/tests/main.c index bd6eb55a..725f863f 100644 --- a/tests/main.c +++ b/tests/main.c @@ -170,6 +170,8 @@ int main(int argc, char* argv[]) test_nghttp2_session_flow_control_disable_remote) || !CU_add_test(pSuite, "session_flow_control_disable_local", test_nghttp2_session_flow_control_disable_local) || + !CU_add_test(pSuite, "session_flow_control_data_recv", + test_nghttp2_session_flow_control_data_recv) || !CU_add_test(pSuite, "session_data_read_temporal_failure", test_nghttp2_session_data_read_temporal_failure) || !CU_add_test(pSuite, "session_on_request_recv_callback", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index e6014894..93dd4833 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -2614,6 +2614,65 @@ void test_nghttp2_session_flow_control_disable_local(void) nghttp2_session_del(session); } +void test_nghttp2_session_flow_control_data_recv(void) +{ + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + uint8_t data[64*1024+16]; + nghttp2_frame_hd hd; + nghttp2_outbound_item *item; + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.send_callback = null_send_callback; + + /* Initial window size to 64KiB - 1*/ + nghttp2_session_client_new(&session, &callbacks, NULL); + + nghttp2_session_open_stream(session, 1, NGHTTP2_FLAG_NONE, + NGHTTP2_PRI_DEFAULT, NGHTTP2_STREAM_OPENED, + NULL); + + /* Create DATA frame */ + memset(data, 0, sizeof(data)); + hd.length = NGHTTP2_MAX_FRAME_LENGTH; + hd.type = NGHTTP2_DATA; + hd.flags = NGHTTP2_FLAG_END_STREAM; + hd.stream_id = 1; + nghttp2_frame_pack_frame_hd(data, &hd); + CU_ASSERT(NGHTTP2_MAX_FRAME_LENGTH+NGHTTP2_FRAME_HEAD_LENGTH == + nghttp2_session_mem_recv(session, data, + NGHTTP2_MAX_FRAME_LENGTH + + NGHTTP2_FRAME_HEAD_LENGTH)); + + item = nghttp2_session_get_next_ob_item(session); + /* Since this is the last frame, stream-level WINDOW_UPDATE is not + issued, but connection-level does. */ + CU_ASSERT(NGHTTP2_WINDOW_UPDATE == OB_CTRL_TYPE(item)); + CU_ASSERT(0 == OB_CTRL(item)->hd.stream_id); + CU_ASSERT(NGHTTP2_MAX_FRAME_LENGTH == + OB_CTRL(item)->window_update.window_size_increment); + + CU_ASSERT(0 == nghttp2_session_send(session)); + + /* Receive DATA for closed stream. They are still subject to under + connection-level flow control, since this situation arises when + RST_STREAM is issued by the remote, but the local side keeps + sending DATA frames. Without calculating connection-level window, + the subsequent flow control gets confused. */ + CU_ASSERT(NGHTTP2_MAX_FRAME_LENGTH+NGHTTP2_FRAME_HEAD_LENGTH == + nghttp2_session_mem_recv(session, data, + NGHTTP2_MAX_FRAME_LENGTH + + NGHTTP2_FRAME_HEAD_LENGTH)); + + item = nghttp2_session_get_next_ob_item(session); + CU_ASSERT(NGHTTP2_WINDOW_UPDATE == OB_CTRL_TYPE(item)); + CU_ASSERT(0 == OB_CTRL(item)->hd.stream_id); + CU_ASSERT(NGHTTP2_MAX_FRAME_LENGTH == + OB_CTRL(item)->window_update.window_size_increment); + + nghttp2_session_del(session); +} + void test_nghttp2_session_data_read_temporal_failure(void) { nghttp2_session *session; diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index d40dd980..bc23763b 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -76,6 +76,7 @@ void test_nghttp2_session_defer_data(void); void test_nghttp2_session_flow_control(void); void test_nghttp2_session_flow_control_disable_remote(void); void test_nghttp2_session_flow_control_disable_local(void); +void test_nghttp2_session_flow_control_data_recv(void); void test_nghttp2_session_data_read_temporal_failure(void); void test_nghttp2_session_on_request_recv_callback(void); void test_nghttp2_session_on_stream_close(void);