From 8e9455188162f378bd4b8fa4dbbf999acc911566 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 13 Nov 2014 00:17:28 +0900 Subject: [PATCH] Handle idle stream in priority field --- lib/nghttp2_session.c | 67 +++++++++++++--- lib/nghttp2_session.h | 7 ++ tests/main.c | 4 + tests/nghttp2_session_test.c | 145 +++++++++++++++++++++++++++++++++-- tests/nghttp2_session_test.h | 2 + 5 files changed, 210 insertions(+), 15 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index f239a69c..3f23cfd3 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -588,7 +588,28 @@ int nghttp2_session_reprioritize_stream if(pri_spec->stream_id != 0) { dep_stream = nghttp2_session_get_stream_raw(session, pri_spec->stream_id); - if(!dep_stream || !nghttp2_stream_in_dep_tree(dep_stream)) { + if(session->server && !dep_stream && + session_detect_idle_stream(session, pri_spec->stream_id)) { + nghttp2_session_adjust_closed_stream(session, 1); + + nghttp2_priority_spec_default_init(&pri_spec_default); + + if(nghttp2_session_can_add_closed_stream(session, 1)) { + + dep_stream = nghttp2_session_open_stream(session, + pri_spec->stream_id, + NGHTTP2_FLAG_NONE, + &pri_spec_default, + NGHTTP2_STREAM_IDLE, + NULL); + + if(dep_stream == NULL) { + return NGHTTP2_ERR_NOMEM; + } + } else { + pri_spec = &pri_spec_default; + } + } else if(!dep_stream || !nghttp2_stream_in_dep_tree(dep_stream)) { nghttp2_priority_spec_default_init(&pri_spec_default); pri_spec = &pri_spec_default; } @@ -822,6 +843,7 @@ nghttp2_stream* nghttp2_session_open_stream(nghttp2_session *session, int stream_alloc = 0; nghttp2_priority_spec pri_spec_default; nghttp2_priority_spec *pri_spec = pri_spec_in; + ssize_t num_adjust_closed = 0; stream = nghttp2_session_get_stream_raw(session, stream_id); @@ -834,6 +856,7 @@ nghttp2_stream* nghttp2_session_open_stream(nghttp2_session *session, if(session->server && (!nghttp2_session_is_my_stream_id(session, stream_id) || initial_state == NGHTTP2_STREAM_IDLE)) { + num_adjust_closed = 1; nghttp2_session_adjust_closed_stream(session, 1); } @@ -848,9 +871,35 @@ nghttp2_stream* nghttp2_session_open_stream(nghttp2_session *session, if(pri_spec->stream_id != 0) { dep_stream = nghttp2_session_get_stream_raw(session, pri_spec->stream_id); - /* If dep_stream is not part of dependency tree, stream will get - default priority. */ - if(!dep_stream || !nghttp2_stream_in_dep_tree(dep_stream)) { + if(session->server && !dep_stream && + session_detect_idle_stream(session, pri_spec->stream_id)) { + ++num_adjust_closed; + nghttp2_session_adjust_closed_stream(session, num_adjust_closed); + + nghttp2_priority_spec_default_init(&pri_spec_default); + + if(nghttp2_session_can_add_closed_stream(session, num_adjust_closed)) { + + dep_stream = nghttp2_session_open_stream(session, + pri_spec->stream_id, + NGHTTP2_FLAG_NONE, + &pri_spec_default, + NGHTTP2_STREAM_IDLE, + NULL); + + if(dep_stream == NULL) { + if(stream_alloc) { + free(stream); + } + + return NULL; + } + } else { + pri_spec = &pri_spec_default; + } + } else if(!dep_stream || !nghttp2_stream_in_dep_tree(dep_stream)) { + /* If dep_stream is not part of dependency tree, stream will get + default priority. */ nghttp2_priority_spec_default_init(&pri_spec_default); pri_spec = &pri_spec_default; } @@ -1082,17 +1131,15 @@ void nghttp2_session_detach_closed_stream(nghttp2_session *session, --session->num_closed_streams; } -/* Returns nonzero if closed stream can not be added to linked list - now. */ -static int session_closed_stream_full(nghttp2_session *session) +int nghttp2_session_can_add_closed_stream(nghttp2_session *session, + ssize_t offset) { size_t num_stream_max; num_stream_max = nghttp2_min(session->local_settings.max_concurrent_streams, session->pending_local_max_concurrent_stream); - return (size_t)nghttp2_max(0, (ssize_t)session->num_closed_streams - 1) + - session->num_incoming_streams >= num_stream_max; + return offset + session->num_incoming_streams <= num_stream_max; } void nghttp2_session_adjust_closed_stream(nghttp2_session *session, @@ -3304,7 +3351,7 @@ int nghttp2_session_on_priority_received(nghttp2_session *session, /* PRIORITY against idle stream can create anchor node in dependency tree. */ if(!session_detect_idle_stream(session, frame->hd.stream_id) || - session_closed_stream_full(session)) { + !nghttp2_session_can_add_closed_stream(session, 1)) { return 0; } diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index d7c8e570..94f7ac05 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -431,6 +431,13 @@ void nghttp2_session_keep_closed_stream(nghttp2_session *session, void nghttp2_session_detach_closed_stream(nghttp2_session *session, nghttp2_stream *stream); +/* + * Returns nonzero if |offset| closed stream(s) can be added to closed + * linked list now. + */ +int nghttp2_session_can_add_closed_stream(nghttp2_session *session, + ssize_t offset); + /* * Deletes closed stream to ensure that number of incoming streams * including active and closed is in the maximum number of allowed diff --git a/tests/main.c b/tests/main.c index 5b0c49fb..276abe60 100644 --- a/tests/main.c +++ b/tests/main.c @@ -149,6 +149,8 @@ int main(int argc, char* argv[]) !CU_add_test(pSuite, "session_upgrade", test_nghttp2_session_upgrade) || !CU_add_test(pSuite, "session_reprioritize_stream", test_nghttp2_session_reprioritize_stream) || + !CU_add_test(pSuite, "session_reprioritize_stream_with_closed_stream_limit", + test_nghttp2_session_reprioritize_stream_with_closed_stream_limit) || !CU_add_test(pSuite, "submit_data", test_nghttp2_submit_data) || !CU_add_test(pSuite, "submit_data_read_length_too_large", test_nghttp2_submit_data_read_length_too_large) || @@ -187,6 +189,8 @@ int main(int argc, char* argv[]) test_nghttp2_submit_invalid_nv) || !CU_add_test(pSuite, "session_open_stream", test_nghttp2_session_open_stream) || + !CU_add_test(pSuite, "session_open_stream_with_closed_stream_limit", + test_nghttp2_session_open_stream_with_closed_stream_limit) || !CU_add_test(pSuite, "session_get_next_ob_item", test_nghttp2_session_get_next_ob_item) || !CU_add_test(pSuite, "session_pop_next_ob_item", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 69bd12f2..37904012 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -3049,20 +3049,27 @@ void test_nghttp2_session_reprioritize_stream(void) CU_ASSERT(10 == stream->weight); CU_ASSERT(NULL == stream->dep_prev); - /* If dep_stream does not exist, default priority is assigned. */ + /* If depenency to idle stream which is not in depdenency tree yet */ nghttp2_priority_spec_init(&pri_spec, 3, 99, 0); nghttp2_session_reprioritize_stream(session, stream, &pri_spec); - CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); - CU_ASSERT(NULL == stream->dep_prev); + CU_ASSERT(99 == stream->weight); + CU_ASSERT(3 == stream->dep_prev->stream_id); + + dep_stream = nghttp2_session_get_stream_raw(session, 3); + + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == dep_stream->weight); dep_stream = open_stream(session, 3); + /* Change weight */ + pri_spec.weight = 128; + nghttp2_session_reprioritize_stream(session, stream, &pri_spec); - CU_ASSERT(99 == stream->weight); + CU_ASSERT(128 == stream->weight); CU_ASSERT(dep_stream == stream->dep_prev); /* Test circular dependency; stream 1 is first removed and becomes @@ -3074,6 +3081,59 @@ void test_nghttp2_session_reprioritize_stream(void) CU_ASSERT(1 == dep_stream->weight); CU_ASSERT(stream == dep_stream->dep_prev); + /* Making priority to closed stream will result in default + priority */ + session->last_recv_stream_id = 9; + + nghttp2_priority_spec_init(&pri_spec, 5, 5, 0); + + nghttp2_session_reprioritize_stream(session, stream, &pri_spec); + + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); + + nghttp2_session_del(session); +} + +void test_nghttp2_session_reprioritize_stream_with_closed_stream_limit(void) +{ + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + nghttp2_stream *stream; + nghttp2_priority_spec pri_spec; + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.send_callback = block_count_send_callback; + + nghttp2_session_server_new(&session, &callbacks, NULL); + + stream = nghttp2_session_open_stream(session, 1, NGHTTP2_STREAM_FLAG_NONE, + &pri_spec_default, + NGHTTP2_STREAM_OPENING, NULL); + + session->pending_local_max_concurrent_stream = 1; + + nghttp2_priority_spec_init(&pri_spec, 101, 10, 0); + + nghttp2_session_reprioritize_stream(session, stream, &pri_spec); + + /* No room to create idle stream, so default priority was applied. */ + + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); + CU_ASSERT(NULL == stream->dep_prev); + + session->pending_local_max_concurrent_stream = 2; + + /* Now idle stream can be created */ + + nghttp2_session_reprioritize_stream(session, stream, &pri_spec); + + CU_ASSERT(10 == stream->weight); + CU_ASSERT(101 == stream->dep_prev->stream_id); + + stream = nghttp2_session_get_stream_raw(session, 101); + + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); + nghttp2_session_del(session); } @@ -4233,15 +4293,31 @@ void test_nghttp2_session_open_stream(void) CU_ASSERT(17 == stream->weight); CU_ASSERT(1 == stream->dep_prev->stream_id); - /* Dependency to non-existent stream will become default priority */ + /* Dependency to idle stream */ nghttp2_priority_spec_init(&pri_spec, 1000000007, 240, 1); stream = nghttp2_session_open_stream(session, 5, NGHTTP2_STREAM_FLAG_NONE, &pri_spec, NGHTTP2_STREAM_OPENED, NULL); + CU_ASSERT(240 == stream->weight); + CU_ASSERT(1000000007 == stream->dep_prev->stream_id); + + stream = nghttp2_session_get_stream_raw(session, 1000000007); + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); CU_ASSERT(NULL != stream->root_next); + /* Dependency to closed stream which is not in dependency tree */ + session->last_recv_stream_id = 7; + + nghttp2_priority_spec_init(&pri_spec, 7, 10, 0); + + stream = nghttp2_session_open_stream(session, 9, NGHTTP2_FLAG_NONE, + &pri_spec, NGHTTP2_STREAM_OPENED, + NULL); + + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); + nghttp2_session_del(session); nghttp2_session_client_new(&session, &callbacks, NULL); @@ -4258,6 +4334,65 @@ void test_nghttp2_session_open_stream(void) nghttp2_session_del(session); } +void test_nghttp2_session_open_stream_with_closed_stream_limit(void) +{ + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + nghttp2_stream *stream; + nghttp2_priority_spec pri_spec; + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + nghttp2_session_server_new(&session, &callbacks, NULL); + + session->pending_local_max_concurrent_stream = 1; + + /* Dependency to idle stream */ + nghttp2_priority_spec_init(&pri_spec, 101, 245, 0); + + stream = nghttp2_session_open_stream(session, 1, NGHTTP2_STREAM_FLAG_NONE, + &pri_spec, NGHTTP2_STREAM_OPENED, + NULL); + + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); + + session->pending_local_max_concurrent_stream = 3; + + /* Now another 2 streams can be added */ + stream = nghttp2_session_open_stream(session, 3, NGHTTP2_STREAM_FLAG_NONE, + &pri_spec, NGHTTP2_STREAM_OPENED, + NULL); + + CU_ASSERT(245 == stream->weight); + CU_ASSERT(101 == stream->dep_prev->stream_id); + + stream = nghttp2_session_get_stream_raw(session, 101); + + CU_ASSERT(NGHTTP2_STREAM_IDLE == stream->state); + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); + + session->pending_local_max_concurrent_stream = 4; + + /* Now another 1 stream can be added */ + + nghttp2_priority_spec_init(&pri_spec, 211, 1, 0); + + /* stream 101 was already created and does not consume another + limit. */ + stream = nghttp2_session_open_stream(session, 101, NGHTTP2_STREAM_FLAG_NONE, + &pri_spec, NGHTTP2_STREAM_OPENED, + NULL); + + CU_ASSERT(1 == stream->weight); + CU_ASSERT(211 == stream->dep_prev->stream_id); + + stream = nghttp2_session_get_stream_raw(session, 211); + + CU_ASSERT(NGHTTP2_STREAM_IDLE == stream->state); + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream->weight); + + nghttp2_session_del(session); +} + void test_nghttp2_session_get_next_ob_item(void) { nghttp2_session *session; diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index fe64849c..0cf02693 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -61,6 +61,7 @@ void test_nghttp2_session_send_push_promise(void); void test_nghttp2_session_is_my_stream_id(void); void test_nghttp2_session_upgrade(void); void test_nghttp2_session_reprioritize_stream(void); +void test_nghttp2_session_reprioritize_stream_with_closed_stream_limit(void); void test_nghttp2_submit_data(void); void test_nghttp2_submit_data_read_length_too_large(void); void test_nghttp2_submit_data_read_length_smallest(void); @@ -83,6 +84,7 @@ void test_nghttp2_submit_window_update_local_window_size(void); void test_nghttp2_submit_altsvc(void); void test_nghttp2_submit_invalid_nv(void); void test_nghttp2_session_open_stream(void); +void test_nghttp2_session_open_stream_with_closed_stream_limit(void); void test_nghttp2_session_get_next_ob_item(void); void test_nghttp2_session_pop_next_ob_item(void); void test_nghttp2_session_reply_fail(void);