From 2ba1389993729fcb6ee5794ac512f2b67b29952e Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Wed, 30 May 2018 20:24:00 -0700 Subject: [PATCH] Fix handling of SETTINGS_MAX_CONCURRENT_STREAMS. The maximum number of outgoing concurrent streams is initially limited to 100 to avoid issues when the local endpoint submits lots of requests before receiving initial SETTINGS frame from the remote endpoint, since sending them at once to the remote endpoint could lead to rejection of some of the requests. This initial limit is overwritten with the value advertised in SETTINGS_MAX_CONCURRENT_STREAMS setting by the remote endpoint, but previously, it wasn't lifted if the remote endpoint didn't advertise that setting (implying no limits), in which case the limit of 100 was retained, even though it was never advertised by the remote endpoint. Signed-off-by: Piotr Sikora --- lib/includes/nghttp2/nghttp2.h | 18 +++++++++--------- lib/nghttp2_session.c | 6 ++++++ lib/nghttp2_session.h | 6 ++++-- tests/nghttp2_session_test.c | 18 ++++++++++++++++++ 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 7f097f41..3f9d39b1 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -2478,15 +2478,15 @@ nghttp2_option_set_no_auto_window_update(nghttp2_option *option, int val); * * This option sets the SETTINGS_MAX_CONCURRENT_STREAMS value of * remote endpoint as if it is received in SETTINGS frame. Without - * specifying this option, before the local endpoint receives - * SETTINGS_MAX_CONCURRENT_STREAMS in SETTINGS frame from remote - * endpoint, SETTINGS_MAX_CONCURRENT_STREAMS is unlimited. This may - * cause problem if local endpoint submits lots of requests initially - * and sending them at once to the remote peer may lead to the - * rejection of some requests. Specifying this option to the sensible - * value, say 100, may avoid this kind of issue. This value will be - * overwritten if the local endpoint receives - * SETTINGS_MAX_CONCURRENT_STREAMS from the remote endpoint. + * specifying this option, the maximum number of outgoing concurrent + * streams is initially limited to 100 to avoid issues when the local + * endpoint submits lots of requests before receiving initial SETTINGS + * frame from the remote endpoint, since sending them at once to the + * remote endpoint could lead to rejection of some of the requests. + * This value will be overwritten when the local endpoint receives + * initial SETTINGS frame from the remote endpoint, either to the + * value advertised in SETTINGS_MAX_CONCURRENT_STREAMS or to the + * default value (unlimited) if none was advertised. */ NGHTTP2_EXTERN void nghttp2_option_set_peer_max_concurrent_streams(nghttp2_option *option, diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 540886a9..7f3aa88f 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -4410,6 +4410,12 @@ int nghttp2_session_on_settings_received(nghttp2_session *session, return session_call_on_frame_received(session, frame); } + if (!session->remote_settings_received) { + session->remote_settings.max_concurrent_streams = + NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS; + session->remote_settings_received = 1; + } + for (i = 0; i < frame->settings.niv; ++i) { nghttp2_settings_entry *entry = &frame->settings.iv[i]; diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index 82cdd3a1..5fbc7155 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -303,8 +303,10 @@ struct nghttp2_session { increased/decreased by submitting WINDOW_UPDATE. See nghttp2_submit_window_update(). */ int32_t local_window_size; - /* Settings value received from the remote endpoint. We just use ID - as index. The index = 0 is unused. */ + /* This flag is used to indicate that the local endpoint received initial + SETTINGS frame from the remote endpoint. */ + uint8_t remote_settings_received; + /* Settings value received from the remote endpoint. */ nghttp2_settings_storage remote_settings; /* Settings value of the local endpoint. */ nghttp2_settings_storage local_settings; diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index a44fe2a6..1394dce6 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -3294,6 +3294,7 @@ void test_nghttp2_session_on_settings_received(void) { nghttp2_outbound_item *item; nghttp2_nv nv = MAKE_NV(":authority", "example.org"); nghttp2_mem *mem; + nghttp2_option *option; mem = nghttp2_mem_default(); @@ -3406,6 +3407,23 @@ void test_nghttp2_session_on_settings_received(void) { nghttp2_frame_settings_free(&frame.settings, mem); nghttp2_session_del(session); + /* Check that remote SETTINGS_MAX_CONCURRENT_STREAMS is set to a value set by + nghttp2_option_set_peer_max_concurrent_streams() and reset to the default + value (unlimited) after receiving initial SETTINGS frame from the peer. */ + nghttp2_option_new(&option); + nghttp2_option_set_peer_max_concurrent_streams(option, 1000); + nghttp2_session_client_new2(&session, &callbacks, NULL, option); + CU_ASSERT(1000 == session->remote_settings.max_concurrent_streams); + + nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_NONE, NULL, 0); + CU_ASSERT(0 == nghttp2_session_on_settings_received(session, &frame, 0)); + CU_ASSERT(NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS == + session->remote_settings.max_concurrent_streams); + + nghttp2_frame_settings_free(&frame.settings, mem); + nghttp2_session_del(session); + nghttp2_option_del(option); + /* Check too large SETTINGS_MAX_FRAME_SIZE */ nghttp2_session_server_new(&session, &callbacks, NULL);