From 72f815d5355de82919818ee5f42c6463715b9e84 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 7 Dec 2015 22:43:15 +0900 Subject: [PATCH 01/66] Update descendant_last_cycle on nghttp2_stream_next_outbound_item Previously, we updated descendant_last_cycle in nghttp2_stream_reschedule, which is called after non-zero DATA frame. But this was not optimal since we still had old descendant_last_cycle, and new stream was scheduled based on it. Now descendant_last_cycle is updated in nghttp2_stream_next_outbound_item, which is called when stream with highest priority is selected from queue. And new stream is scheduled based on it. This commit also removes 0-reset of descendant_last_cycle and cycle in nghttp2_stream_reschedule. This could help making them lower, so that they are not overflow. But there is a pattern that it doesn't work, and we are not sure they are really useful at this moment. --- lib/nghttp2_stream.c | 28 +++++++++++----------------- lib/nghttp2_stream.h | 3 ++- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/lib/nghttp2_stream.c b/lib/nghttp2_stream.c index 54219a4d..06b64850 100644 --- a/lib/nghttp2_stream.c +++ b/lib/nghttp2_stream.c @@ -207,25 +207,13 @@ void nghttp2_stream_reschedule(nghttp2_stream *stream) { dep_stream = stream->dep_prev; for (; dep_stream; stream = dep_stream, dep_stream = dep_stream->dep_prev) { - if (nghttp2_pq_size(&dep_stream->obq) == 1) { - dep_stream->descendant_last_cycle = 0; - stream->cycle = 0; - } else { - /* We update descendant_last_cycle here, and we don't do it when - no data is written for stream. This effectively means that - we treat these streams as if they are not scheduled at all. - This does not cause disruption in scheduling machinery. It - just makes new streams scheduled a bit early. */ - dep_stream->descendant_last_cycle = stream->cycle; + nghttp2_pq_remove(&dep_stream->obq, &stream->pq_entry); - nghttp2_pq_remove(&dep_stream->obq, &stream->pq_entry); + stream->cycle = + stream_next_cycle(stream, dep_stream->descendant_last_cycle); + stream->seq = dep_stream->descendant_next_seq++; - stream->cycle = - stream_next_cycle(stream, dep_stream->descendant_last_cycle); - stream->seq = dep_stream->descendant_next_seq++; - - nghttp2_pq_push(&dep_stream->obq, &stream->pq_entry); - } + nghttp2_pq_push(&dep_stream->obq, &stream->pq_entry); DEBUGF(fprintf(stderr, "stream: stream=%d obq resched cycle=%ld\n", stream->stream_id, stream->cycle)); @@ -861,9 +849,15 @@ int nghttp2_stream_in_dep_tree(nghttp2_stream *stream) { nghttp2_outbound_item * nghttp2_stream_next_outbound_item(nghttp2_stream *stream) { nghttp2_pq_entry *ent; + nghttp2_stream *si; for (;;) { if (stream_active(stream)) { + /* Update ascendant's descendant_last_cycle here, so that we can + assure that new stream is scheduled based on it. */ + for (si = stream; si->dep_prev; si = si->dep_prev) { + si->dep_prev->descendant_last_cycle = si->cycle; + } return stream->item; } ent = nghttp2_pq_top(&stream->obq); diff --git a/lib/nghttp2_stream.h b/lib/nghttp2_stream.h index e2dddefa..bbb8b9f3 100644 --- a/lib/nghttp2_stream.h +++ b/lib/nghttp2_stream.h @@ -419,7 +419,8 @@ int nghttp2_stream_in_dep_tree(nghttp2_stream *stream); void nghttp2_stream_reschedule(nghttp2_stream *stream); /* - * Returns a stream which has highest priority. + * Returns a stream which has highest priority, updating + * descendant_last_cycle of selected stream's ancestors. */ nghttp2_outbound_item * nghttp2_stream_next_outbound_item(nghttp2_stream *stream); From 4bcc14fc67e668140ac977154c1960b78378e2f9 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 7 Dec 2015 23:08:54 +0900 Subject: [PATCH 02/66] Reschedule stream when only weight is changed Previously, we only updated stream's weight field when only weight was changed by PRIORITY frame. If stream is queued, it would be better to actually reschedule it based on new weight. This could be especially useful if weight is increased. --- lib/nghttp2_session.c | 12 +++-------- lib/nghttp2_stream.c | 46 +++++++++++++++++++++++++++++++++++++++++++ lib/nghttp2_stream.h | 6 ++++++ 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 191e2720..a9829048 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -666,15 +666,9 @@ int nghttp2_session_reprioritize_stream( assert(dep_stream); if (dep_stream == stream->dep_prev && !pri_spec->exclusive) { - /* This is minor optimization when just weight is changed. - Currently, we don't reschedule stream in this case, since we - don't retain enough information to do that - (descendant_last_cycle we used to schedule it). This means new - weight is only applied in the next scheduling, and if weight is - drastically increased, library is not responding very quickly. - If this is really an issue, we will do workaround for this. */ - dep_stream->sum_dep_weight += pri_spec->weight - stream->weight; - stream->weight = pri_spec->weight; + /* This is minor optimization when just weight is changed. */ + nghttp2_stream_change_weight(stream, pri_spec->weight); + return 0; } diff --git a/lib/nghttp2_stream.c b/lib/nghttp2_stream.c index 06b64850..e3280d43 100644 --- a/lib/nghttp2_stream.c +++ b/lib/nghttp2_stream.c @@ -222,6 +222,52 @@ void nghttp2_stream_reschedule(nghttp2_stream *stream) { } } +void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight) { + nghttp2_stream *dep_stream; + uint64_t last_cycle; + uint64_t cycle; + int32_t old_weight; + + if (stream->weight == weight) { + return; + } + + old_weight = stream->weight; + stream->weight = weight; + + dep_stream = stream->dep_prev; + + if (!dep_stream) { + return; + } + + dep_stream->sum_dep_weight += weight - old_weight; + + if (!stream->queued) { + return; + } + + last_cycle = + stream->cycle - + stream->last_writelen * NGHTTP2_MAX_WEIGHT / (uint32_t)old_weight; + + cycle = stream_next_cycle(stream, last_cycle); + + if (cycle < dep_stream->descendant_last_cycle) { + cycle = dep_stream->descendant_last_cycle; + } + + nghttp2_pq_remove(&dep_stream->obq, &stream->pq_entry); + + stream->cycle = cycle; + stream->seq = dep_stream->descendant_next_seq++; + + nghttp2_pq_push(&dep_stream->obq, &stream->pq_entry); + + DEBUGF(fprintf(stderr, "stream: stream=%d obq resched cycle=%ld\n", + stream->stream_id, stream->cycle)); +} + static nghttp2_stream *stream_last_sib(nghttp2_stream *stream) { for (; stream->sib_next; stream = stream->sib_next) ; diff --git a/lib/nghttp2_stream.h b/lib/nghttp2_stream.h index bbb8b9f3..123ea2f2 100644 --- a/lib/nghttp2_stream.h +++ b/lib/nghttp2_stream.h @@ -418,6 +418,12 @@ int nghttp2_stream_in_dep_tree(nghttp2_stream *stream); */ void nghttp2_stream_reschedule(nghttp2_stream *stream); +/* + * Changes |stream|'s weight to |weight|. If |stream| is queued, it + * will be rescheduled based on new weight. + */ +void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight); + /* * Returns a stream which has highest priority, updating * descendant_last_cycle of selected stream's ancestors. From abcdbf003979f56c8880d75542e34670a7dd0800 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 8 Dec 2015 23:15:55 +0900 Subject: [PATCH 03/66] Take into account remainder due to integer division when calculating cycle --- lib/nghttp2_stream.c | 53 +++++++++++++++++++++++++++----------------- lib/nghttp2_stream.h | 2 ++ 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/lib/nghttp2_stream.c b/lib/nghttp2_stream.c index e3280d43..0e265c06 100644 --- a/lib/nghttp2_stream.c +++ b/lib/nghttp2_stream.c @@ -80,6 +80,7 @@ void nghttp2_stream_init(nghttp2_stream *stream, int32_t stream_id, stream->queued = 0; stream->descendant_last_cycle = 0; stream->cycle = 0; + stream->pending_penalty = 0; stream->descendant_next_seq = 0; stream->seq = 0; stream->last_writelen = 0; @@ -115,9 +116,14 @@ static int stream_subtree_active(nghttp2_stream *stream) { /* * Returns next cycle for |stream|. */ -static uint64_t stream_next_cycle(nghttp2_stream *stream, uint64_t last_cycle) { - return last_cycle + - stream->last_writelen * NGHTTP2_MAX_WEIGHT / (uint32_t)stream->weight; +static void stream_next_cycle(nghttp2_stream *stream, uint64_t last_cycle) { + size_t penalty; + + penalty = + stream->last_writelen * NGHTTP2_MAX_WEIGHT + stream->pending_penalty; + + stream->cycle = last_cycle + penalty / (uint32_t)stream->weight; + stream->pending_penalty = penalty % (uint32_t)stream->weight; } static int stream_obq_push(nghttp2_stream *dep_stream, nghttp2_stream *stream) { @@ -125,8 +131,7 @@ static int stream_obq_push(nghttp2_stream *dep_stream, nghttp2_stream *stream) { for (; dep_stream && !stream->queued; stream = dep_stream, dep_stream = dep_stream->dep_prev) { - stream->cycle = - stream_next_cycle(stream, dep_stream->descendant_last_cycle); + stream_next_cycle(stream, dep_stream->descendant_last_cycle); stream->seq = dep_stream->descendant_next_seq++; DEBUGF(fprintf(stderr, "stream: stream=%d obq push cycle=%ld\n", @@ -169,6 +174,7 @@ static void stream_obq_remove(nghttp2_stream *stream) { stream->queued = 0; stream->cycle = 0; + stream->pending_penalty = 0; stream->descendant_last_cycle = 0; stream->last_writelen = 0; @@ -209,8 +215,7 @@ void nghttp2_stream_reschedule(nghttp2_stream *stream) { for (; dep_stream; stream = dep_stream, dep_stream = dep_stream->dep_prev) { nghttp2_pq_remove(&dep_stream->obq, &stream->pq_entry); - stream->cycle = - stream_next_cycle(stream, dep_stream->descendant_last_cycle); + stream_next_cycle(stream, dep_stream->descendant_last_cycle); stream->seq = dep_stream->descendant_next_seq++; nghttp2_pq_push(&dep_stream->obq, &stream->pq_entry); @@ -225,8 +230,8 @@ void nghttp2_stream_reschedule(nghttp2_stream *stream) { void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight) { nghttp2_stream *dep_stream; uint64_t last_cycle; - uint64_t cycle; int32_t old_weight; + size_t wlen_penalty; if (stream->weight == weight) { return; @@ -247,20 +252,28 @@ void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight) { return; } - last_cycle = - stream->cycle - - stream->last_writelen * NGHTTP2_MAX_WEIGHT / (uint32_t)old_weight; - - cycle = stream_next_cycle(stream, last_cycle); - - if (cycle < dep_stream->descendant_last_cycle) { - cycle = dep_stream->descendant_last_cycle; - } - nghttp2_pq_remove(&dep_stream->obq, &stream->pq_entry); - stream->cycle = cycle; - stream->seq = dep_stream->descendant_next_seq++; + wlen_penalty = stream->last_writelen * NGHTTP2_MAX_WEIGHT; + + /* Compute old stream->pending_penalty we used to calculate + stream->cycle */ + stream->pending_penalty = (stream->pending_penalty + (uint32_t)old_weight - + (wlen_penalty % (uint32_t)old_weight)) % + (uint32_t)old_weight; + + last_cycle = stream->cycle - + (wlen_penalty + stream->pending_penalty) / (uint32_t)old_weight; + + /* Now we have old stream->pending_penalty and new stream->weight in + place */ + stream_next_cycle(stream, last_cycle); + + if (stream->cycle < dep_stream->descendant_last_cycle) { + stream->cycle = dep_stream->descendant_last_cycle; + } + + /* Continue to use same stream->seq */ nghttp2_pq_push(&dep_stream->obq, &stream->pq_entry); diff --git a/lib/nghttp2_stream.h b/lib/nghttp2_stream.h index 123ea2f2..b5d07592 100644 --- a/lib/nghttp2_stream.h +++ b/lib/nghttp2_stream.h @@ -197,6 +197,8 @@ struct nghttp2_stream { int32_t local_window_size; /* weight of this stream */ int32_t weight; + /* This is unpaid penalty (offset) when calculating cycle. */ + uint32_t pending_penalty; /* sum of weight of direct descendants */ int32_t sum_dep_weight; nghttp2_stream_state state; From 7cc2d22ab5389cf45a0f43ac7fb09338fcb686fb Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 8 Dec 2015 23:33:26 +0900 Subject: [PATCH 04/66] Fix compile error with gcc --- lib/nghttp2_stream.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/nghttp2_stream.c b/lib/nghttp2_stream.c index 0e265c06..3e848d8e 100644 --- a/lib/nghttp2_stream.c +++ b/lib/nghttp2_stream.c @@ -123,7 +123,7 @@ static void stream_next_cycle(nghttp2_stream *stream, uint64_t last_cycle) { stream->last_writelen * NGHTTP2_MAX_WEIGHT + stream->pending_penalty; stream->cycle = last_cycle + penalty / (uint32_t)stream->weight; - stream->pending_penalty = penalty % (uint32_t)stream->weight; + stream->pending_penalty = (uint32_t)(penalty % (uint32_t)stream->weight); } static int stream_obq_push(nghttp2_stream *dep_stream, nghttp2_stream *stream) { @@ -258,9 +258,10 @@ void nghttp2_stream_change_weight(nghttp2_stream *stream, int32_t weight) { /* Compute old stream->pending_penalty we used to calculate stream->cycle */ - stream->pending_penalty = (stream->pending_penalty + (uint32_t)old_weight - - (wlen_penalty % (uint32_t)old_weight)) % - (uint32_t)old_weight; + stream->pending_penalty = + (uint32_t)((stream->pending_penalty + (uint32_t)old_weight - + (wlen_penalty % (uint32_t)old_weight)) % + (uint32_t)old_weight); last_cycle = stream->cycle - (wlen_penalty + stream->pending_penalty) / (uint32_t)old_weight; From 924fef1f32c9c217bdf379852cd748d71fab3b8c Mon Sep 17 00:00:00 2001 From: Andreas Pohl Date: Tue, 8 Dec 2015 15:47:04 +0100 Subject: [PATCH 05/66] libnghttp2_asio: Added io_service accessors To allow the asio wrapper to work with boost.fiber it is required to access the underlying io_service objects. --- src/asio_io_service_pool.cc | 4 ++++ src/asio_io_service_pool.h | 3 +++ src/asio_server.cc | 4 ++++ src/asio_server.h | 3 +++ src/asio_server_http2.cc | 4 ++++ src/asio_server_http2_impl.cc | 4 ++++ src/asio_server_http2_impl.h | 1 + src/includes/nghttp2/asio_http2_server.h | 3 +++ 8 files changed, 26 insertions(+) diff --git a/src/asio_io_service_pool.cc b/src/asio_io_service_pool.cc index ba405412..cbf36ad7 100644 --- a/src/asio_io_service_pool.cc +++ b/src/asio_io_service_pool.cc @@ -92,6 +92,10 @@ boost::asio::io_service &io_service_pool::get_io_service() { return io_service; } +std::vector> &io_service_pool::get_io_services() { + return io_services_; +} + } // namespace asio_http2 } // namespace nghttp2 diff --git a/src/asio_io_service_pool.h b/src/asio_io_service_pool.h index 242cac8f..03a60fe0 100644 --- a/src/asio_io_service_pool.h +++ b/src/asio_io_service_pool.h @@ -70,6 +70,9 @@ public: /// Get an io_service to use. boost::asio::io_service &get_io_service(); + /// Get access to all io_service objects. + std::vector> &get_io_services(); + private: /// The pool of io_services. std::vector> io_services_; diff --git a/src/asio_server.cc b/src/asio_server.cc index 531677f3..f9352879 100644 --- a/src/asio_server.cc +++ b/src/asio_server.cc @@ -169,6 +169,10 @@ void server::stop() { io_service_pool_.stop(); } void server::join() { io_service_pool_.join(); } +std::vector> &server::get_io_services() { + return io_service_pool_.get_io_services(); +} + } // namespace server } // namespace asio_http2 } // namespace nghttp2 diff --git a/src/asio_server.h b/src/asio_server.h index 7f601201..ed89b9a5 100644 --- a/src/asio_server.h +++ b/src/asio_server.h @@ -73,6 +73,9 @@ public: void join(); void stop(); + /// Get access to all io_service objects. + std::vector> &get_io_services(); + private: /// Initiate an asynchronous accept operation. void start_accept(tcp::acceptor &acceptor, serve_mux &mux); diff --git a/src/asio_server_http2.cc b/src/asio_server_http2.cc index 7d162e34..17808f8d 100644 --- a/src/asio_server_http2.cc +++ b/src/asio_server_http2.cc @@ -77,6 +77,10 @@ void http2::stop() { impl_->stop(); } void http2::join() { return impl_->join(); } +std::vector> &http2::get_io_services() { + return impl_->get_io_services(); +} + } // namespace server } // namespace asio_http2 diff --git a/src/asio_server_http2_impl.cc b/src/asio_server_http2_impl.cc index 79994a83..187af44e 100644 --- a/src/asio_server_http2_impl.cc +++ b/src/asio_server_http2_impl.cc @@ -59,6 +59,10 @@ void http2_impl::stop() { return server_->stop(); } void http2_impl::join() { return server_->join(); } +std::vector> &http2_impl::get_io_services() { + return server_->get_io_services(); +} + } // namespace server } // namespace asio_http2 diff --git a/src/asio_server_http2_impl.h b/src/asio_server_http2_impl.h index e4068694..d38b4307 100644 --- a/src/asio_server_http2_impl.h +++ b/src/asio_server_http2_impl.h @@ -50,6 +50,7 @@ public: bool handle(std::string pattern, request_cb cb); void stop(); void join(); + std::vector> &get_io_services(); private: std::unique_ptr server_; diff --git a/src/includes/nghttp2/asio_http2_server.h b/src/includes/nghttp2/asio_http2_server.h index 1c5a71fd..0a892576 100644 --- a/src/includes/nghttp2/asio_http2_server.h +++ b/src/includes/nghttp2/asio_http2_server.h @@ -201,6 +201,9 @@ public: // Join on http2 server and wait for it to fully stop void join(); + // Get access to the io_service objects. + std::vector> &get_io_services(); + private: std::unique_ptr impl_; }; From 86505b1c548b5aaae4e0230cc243ea027f13d8dd Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 8 Dec 2015 23:56:21 +0900 Subject: [PATCH 06/66] Update h2load howto and all manual pages --- doc/h2load.1 | 24 +++++++++++++++++++- doc/h2load.1.rst | 17 ++++++++++++++ doc/h2load.h2r | 2 ++ doc/nghttp.1 | 5 ++-- doc/nghttp.1.rst | 3 ++- doc/nghttpd.1 | 2 +- doc/nghttpx.1 | 2 +- doc/sources/h2load-howto.rst | 44 +++++++++++++++++++++--------------- 8 files changed, 75 insertions(+), 24 deletions(-) diff --git a/doc/h2load.1 b/doc/h2load.1 index 241d6505..272b9f0b 100644 --- a/doc/h2load.1 +++ b/doc/h2load.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "H2LOAD" "1" "November 29, 2015" "1.5.1-DEV" "nghttp2" +.TH "H2LOAD" "1" "December 08, 2015" "1.5.1-DEV" "nghttp2" .SH NAME h2load \- HTTP/2 benchmarking tool . @@ -374,6 +374,28 @@ The fraction of the number of connections within standard deviation range (mean +/\- sd) against total number of successful connections. .UNINDENT +.TP +.B req/s (client) +.INDENT 7.0 +.TP +.B min +The minimum request per second among all clients. +.TP +.B max +The maximum request per second among all clients. +.TP +.B mean +The mean request per second among all clients. +.TP +.B sd +The standard deviation of request per second among all clients. +server. +.TP +.B +/\- sd +The fraction of the number of connections within standard +deviation range (mean +/\- sd) against total number of successful +connections. +.UNINDENT .UNINDENT .SH FLOW CONTROL .sp diff --git a/doc/h2load.1.rst b/doc/h2load.1.rst index 448c4276..c6790728 100644 --- a/doc/h2load.1.rst +++ b/doc/h2load.1.rst @@ -213,6 +213,8 @@ is 1 second and 500ms is 500 milliseconds). Units are h, m, s or ms (hours, minutes, seconds and milliseconds, respectively). If a unit is omitted, a second is used as unit. +.. _h2load-1-output: + OUTPUT ------ @@ -301,6 +303,21 @@ time for 1st byte (of (decrypted in case of TLS) application data) deviation range (mean +/- sd) against total number of successful connections. +req/s (client) + min + The minimum request per second among all clients. + max + The maximum request per second among all clients. + mean + The mean request per second among all clients. + sd + The standard deviation of request per second among all clients. + server. + +/- sd + The fraction of the number of connections within standard + deviation range (mean +/- sd) against total number of successful + connections. + FLOW CONTROL ------------ diff --git a/doc/h2load.h2r b/doc/h2load.h2r index e022aa8d..8cabe533 100644 --- a/doc/h2load.h2r +++ b/doc/h2load.h2r @@ -1,3 +1,5 @@ +.. _h2load-1-output: + OUTPUT ------ diff --git a/doc/nghttp.1 b/doc/nghttp.1 index b246da69..f2bdc527 100644 --- a/doc/nghttp.1 +++ b/doc/nghttp.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "NGHTTP" "1" "November 29, 2015" "1.5.1-DEV" "nghttp2" +.TH "NGHTTP" "1" "December 08, 2015" "1.5.1-DEV" "nghttp2" .SH NAME nghttp \- HTTP/2 client . @@ -152,7 +152,8 @@ Default: \fB16\fP .B \-M, \-\-peer\-max\-concurrent\-streams= Use as SETTINGS_MAX_CONCURRENT_STREAMS value of remote endpoint as if it is received in SETTINGS frame. -The default is large enough as it is seen as unlimited. +.sp +Default: \fB100\fP .UNINDENT .INDENT 0.0 .TP diff --git a/doc/nghttp.1.rst b/doc/nghttp.1.rst index 128eeab6..775f86f0 100644 --- a/doc/nghttp.1.rst +++ b/doc/nghttp.1.rst @@ -116,7 +116,8 @@ OPTIONS Use as SETTINGS_MAX_CONCURRENT_STREAMS value of remote endpoint as if it is received in SETTINGS frame. - The default is large enough as it is seen as unlimited. + + Default: ``100`` .. option:: -c, --header-table-size= diff --git a/doc/nghttpd.1 b/doc/nghttpd.1 index ad8d93da..6f1978f9 100644 --- a/doc/nghttpd.1 +++ b/doc/nghttpd.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "NGHTTPD" "1" "November 29, 2015" "1.5.1-DEV" "nghttp2" +.TH "NGHTTPD" "1" "December 08, 2015" "1.5.1-DEV" "nghttp2" .SH NAME nghttpd \- HTTP/2 server . diff --git a/doc/nghttpx.1 b/doc/nghttpx.1 index a0982c49..a6f66efb 100644 --- a/doc/nghttpx.1 +++ b/doc/nghttpx.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "NGHTTPX" "1" "November 29, 2015" "1.5.1-DEV" "nghttp2" +.TH "NGHTTPX" "1" "December 08, 2015" "1.5.1-DEV" "nghttp2" .SH NAME nghttpx \- HTTP/2 proxy . diff --git a/doc/sources/h2load-howto.rst b/doc/sources/h2load-howto.rst index 7c07194f..af3c42e3 100644 --- a/doc/sources/h2load-howto.rst +++ b/doc/sources/h2load-howto.rst @@ -1,10 +1,10 @@ h2load - HTTP/2 benchmarking tool - HOW-TO ========================================== -h2load is benchmarking tool for HTTP/2. If built with +h2load is benchmarking tool for HTTP/2 and HTTP/1.1. If built with spdylay (http://tatsuhiro-t.github.io/spdylay/) library, it also -supports SPDY protocol. It supports SSL/TLS and clear text for both -HTTP/2 and SPDY. +supports SPDY protocol. It supports SSL/TLS and clear text for all +supported protocols. Basic Usage ----------- @@ -22,29 +22,37 @@ In order to set benchmark settings, specify following 3 options. If ``auto`` is given, the number of given URIs is used. Default: ``auto`` +For SSL/TLS connection, the protocol will be negotiated via ALPN/NPN. +You can set specific protocols in ``--npn-list`` option. For +cleartext connection, the default protocol is HTTP/2. To change the +protocol in cleartext connection, use ``--no-tls-proto`` option. For +convenience, ``--h1`` option forces HTTP/1.1 for both cleartext and +SSL/TLS connections. + Here is a command-line to perform benchmark to URI \https://localhost using total 100000 requests, 100 concurrent clients and 10 max -concurrent streams:: +concurrent streams: + +.. code-block:: text $ h2load -n100000 -c100 -m10 https://localhost -The benchmarking result looks like this:: +The benchmarking result looks like this: - finished in 0 sec, 385 millisec and 851 microsec, 2591 req/s, 1689 kbytes/s - requests: 1000 total, 1000 started, 1000 done, 1000 succeeded, 0 failed, 0 errored - status codes: 1000 2xx, 0 3xx, 0 4xx, 0 5xx - traffic: 667500 bytes total, 28700 bytes headers, 612000 bytes data +.. code-block:: text -The number of ``failed`` is the number of requests returned with non -2xx status. The number of ``error`` is the number of ``failed`` plus -the number of requests which failed with connection error. + finished in 7.08s, 141164.80 req/s, 555.33MB/s + requests: 1000000 total, 1000000 started, 1000000 done, 1000000 succeeded, 0 failed, 0 errored, 0 timeout + status codes: 1000000 2xx, 0 3xx, 0 4xx, 0 5xx + traffic: 4125025824 bytes total, 11023424 bytes headers (space savings 93.07%), 4096000000 bytes data + min max mean sd +/- sd + time for request: 15.31ms 146.85ms 69.78ms 9.26ms 92.43% + time for connect: 1.08ms 25.04ms 10.71ms 9.80ms 64.00% + time to 1st byte: 25.36ms 184.96ms 79.11ms 53.97ms 78.00% + req/s (client) : 1412.04 1447.84 1426.52 10.57 63.00% -The number of ``total`` in ``traffic`` is the received application -data. If SSL/TLS is used, this number is calculated after decryption. -The number of ``headers`` is the sum of payload size of response -HEADERS (or SYN_REPLY for SPDY). This number comes before -decompressing header block. The number of ``data`` is the sum of -response body. +See the h2load manual page :ref:`h2load-1-output` section for the +explanation of the above numbers. Flow Control ------------ From a4392d4a7f80a29729b3be434e749e2254bd0d8d Mon Sep 17 00:00:00 2001 From: Andreas Pohl Date: Wed, 9 Dec 2015 23:11:40 +0100 Subject: [PATCH 07/66] libnghttp2_asio: Make io_service accessors const --- src/asio_io_service_pool.cc | 2 +- src/asio_io_service_pool.h | 2 +- src/asio_server.cc | 2 +- src/asio_server.h | 2 +- src/asio_server_http2.cc | 2 +- src/asio_server_http2_impl.cc | 2 +- src/asio_server_http2_impl.h | 2 +- src/includes/nghttp2/asio_http2_server.h | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/asio_io_service_pool.cc b/src/asio_io_service_pool.cc index cbf36ad7..916f6d1e 100644 --- a/src/asio_io_service_pool.cc +++ b/src/asio_io_service_pool.cc @@ -92,7 +92,7 @@ boost::asio::io_service &io_service_pool::get_io_service() { return io_service; } -std::vector> &io_service_pool::get_io_services() { +const std::vector> &io_service_pool::get_io_services() const { return io_services_; } diff --git a/src/asio_io_service_pool.h b/src/asio_io_service_pool.h index 03a60fe0..9c29619d 100644 --- a/src/asio_io_service_pool.h +++ b/src/asio_io_service_pool.h @@ -71,7 +71,7 @@ public: boost::asio::io_service &get_io_service(); /// Get access to all io_service objects. - std::vector> &get_io_services(); + const std::vector> &get_io_services() const; private: /// The pool of io_services. diff --git a/src/asio_server.cc b/src/asio_server.cc index f9352879..ff9bb3b5 100644 --- a/src/asio_server.cc +++ b/src/asio_server.cc @@ -169,7 +169,7 @@ void server::stop() { io_service_pool_.stop(); } void server::join() { io_service_pool_.join(); } -std::vector> &server::get_io_services() { +const std::vector> &server::get_io_services() const { return io_service_pool_.get_io_services(); } diff --git a/src/asio_server.h b/src/asio_server.h index ed89b9a5..433f6530 100644 --- a/src/asio_server.h +++ b/src/asio_server.h @@ -74,7 +74,7 @@ public: void stop(); /// Get access to all io_service objects. - std::vector> &get_io_services(); + const std::vector> &get_io_services() const; private: /// Initiate an asynchronous accept operation. diff --git a/src/asio_server_http2.cc b/src/asio_server_http2.cc index 17808f8d..ca270a25 100644 --- a/src/asio_server_http2.cc +++ b/src/asio_server_http2.cc @@ -77,7 +77,7 @@ void http2::stop() { impl_->stop(); } void http2::join() { return impl_->join(); } -std::vector> &http2::get_io_services() { +const std::vector> &http2::get_io_services() const { return impl_->get_io_services(); } diff --git a/src/asio_server_http2_impl.cc b/src/asio_server_http2_impl.cc index 187af44e..62c0df29 100644 --- a/src/asio_server_http2_impl.cc +++ b/src/asio_server_http2_impl.cc @@ -59,7 +59,7 @@ void http2_impl::stop() { return server_->stop(); } void http2_impl::join() { return server_->join(); } -std::vector> &http2_impl::get_io_services() { +const std::vector> &http2_impl::get_io_services() const { return server_->get_io_services(); } diff --git a/src/asio_server_http2_impl.h b/src/asio_server_http2_impl.h index d38b4307..395310bd 100644 --- a/src/asio_server_http2_impl.h +++ b/src/asio_server_http2_impl.h @@ -50,7 +50,7 @@ public: bool handle(std::string pattern, request_cb cb); void stop(); void join(); - std::vector> &get_io_services(); + const std::vector> &get_io_services() const; private: std::unique_ptr server_; diff --git a/src/includes/nghttp2/asio_http2_server.h b/src/includes/nghttp2/asio_http2_server.h index 0a892576..8756022e 100644 --- a/src/includes/nghttp2/asio_http2_server.h +++ b/src/includes/nghttp2/asio_http2_server.h @@ -202,7 +202,7 @@ public: void join(); // Get access to the io_service objects. - std::vector> &get_io_services(); + const std::vector> &get_io_services() const; private: std::unique_ptr impl_; From 0c70ff56589e4b02c61a71d471adf1a45e23dc16 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 10 Dec 2015 23:18:02 +0900 Subject: [PATCH 08/66] Simplify --- lib/nghttp2_session.c | 74 +++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index a9829048..780ad5f1 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -2556,6 +2556,8 @@ static int session_after_frame_sent2(nghttp2_session *session) { nghttp2_bufs *framebufs = &aob->framebufs; nghttp2_frame *frame; nghttp2_mem *mem; + nghttp2_stream *stream; + nghttp2_data_aux_data *aux_data; mem = &session->mem; frame = &item->frame; @@ -2578,50 +2580,46 @@ static int session_after_frame_sent2(nghttp2_session *session) { active_outbound_item_reset(&session->aob, mem); return 0; - } else { - nghttp2_stream *stream; - nghttp2_data_aux_data *aux_data; + } - aux_data = &item->aux_data.data; + /* DATA frame */ - /* On EOF, we have already detached data. Please note that - application may issue nghttp2_submit_data() in - on_frame_send_callback (call from session_after_frame_sent1), - which attach data to stream. We don't want to detach it. */ - if (aux_data->eof) { - active_outbound_item_reset(aob, mem); + aux_data = &item->aux_data.data; - return 0; - } - - /* Reset no_copy here because next write may not use this. */ - aux_data->no_copy = 0; - - stream = nghttp2_session_get_stream(session, frame->hd.stream_id); - - /* If session is closed or RST_STREAM was queued, we won't send - further data. */ - if (nghttp2_session_predicate_data_send(session, stream) != 0) { - if (stream) { - rv = nghttp2_stream_detach_item(stream); - - if (nghttp2_is_fatal(rv)) { - return rv; - } - } - - active_outbound_item_reset(aob, mem); - - return 0; - } - - aob->item = NULL; - active_outbound_item_reset(&session->aob, mem); + /* On EOF, we have already detached data. Please note that + application may issue nghttp2_submit_data() in + on_frame_send_callback (call from session_after_frame_sent1), + which attach data to stream. We don't want to detach it. */ + if (aux_data->eof) { + active_outbound_item_reset(aob, mem); return 0; } - /* Unreachable */ - assert(0); + + /* Reset no_copy here because next write may not use this. */ + aux_data->no_copy = 0; + + stream = nghttp2_session_get_stream(session, frame->hd.stream_id); + + /* If session is closed or RST_STREAM was queued, we won't send + further data. */ + if (nghttp2_session_predicate_data_send(session, stream) != 0) { + if (stream) { + rv = nghttp2_stream_detach_item(stream); + + if (nghttp2_is_fatal(rv)) { + return rv; + } + } + + active_outbound_item_reset(aob, mem); + + return 0; + } + + aob->item = NULL; + active_outbound_item_reset(&session->aob, mem); + return 0; } From f14ac7431622c34e7cd3355719ad417fab71de5e Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 10 Dec 2015 23:54:54 +0900 Subject: [PATCH 09/66] At least check stream ID is valid when PUSH_PROMISE is received in goaway mode --- lib/nghttp2_session.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 780ad5f1..555a8aaa 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -4121,17 +4121,18 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "PUSH_PROMISE: push disabled"); } - if (session->goaway_flags) { - /* We just dicard PUSH_PROMISE after GOAWAY is sent or - received. */ - return NGHTTP2_ERR_IGN_HEADER_BLOCK; - } if (!nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) { return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "PUSH_PROMISE: invalid stream_id"); } + if (session->goaway_flags) { + /* We just dicard PUSH_PROMISE after GOAWAY is sent or + received. */ + return NGHTTP2_ERR_IGN_HEADER_BLOCK; + } + if (!session_is_new_peer_stream_id(session, frame->push_promise.promised_stream_id)) { /* The spec says if an endpoint receives a PUSH_PROMISE with From ab93db2259e64d909e001979553b3cabe2c6b3a4 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 11 Dec 2015 00:18:27 +0900 Subject: [PATCH 10/66] Add test when client sends push response to server --- tests/main.c | 2 ++ tests/nghttp2_session_test.c | 53 ++++++++++++++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 3 files changed, 56 insertions(+) diff --git a/tests/main.c b/tests/main.c index 72573040..711088a0 100644 --- a/tests/main.c +++ b/tests/main.c @@ -89,6 +89,8 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_session_recv_headers_with_priority) || !CU_add_test(pSuite, "session_recv_headers_early_response", test_nghttp2_session_recv_headers_early_response) || + !CU_add_test(pSuite, "session_server_recv_push_response", + test_nghttp2_session_server_recv_push_response) || !CU_add_test(pSuite, "session_recv_premature_headers", test_nghttp2_session_recv_premature_headers) || !CU_add_test(pSuite, "session_recv_unknown_frame", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 1c896535..7b503056 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -1435,6 +1435,59 @@ void test_nghttp2_session_recv_headers_early_response(void) { nghttp2_bufs_free(&bufs); } +void test_nghttp2_session_server_recv_push_response(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + nghttp2_bufs bufs; + nghttp2_buf *buf; + ssize_t rv; + my_user_data ud; + nghttp2_mem *mem; + nghttp2_frame frame; + nghttp2_hd_deflater deflater; + nghttp2_nv *nva; + size_t nvlen; + + mem = nghttp2_mem_default(); + frame_pack_bufs_init(&bufs); + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.on_invalid_frame_recv_callback = on_invalid_frame_recv_callback; + + nghttp2_session_server_new(&session, &callbacks, &ud); + + nghttp2_hd_deflate_init(&deflater, mem); + + nghttp2_session_open_stream(session, 2, NGHTTP2_STREAM_FLAG_NONE, + &pri_spec_default, + NGHTTP2_STREAM_RESERVED, NULL); + + nvlen = ARRLEN(resnv); + nghttp2_nv_array_copy(&nva, resnv, nvlen, mem); + nghttp2_frame_headers_init(&frame.headers, NGHTTP2_FLAG_END_HEADERS, 2, + NGHTTP2_HCAT_HEADERS, &pri_spec_default, nva, + nvlen); + rv = nghttp2_frame_pack_headers(&bufs, &frame.headers, &deflater); + + CU_ASSERT(0 == rv); + CU_ASSERT(nghttp2_bufs_len(&bufs) > 0); + + nghttp2_frame_headers_free(&frame.headers, mem); + + buf = &bufs.head->buf; + + ud.invalid_frame_recv_cb_called = 0; + + rv = nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf)); + + CU_ASSERT((ssize_t)nghttp2_buf_len(buf) == rv); + CU_ASSERT(1 == ud.invalid_frame_recv_cb_called); + + nghttp2_bufs_free(&bufs); + nghttp2_hd_deflate_free(&deflater); + nghttp2_session_del(session); +} + void test_nghttp2_session_recv_premature_headers(void) { nghttp2_session *session; nghttp2_session_callbacks callbacks; diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index 1834fe95..e834b32e 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -34,6 +34,7 @@ void test_nghttp2_session_recv_data_no_auto_flow_control(void); void test_nghttp2_session_recv_continuation(void); void test_nghttp2_session_recv_headers_with_priority(void); void test_nghttp2_session_recv_headers_early_response(void); +void test_nghttp2_session_server_recv_push_response(void); void test_nghttp2_session_recv_premature_headers(void); void test_nghttp2_session_recv_unknown_frame(void); void test_nghttp2_session_recv_unexpected_continuation(void); From 68c5deea5a6cc7edf0e248b73d318ddba7108bc2 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 11 Dec 2015 21:23:49 +0900 Subject: [PATCH 11/66] Clarify the condition when opening new stream from remote is allowed --- lib/nghttp2_session.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 555a8aaa..94b18d41 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -1320,6 +1320,15 @@ int nghttp2_session_close_stream_if_shut_rdwr(nghttp2_session *session, return 0; } +/* + * Returns nonzero if local endpoint allows reception of new stream + * from remote. + */ +static int session_allow_incoming_new_stream(nghttp2_session *session) { + return (session->goaway_flags & + (NGHTTP2_GOAWAY_TERM_ON_SEND | NGHTTP2_GOAWAY_SENT)) == 0; +} + /* * This function returns nonzero if session is closing. */ @@ -3438,17 +3447,17 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, } session->last_recv_stream_id = frame->hd.stream_id; - if (session->goaway_flags & NGHTTP2_GOAWAY_SENT) { - /* We just ignore stream after GOAWAY was queued */ - return NGHTTP2_ERR_IGN_HEADER_BLOCK; - } - if (session_is_incoming_concurrent_streams_max(session)) { return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "request HEADERS: max concurrent streams exceeded"); } + if (!session_allow_incoming_new_stream(session)) { + /* We just ignore stream after GOAWAY was sent */ + return NGHTTP2_ERR_IGN_HEADER_BLOCK; + } + if (frame->headers.pri_spec.stream_id == frame->hd.stream_id) { return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "request HEADERS: depend on itself"); @@ -3513,16 +3522,18 @@ int nghttp2_session_on_push_response_headers_received(nghttp2_session *session, session, frame, NGHTTP2_ERR_PROTO, "push response HEADERS: stream_id == 0"); } - if (session->goaway_flags) { - /* We don't accept new stream after GOAWAY is sent or received. */ - return NGHTTP2_ERR_IGN_HEADER_BLOCK; - } if (session_is_incoming_concurrent_streams_max(session)) { return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "push response HEADERS: max concurrent streams exceeded"); } + + if (!session_allow_incoming_new_stream(session)) { + /* We don't accept new stream after GOAWAY was sent. */ + return NGHTTP2_ERR_IGN_HEADER_BLOCK; + } + if (session_is_incoming_concurrent_streams_pending_max(session)) { return session_inflate_handle_invalid_stream(session, frame, NGHTTP2_ERR_REFUSED_STREAM); @@ -4127,9 +4138,8 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, session, frame, NGHTTP2_ERR_PROTO, "PUSH_PROMISE: invalid stream_id"); } - if (session->goaway_flags) { - /* We just dicard PUSH_PROMISE after GOAWAY is sent or - received. */ + if (!session_allow_incoming_new_stream(session)) { + /* We just discard PUSH_PROMISE after GOAWAY was sent */ return NGHTTP2_ERR_IGN_HEADER_BLOCK; } From 248a64f0b2ca89cbe4efaaa5c90adbb1d31766f7 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 11 Dec 2015 23:14:52 +0900 Subject: [PATCH 12/66] Compile with OpenSSL 1.1.0-pre1 --- src/HttpServer.cc | 1 + src/shrpx_ssl.cc | 1 + src/ssl.cc | 13 +++++-------- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/HttpServer.cc b/src/HttpServer.cc index 14cb0395..c331d12c 100644 --- a/src/HttpServer.cc +++ b/src/HttpServer.cc @@ -53,6 +53,7 @@ #include #include +#include #include diff --git a/src/shrpx_ssl.cc b/src/shrpx_ssl.cc index 0a7ca6ac..61e61cbf 100644 --- a/src/shrpx_ssl.cc +++ b/src/shrpx_ssl.cc @@ -41,6 +41,7 @@ #include #include #include +#include #include diff --git a/src/ssl.cc b/src/ssl.cc index bfb531f8..d5bb3562 100644 --- a/src/ssl.cc +++ b/src/ssl.cc @@ -80,12 +80,7 @@ LibsslGlobalLock::LibsslGlobalLock() { LibsslGlobalLock::~LibsslGlobalLock() { ssl_global_locks.clear(); } const char *get_tls_protocol(SSL *ssl) { - auto session = SSL_get_session(ssl); - if (!session) { - return "unknown"; - } - - switch (session->ssl_version) { + switch (SSL_version(ssl)) { case SSL2_VERSION: return "SSLv2"; case SSL3_VERSION: @@ -113,10 +108,12 @@ TLSSessionInfo *get_tls_session_info(TLSSessionInfo *tls_info, SSL *ssl) { tls_info->cipher = SSL_get_cipher_name(ssl); tls_info->protocol = get_tls_protocol(ssl); - tls_info->session_id = session->session_id; - tls_info->session_id_length = session->session_id_length; tls_info->session_reused = SSL_session_reused(ssl); + unsigned int session_id_length; + tls_info->session_id = SSL_SESSION_get_id(session, &session_id_length); + tls_info->session_id_length = session_id_length; + return tls_info; } From 228d92244aa66b80e506fd3e4ddc6b2fc45efb5a Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 15 Dec 2015 00:21:13 +0900 Subject: [PATCH 13/66] Optimize nghttp2_pq swap --- lib/nghttp2_pq.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/nghttp2_pq.c b/lib/nghttp2_pq.c index 54c3eca4..bebccc76 100644 --- a/lib/nghttp2_pq.c +++ b/lib/nghttp2_pq.c @@ -44,11 +44,13 @@ void nghttp2_pq_free(nghttp2_pq *pq) { } static void swap(nghttp2_pq *pq, size_t i, size_t j) { - nghttp2_pq_entry *t = pq->q[i]; - pq->q[i] = pq->q[j]; - pq->q[i]->index = i; - pq->q[j] = t; - pq->q[j]->index = j; + nghttp2_pq_entry *a = pq->q[i]; + nghttp2_pq_entry *b = pq->q[j]; + + pq->q[i] = b; + b->index = i; + pq->q[j] = a; + a->index = j; } static void bubble_up(nghttp2_pq *pq, size_t index) { From 71012fe83a9ab4f8ea8c4247f7b3cce4e1f96684 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 15 Dec 2015 22:47:05 +0900 Subject: [PATCH 14/66] nghttpx: Add constexpr --- src/shrpx_client_handler.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 8ace9d4a..9e70e457 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -729,10 +729,11 @@ int ClientHandler::perform_http2_upgrade(HttpsUpstream *http) { on_read_ = &ClientHandler::upstream_http2_connhd_read; write_ = &ClientHandler::write_clear; - static char res[] = "HTTP/1.1 101 Switching Protocols\r\n" - "Connection: Upgrade\r\n" - "Upgrade: " NGHTTP2_CLEARTEXT_PROTO_VERSION_ID "\r\n" - "\r\n"; + static constexpr char res[] = + "HTTP/1.1 101 Switching Protocols\r\n" + "Connection: Upgrade\r\n" + "Upgrade: " NGHTTP2_CLEARTEXT_PROTO_VERSION_ID "\r\n" + "\r\n"; upstream->get_response_buf()->write(res, sizeof(res) - 1); upstream_ = std::move(upstream); From ef7d6e8a0c5096021237abea55f055679866b3e7 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 16 Dec 2015 00:38:30 +0900 Subject: [PATCH 15/66] nghttpx: Loose HTTP Upgrade condition --- src/shrpx_client_handler.cc | 39 +++++++++++++++++++++++++++++++------ src/shrpx_https_upstream.cc | 5 ----- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 9e70e457..b6efe4d7 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -718,6 +718,34 @@ void ClientHandler::direct_http2_upgrade() { int ClientHandler::perform_http2_upgrade(HttpsUpstream *http) { auto upstream = make_unique(this); + + auto output = upstream->get_response_buf(); + + // We might have written non-final header in response_buf, in this + // case, response_state is still INITIAL. If this non-final header + // and upgrade header fit in output buffer, do upgrade. Otherwise, + // to avoid to send this non-final header as response body in HTTP/2 + // upstream, fail upgrade. + auto downstream = http->get_downstream(); + auto input = downstream->get_response_buf(); + + static constexpr char res[] = + "HTTP/1.1 101 Switching Protocols\r\n" + "Connection: Upgrade\r\n" + "Upgrade: " NGHTTP2_CLEARTEXT_PROTO_VERSION_ID "\r\n" + "\r\n"; + + auto required_size = str_size(res) + input->rleft(); + + if (output->wleft() < required_size) { + if (LOG_ENABLED(INFO)) { + CLOG(INFO, this) + << "HTTP Upgrade failed because of insufficient buffer space: need " + << required_size << ", available " << output->wleft(); + } + return -1; + } + if (upstream->upgrade_upstream(http) != 0) { return -1; } @@ -729,12 +757,11 @@ int ClientHandler::perform_http2_upgrade(HttpsUpstream *http) { on_read_ = &ClientHandler::upstream_http2_connhd_read; write_ = &ClientHandler::write_clear; - static constexpr char res[] = - "HTTP/1.1 101 Switching Protocols\r\n" - "Connection: Upgrade\r\n" - "Upgrade: " NGHTTP2_CLEARTEXT_PROTO_VERSION_ID "\r\n" - "\r\n"; - upstream->get_response_buf()->write(res, sizeof(res) - 1); + auto nread = + downstream->get_response_buf()->remove(output->last, output->wleft()); + output->write(nread); + + output->write(res, str_size(res)); upstream_ = std::move(upstream); signal_write(); diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 2f9cd831..1acd8f4a 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -409,11 +409,6 @@ int htp_msg_completecb(http_parser *htp) { if (handler->get_http2_upgrade_allowed() && downstream->get_http2_upgrade_request() && - // we may write non-final header in response_buf, in this case, - // response_state is still INITIAL. So don't upgrade in this - // case, otherwise we end up send this non-final header as - // response body in HTTP/2 upstream. - downstream->get_response_buf()->rleft() == 0 && handler->perform_http2_upgrade(upstream) != 0) { if (LOG_ENABLED(INFO)) { ULOG(INFO, upstream) << "HTTP Upgrade to HTTP/2 failed"; From 15d9f222eddfbdafab501e5b23ebe5d8b4be83f2 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 16 Dec 2015 21:31:43 +0900 Subject: [PATCH 16/66] Add --lib-only configure option This is a short hand for --disable-app --disable-examples --disable-hpack-tools --disable-python-bindings, for users who want to build libnghttp2 only. --- configure.ac | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/configure.ac b/configure.ac index 6bc25f4d..d2aaa651 100644 --- a/configure.ac +++ b/configure.ac @@ -104,6 +104,11 @@ AC_ARG_ENABLE([failmalloc], [Do not build failmalloc test program])], [request_failmalloc=$enableval], [request_failmalloc=yes]) +AC_ARG_ENABLE([lib-only], + [AS_HELP_STRING([--enable-lib-only], + [Build libnghttp2 only. This is a short hand for --disable-app --disable-examples --disable-hpack-tools --disable-python-bindings])], + [request_lib_only=$enableval], [request_lib_only=no]) + AC_ARG_WITH([libxml2], [AS_HELP_STRING([--with-libxml2], [Use libxml2 [default=check]])], @@ -150,6 +155,13 @@ PKG_PROG_PKG_CONFIG([0.20]) AM_PATH_PYTHON([2.7],, [:]) +if [test "x$request_lib_only" = "xyes"]; then + request_app=no + request_hpack_tools=no + request_examples=no + request_python_bindings=no +fi + if [test "x$request_python_bindings" != "xno"]; then AX_PYTHON_DEVEL([>= '2.7']) fi From a30dad4f5e2ce6a3ebd8a955fd1790dc53353bc8 Mon Sep 17 00:00:00 2001 From: Andreas Pohl Date: Wed, 16 Dec 2015 18:38:21 +0100 Subject: [PATCH 17/66] libnghttp2_asio: Added access to a requests remote endpoint --- src/asio_server_connection.h | 5 +++-- src/asio_server_http2_handler.cc | 8 +++++++- src/asio_server_http2_handler.h | 6 +++++- src/asio_server_request.cc | 4 ++++ src/asio_server_request_impl.cc | 8 ++++++++ src/asio_server_request_impl.h | 5 +++++ src/includes/nghttp2/asio_http2_server.h | 3 +++ 7 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/asio_server_connection.h b/src/asio_server_connection.h index 3aba7336..beac04f8 100644 --- a/src/asio_server_connection.h +++ b/src/asio_server_connection.h @@ -70,8 +70,9 @@ public: /// Start the first asynchronous operation for the connection. void start() { - handler_ = std::make_shared(socket_.get_io_service(), - [this]() { do_write(); }, mux_); + handler_ = std::make_shared( + socket_.get_io_service(), socket_.lowest_layer().remote_endpoint(), + [this]() { do_write(); }, mux_); if (handler_->start() != 0) { return; } diff --git a/src/asio_server_http2_handler.cc b/src/asio_server_http2_handler.cc index 47cd18c5..bf594f87 100644 --- a/src/asio_server_http2_handler.cc +++ b/src/asio_server_http2_handler.cc @@ -136,6 +136,9 @@ int on_frame_recv_callback(nghttp2_session *session, const nghttp2_frame *frame, break; } + auto& req = strm->request().impl(); + req.remote_endpoint(handler->remote_endpoint()); + handler->call_on_request(*strm); if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { @@ -225,8 +228,9 @@ int on_frame_not_send_callback(nghttp2_session *session, } // namespace http2_handler::http2_handler(boost::asio::io_service &io_service, + boost::asio::ip::tcp::endpoint ep, connection_write writefun, serve_mux &mux) - : writefun_(writefun), mux_(mux), io_service_(io_service), + : writefun_(writefun), mux_(mux), io_service_(io_service), remote_ep_(ep), session_(nullptr), buf_(nullptr), buflen_(0), inside_callback_(false), tstamp_cached_(time(nullptr)), formatted_date_(util::http_date(tstamp_cached_)) {} @@ -449,6 +453,8 @@ response *http2_handler::push_promise(boost::system::error_code &ec, boost::asio::io_service &http2_handler::io_service() { return io_service_; } +boost::asio::ip::tcp::endpoint http2_handler::remote_endpoint() { return remote_ep_; } + callback_guard::callback_guard(http2_handler &h) : handler(h) { handler.enter_callback(); } diff --git a/src/asio_server_http2_handler.h b/src/asio_server_http2_handler.h index 068d4527..19dcfb77 100644 --- a/src/asio_server_http2_handler.h +++ b/src/asio_server_http2_handler.h @@ -53,7 +53,8 @@ using connection_write = std::function; class http2_handler : public std::enable_shared_from_this { public: - http2_handler(boost::asio::io_service &io_service, connection_write writefun, + http2_handler(boost::asio::io_service &io_service, + boost::asio::ip::tcp::endpoint ep, connection_write writefun, serve_mux &mux); ~http2_handler(); @@ -89,6 +90,8 @@ public: boost::asio::io_service &io_service(); + boost::asio::ip::tcp::endpoint remote_endpoint(); + const std::string &http_date(); template @@ -152,6 +155,7 @@ private: connection_write writefun_; serve_mux &mux_; boost::asio::io_service &io_service_; + boost::asio::ip::tcp::endpoint remote_ep_; nghttp2_session *session_; const uint8_t *buf_; std::size_t buflen_; diff --git a/src/asio_server_request.cc b/src/asio_server_request.cc index 0241c489..8237ff76 100644 --- a/src/asio_server_request.cc +++ b/src/asio_server_request.cc @@ -50,6 +50,10 @@ void request::on_data(data_cb cb) const { request_impl &request::impl() const { return *impl_; } +boost::asio::ip::tcp::endpoint request::remote_endpoint() const { + return impl_->remote_endpoint(); +} + } // namespace server } // namespace asio_http2 } // namespace nghttp2 diff --git a/src/asio_server_request_impl.cc b/src/asio_server_request_impl.cc index 3a78208f..a7ab88c7 100644 --- a/src/asio_server_request_impl.cc +++ b/src/asio_server_request_impl.cc @@ -54,6 +54,14 @@ void request_impl::call_on_data(const uint8_t *data, std::size_t len) { } } +boost::asio::ip::tcp::endpoint request_impl::remote_endpoint() const { + return remote_ep_; +} + +void request_impl::remote_endpoint(boost::asio::ip::tcp::endpoint ep) { + remote_ep_ = ep; +} + } // namespace server } // namespace asio_http2 } // namespace nghttp2 diff --git a/src/asio_server_request_impl.h b/src/asio_server_request_impl.h index 8d4b0149..087b65ac 100644 --- a/src/asio_server_request_impl.h +++ b/src/asio_server_request_impl.h @@ -28,6 +28,7 @@ #include "nghttp2_config.h" #include +#include namespace nghttp2 { namespace asio_http2 { @@ -54,12 +55,16 @@ public: void stream(class stream *s); void call_on_data(const uint8_t *data, std::size_t len); + boost::asio::ip::tcp::endpoint remote_endpoint() const; + void remote_endpoint(boost::asio::ip::tcp::endpoint ep); + private: class stream *strm_; header_map header_; std::string method_; uri_ref uri_; data_cb on_data_cb_; + boost::asio::ip::tcp::endpoint remote_ep_; }; } // namespace server diff --git a/src/includes/nghttp2/asio_http2_server.h b/src/includes/nghttp2/asio_http2_server.h index 8756022e..f98c6c79 100644 --- a/src/includes/nghttp2/asio_http2_server.h +++ b/src/includes/nghttp2/asio_http2_server.h @@ -59,6 +59,9 @@ public: // Application must not call this directly. request_impl &impl() const; + // Returns the remote endpoint of the request + boost::asio::ip::tcp::endpoint remote_endpoint() const; + private: std::unique_ptr impl_; }; From 9cfda0c070b24850aea05b35516d236cb44d5249 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 17 Dec 2015 18:04:16 +0900 Subject: [PATCH 18/66] Update doc --- lib/nghttp2_session.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 94b18d41..43474b96 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3633,6 +3633,8 @@ static int session_process_headers_frame(nghttp2_session *session) { return nghttp2_session_on_response_headers_received(session, frame, stream); } + /* This handles the case when server received HEADERS for stream + in reserved stream from client. That is protocol error. */ frame->headers.cat = NGHTTP2_HCAT_HEADERS; return nghttp2_session_on_headers_received(session, frame, stream); } From 9f8fc7b2bb5614886f4eea6dacb22f95246356a9 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 17 Dec 2015 21:30:30 +0900 Subject: [PATCH 19/66] Strict error handling for frames which are not allowed after closed (remote) This makes h2spec strict mode a bit happier. We still one failing test with h2spec -S (strict mode). --- lib/nghttp2_session.c | 41 ++++++++++------- tests/main.c | 2 + tests/nghttp2_session_test.c | 86 +++++++++++++++++++++++++++++++++--- tests/nghttp2_session_test.h | 1 + 4 files changed, 107 insertions(+), 23 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 43474b96..98b75bb9 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3443,6 +3443,12 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, "request HEADERS: invalid stream_id"); } + stream = nghttp2_session_get_stream_raw(session, frame->hd.stream_id); + if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) { + return session_inflate_handle_invalid_connection( + session, frame, NGHTTP2_ERR_STREAM_CLOSED, "HEADERS: stream closed"); + } + return NGHTTP2_ERR_IGN_HEADER_BLOCK; } session->last_recv_stream_id = frame->hd.stream_id; @@ -3721,6 +3727,9 @@ int nghttp2_session_on_rst_stream_received(nghttp2_session *session, } } + /* We may use stream->shut_flags for strict error checking. */ + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + rv = session_call_on_frame_received(session, frame); if (rv != 0) { return rv; @@ -4165,7 +4174,11 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "PUSH_PROMISE: stream in idle"); } + + /* Currently, client does not retain closed stream, so we don't + check NGHTTP2_SHUT_RD condition here. */ } + rv = nghttp2_session_add_rst_stream( session, frame->push_promise.promised_stream_id, NGHTTP2_CANCEL); if (rv != 0) { @@ -4173,25 +4186,13 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, } return NGHTTP2_ERR_IGN_HEADER_BLOCK; } + if (stream->shut_flags & NGHTTP2_SHUT_RD) { - if (session->callbacks.on_invalid_frame_recv_callback) { - if (session->callbacks.on_invalid_frame_recv_callback( - session, frame, NGHTTP2_PROTOCOL_ERROR, session->user_data) != - 0) { - return NGHTTP2_ERR_CALLBACK_FAILURE; - } - } - rv = nghttp2_session_add_rst_stream(session, - frame->push_promise.promised_stream_id, - NGHTTP2_PROTOCOL_ERROR); - if (rv != 0) { - return rv; - } - return NGHTTP2_ERR_IGN_HEADER_BLOCK; + return session_inflate_handle_invalid_connection( + session, frame, NGHTTP2_ERR_STREAM_CLOSED, + "PUSH_PROMISE: stream closed"); } - /* TODO It is unclear reserved stream dpeneds on associated - stream with or without exclusive flag set */ nghttp2_priority_spec_init(&pri_spec, stream->stream_id, NGHTTP2_DEFAULT_WEIGHT, 0); @@ -4632,6 +4633,14 @@ static int session_on_data_received_fail_fast(nghttp2_session *session) { error_code = NGHTTP2_PROTOCOL_ERROR; goto fail; } + + stream = nghttp2_session_get_stream_raw(session, stream_id); + if (stream && (stream->shut_flags & NGHTTP2_SHUT_RD)) { + failure_reason = "DATA: stream closed"; + error_code = NGHTTP2_STREAM_CLOSED; + goto fail; + } + return NGHTTP2_ERR_IGN_PAYLOAD; } if (stream->shut_flags & NGHTTP2_SHUT_RD) { diff --git a/tests/main.c b/tests/main.c index 711088a0..4aabe0d0 100644 --- a/tests/main.c +++ b/tests/main.c @@ -128,6 +128,8 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_session_on_window_update_received) || !CU_add_test(pSuite, "session_on_data_received", test_nghttp2_session_on_data_received) || + !CU_add_test(pSuite, "session_on_data_received_fail_fast", + test_nghttp2_session_on_data_received_fail_fast) || !CU_add_test(pSuite, "session_send_headers_start_stream", test_nghttp2_session_send_headers_start_stream) || !CU_add_test(pSuite, "session_send_headers_reply", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 7b503056..f4abbb9d 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -2249,6 +2249,25 @@ void test_nghttp2_session_on_request_headers_received(void) { nghttp2_frame_headers_free(&frame.headers, mem); nghttp2_session_del(session); + + nghttp2_session_server_new(&session, &callbacks, &user_data); + + /* HEADERS to closed stream */ + stream = open_stream(session, 1); + session->last_recv_stream_id = 1; + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + nghttp2_session_close_stream(session, 1, NGHTTP2_NO_ERROR); + + nghttp2_frame_headers_init(&frame.headers, NGHTTP2_FLAG_END_HEADERS, 1, + NGHTTP2_HCAT_REQUEST, NULL, NULL, 0); + + CU_ASSERT(NGHTTP2_ERR_IGN_HEADER_BLOCK == + nghttp2_session_on_request_headers_received(session, &frame)); + CU_ASSERT(session->goaway_flags & NGHTTP2_GOAWAY_TERM_ON_SEND); + + nghttp2_frame_headers_free(&frame.headers, mem); + + nghttp2_session_del(session); } void test_nghttp2_session_on_response_headers_received(void) { @@ -2742,14 +2761,20 @@ void test_nghttp2_session_on_push_promise_received(void) { CU_ASSERT(1 == session->num_incoming_reserved_streams); CU_ASSERT(NULL == nghttp2_session_get_stream(session, 4)); item = nghttp2_session_get_next_ob_item(session); - CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); - CU_ASSERT(4 == item->frame.hd.stream_id); - CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == item->frame.rst_stream.error_code); + CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); + CU_ASSERT(NGHTTP2_STREAM_CLOSED == item->frame.goaway.error_code); CU_ASSERT(0 == nghttp2_session_send(session)); CU_ASSERT(4 == session->last_recv_stream_id); + nghttp2_session_del(session); + + nghttp2_session_client_new(&session, &callbacks, &user_data); + + stream = nghttp2_session_open_stream(session, 1, NGHTTP2_STREAM_FLAG_NONE, + &pri_spec_default, + NGHTTP2_STREAM_OPENING, NULL); + /* Attempt to PUSH_PROMISE against stream in closing state */ - stream->shut_flags = NGHTTP2_SHUT_NONE; stream->state = NGHTTP2_STREAM_CLOSING; frame.push_promise.promised_stream_id = 6; @@ -2759,7 +2784,7 @@ void test_nghttp2_session_on_push_promise_received(void) { nghttp2_session_on_push_promise_received(session, &frame)); CU_ASSERT(0 == user_data.begin_headers_cb_called); - CU_ASSERT(1 == session->num_incoming_reserved_streams); + CU_ASSERT(0 == session->num_incoming_reserved_streams); CU_ASSERT(NULL == nghttp2_session_get_stream(session, 6)); item = nghttp2_session_get_next_ob_item(session); CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); @@ -2767,7 +2792,7 @@ void test_nghttp2_session_on_push_promise_received(void) { CU_ASSERT(NGHTTP2_CANCEL == item->frame.rst_stream.error_code); CU_ASSERT(0 == nghttp2_session_send(session)); - /* Attempt to PUSH_PROMISE against non-existent stream */ + /* Attempt to PUSH_PROMISE against idle stream */ frame.hd.stream_id = 3; frame.push_promise.promised_stream_id = 8; @@ -2777,7 +2802,7 @@ void test_nghttp2_session_on_push_promise_received(void) { nghttp2_session_on_push_promise_received(session, &frame)); CU_ASSERT(0 == user_data.begin_headers_cb_called); - CU_ASSERT(1 == session->num_incoming_reserved_streams); + CU_ASSERT(0 == session->num_incoming_reserved_streams); CU_ASSERT(NULL == nghttp2_session_get_stream(session, 8)); item = nghttp2_session_get_next_ob_item(session); CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); @@ -3161,6 +3186,53 @@ void test_nghttp2_session_on_data_received(void) { nghttp2_session_del(session); } +void test_nghttp2_session_on_data_received_fail_fast(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + uint8_t buf[9]; + nghttp2_stream *stream; + nghttp2_frame_hd hd; + nghttp2_outbound_item *item; + + memset(&callbacks, 0, sizeof(callbacks)); + + nghttp2_frame_hd_init(&hd, 0, NGHTTP2_DATA, NGHTTP2_FLAG_NONE, 1); + nghttp2_frame_pack_frame_hd(buf, &hd); + + nghttp2_session_server_new(&session, &callbacks, NULL); + + /* DATA to closed (remote) */ + stream = open_stream(session, 1); + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + + CU_ASSERT((ssize_t)sizeof(buf) == + nghttp2_session_mem_recv(session, buf, sizeof(buf))); + + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NULL != item); + CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); + + nghttp2_session_del(session); + + nghttp2_session_server_new(&session, &callbacks, NULL); + + /* DATA to closed stream with explicit closed (remote) */ + stream = open_stream(session, 1); + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); + nghttp2_session_close_stream(session, 1, NGHTTP2_NO_ERROR); + + CU_ASSERT((ssize_t)sizeof(buf) == + nghttp2_session_mem_recv(session, buf, sizeof(buf))); + + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NULL != item); + CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); + + nghttp2_session_del(session); +} + void test_nghttp2_session_send_headers_start_stream(void) { nghttp2_session *session; nghttp2_session_callbacks callbacks; diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index e834b32e..ea22e76b 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -54,6 +54,7 @@ void test_nghttp2_session_on_ping_received(void); void test_nghttp2_session_on_goaway_received(void); void test_nghttp2_session_on_window_update_received(void); void test_nghttp2_session_on_data_received(void); +void test_nghttp2_session_on_data_received_fail_fast(void); void test_nghttp2_session_send_headers_start_stream(void); void test_nghttp2_session_send_headers_reply(void); void test_nghttp2_session_send_headers_frame_size_error(void); From 80f7abb565b4e92bab11759d4efefd6119220a6d Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 17 Dec 2015 22:22:22 +0900 Subject: [PATCH 20/66] Fix crash caused by the regression in 9f8fc7b2bb5614886f4eea6dacb22f95246356a9 --- lib/nghttp2_session.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 98b75bb9..0e32fc18 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3725,11 +3725,11 @@ int nghttp2_session_on_rst_stream_received(nghttp2_session *session, return session_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "RST_STREAM: stream in idle"); } + } else { + /* We may use stream->shut_flags for strict error checking. */ + nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); } - /* We may use stream->shut_flags for strict error checking. */ - nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); - rv = session_call_on_frame_received(session, frame); if (rv != 0) { return rv; From feae76fbc0fc14b6eea2900dd33141ac23a9bf97 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 18 Dec 2015 22:44:08 +0900 Subject: [PATCH 21/66] Remove unused commented lines --- tests/main.c | 2 -- tests/nghttp2_session_test.c | 42 ------------------------------------ tests/nghttp2_session_test.h | 1 - 3 files changed, 45 deletions(-) diff --git a/tests/main.c b/tests/main.c index 4aabe0d0..fb02ed5d 100644 --- a/tests/main.c +++ b/tests/main.c @@ -206,8 +206,6 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_session_reply_fail) || !CU_add_test(pSuite, "session_max_concurrent_streams", test_nghttp2_session_max_concurrent_streams) || - !CU_add_test(pSuite, "session_stream_close_on_headers_push", - test_nghttp2_session_stream_close_on_headers_push) || !CU_add_test(pSuite, "session_stop_data_with_rst_stream", test_nghttp2_session_stop_data_with_rst_stream) || !CU_add_test(pSuite, "session_defer_data", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index f4abbb9d..4902625a 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -338,16 +338,6 @@ static int send_data_callback(nghttp2_session *session _U_, return 0; } -/* static void no_stream_user_data_stream_close_callback */ -/* (nghttp2_session *session, */ -/* int32_t stream_id, */ -/* nghttp2_error_code error_code, */ -/* void *user_data) */ -/* { */ -/* my_user_data* my_data = (my_user_data*)user_data; */ -/* ++my_data->stream_close_cb_called; */ -/* } */ - static ssize_t block_count_send_callback(nghttp2_session *session _U_, const uint8_t *data _U_, size_t len, int flags _U_, void *user_data) { @@ -5249,38 +5239,6 @@ void test_nghttp2_session_max_concurrent_streams(void) { nghttp2_session_del(session); } -/* - * Check that on_stream_close_callback is called when server pushed - * HEADERS have NGHTTP2_FLAG_END_STREAM. - */ -void test_nghttp2_session_stream_close_on_headers_push(void) { - /* nghttp2_session *session; */ - /* nghttp2_session_callbacks callbacks; */ - /* const char *nv[] = { NULL }; */ - /* my_user_data ud; */ - /* nghttp2_frame frame; */ - - /* memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); */ - /* callbacks.on_stream_close_callback = */ - /* no_stream_user_data_stream_close_callback; */ - /* ud.stream_close_cb_called = 0; */ - - /* nghttp2_session_client_new(&session, NGHTTP2_PROTO_SPDY2, &callbacks, &ud); - */ - /* nghttp2_session_open_stream(session, 1, NGHTTP2_CTRL_FLAG_NONE, 3, */ - /* NGHTTP2_STREAM_OPENING, NULL); */ - /* nghttp2_frame_syn_stream_init(&frame.syn_stream, NGHTTP2_PROTO_SPDY2, */ - /* NGHTTP2_CTRL_FLAG_FIN | */ - /* NGHTTP2_CTRL_FLAG_UNIDIRECTIONAL, */ - /* 2, 1, 3, dup_nv(nv)); */ - - /* CU_ASSERT(0 == nghttp2_session_on_request_headers_received(session, - * &frame)); */ - - /* nghttp2_frame_syn_stream_free(&frame.syn_stream); */ - /* nghttp2_session_del(session); */ -} - void test_nghttp2_session_stop_data_with_rst_stream(void) { nghttp2_session *session; nghttp2_session_callbacks callbacks; diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index ea22e76b..1a0d47b3 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -95,7 +95,6 @@ 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); void test_nghttp2_session_max_concurrent_streams(void); -void test_nghttp2_session_stream_close_on_headers_push(void); void test_nghttp2_session_stop_data_with_rst_stream(void); void test_nghttp2_session_defer_data(void); void test_nghttp2_session_flow_control(void); From 2d2188e77ba8321f5252cf6bd763c141aebe889f Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 19 Dec 2015 18:35:01 +0900 Subject: [PATCH 22/66] src: Add 451 status code https://datatracker.ietf.org/doc/draft-ietf-httpbis-legally-restricted-status/ --- src/http2.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/http2.cc b/src/http2.cc index 671b1a92..6f9128e1 100644 --- a/src/http2.cc +++ b/src/http2.cc @@ -113,6 +113,8 @@ std::string get_status_string(unsigned int status_code) { return "429 Too Many Requests"; case 431: return "431 Request Header Fields Too Large"; + case 451: + return "451 Unavailable For Legal Reasons"; case 500: return "500 Internal Server Error"; case 501: @@ -215,6 +217,8 @@ const char *stringify_status(unsigned int status_code) { return "429"; case 431: return "431"; + case 451: + return "451"; case 500: return "500"; case 501: From 9f2d064d7c8d1db3969bfa77ddc44734bd3b34a4 Mon Sep 17 00:00:00 2001 From: Andreas Pohl Date: Sat, 19 Dec 2015 14:08:15 +0100 Subject: [PATCH 23/66] libnghttp2_asio: Optimized remote endpoint interface to const ref where possible --- src/asio_server_http2_handler.cc | 4 +++- src/asio_server_http2_handler.h | 2 +- src/asio_server_request.cc | 2 +- src/asio_server_request_impl.cc | 2 +- src/asio_server_request_impl.h | 2 +- src/includes/nghttp2/asio_http2_server.h | 2 +- 6 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/asio_server_http2_handler.cc b/src/asio_server_http2_handler.cc index bf594f87..fced5248 100644 --- a/src/asio_server_http2_handler.cc +++ b/src/asio_server_http2_handler.cc @@ -453,7 +453,9 @@ response *http2_handler::push_promise(boost::system::error_code &ec, boost::asio::io_service &http2_handler::io_service() { return io_service_; } -boost::asio::ip::tcp::endpoint http2_handler::remote_endpoint() { return remote_ep_; } +const boost::asio::ip::tcp::endpoint &http2_handler::remote_endpoint() { + return remote_ep_; +} callback_guard::callback_guard(http2_handler &h) : handler(h) { handler.enter_callback(); diff --git a/src/asio_server_http2_handler.h b/src/asio_server_http2_handler.h index 19dcfb77..a907e78f 100644 --- a/src/asio_server_http2_handler.h +++ b/src/asio_server_http2_handler.h @@ -90,7 +90,7 @@ public: boost::asio::io_service &io_service(); - boost::asio::ip::tcp::endpoint remote_endpoint(); + const boost::asio::ip::tcp::endpoint &remote_endpoint(); const std::string &http_date(); diff --git a/src/asio_server_request.cc b/src/asio_server_request.cc index 8237ff76..9612363b 100644 --- a/src/asio_server_request.cc +++ b/src/asio_server_request.cc @@ -50,7 +50,7 @@ void request::on_data(data_cb cb) const { request_impl &request::impl() const { return *impl_; } -boost::asio::ip::tcp::endpoint request::remote_endpoint() const { +const boost::asio::ip::tcp::endpoint &request::remote_endpoint() const { return impl_->remote_endpoint(); } diff --git a/src/asio_server_request_impl.cc b/src/asio_server_request_impl.cc index a7ab88c7..915b03e8 100644 --- a/src/asio_server_request_impl.cc +++ b/src/asio_server_request_impl.cc @@ -54,7 +54,7 @@ void request_impl::call_on_data(const uint8_t *data, std::size_t len) { } } -boost::asio::ip::tcp::endpoint request_impl::remote_endpoint() const { +const boost::asio::ip::tcp::endpoint &request_impl::remote_endpoint() const { return remote_ep_; } diff --git a/src/asio_server_request_impl.h b/src/asio_server_request_impl.h index 087b65ac..b4a37ff1 100644 --- a/src/asio_server_request_impl.h +++ b/src/asio_server_request_impl.h @@ -55,7 +55,7 @@ public: void stream(class stream *s); void call_on_data(const uint8_t *data, std::size_t len); - boost::asio::ip::tcp::endpoint remote_endpoint() const; + const boost::asio::ip::tcp::endpoint &remote_endpoint() const; void remote_endpoint(boost::asio::ip::tcp::endpoint ep); private: diff --git a/src/includes/nghttp2/asio_http2_server.h b/src/includes/nghttp2/asio_http2_server.h index f98c6c79..94bdc911 100644 --- a/src/includes/nghttp2/asio_http2_server.h +++ b/src/includes/nghttp2/asio_http2_server.h @@ -60,7 +60,7 @@ public: request_impl &impl() const; // Returns the remote endpoint of the request - boost::asio::ip::tcp::endpoint remote_endpoint() const; + const boost::asio::ip::tcp::endpoint &remote_endpoint() const; private: std::unique_ptr impl_; From 5a2d75551d01747f524cbb6cb44a498d9e3db61e Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 19 Dec 2015 22:56:10 +0900 Subject: [PATCH 24/66] h2load: Remove "(client)" from per-client req/s stat for simplicity --- doc/h2load.h2r | 2 +- src/h2load.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/h2load.h2r b/doc/h2load.h2r index 8cabe533..f06f7e01 100644 --- a/doc/h2load.h2r +++ b/doc/h2load.h2r @@ -88,7 +88,7 @@ time for 1st byte (of (decrypted in case of TLS) application data) deviation range (mean +/- sd) against total number of successful connections. -req/s (client) +req/s min The minimum request per second among all clients. max diff --git a/src/h2load.cc b/src/h2load.cc index 3f642eb6..1b8bd343 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -2253,7 +2253,7 @@ time for request: )" << std::setw(10) << util::format_duration(ts.request.min) << util::format_duration(ts.ttfb.mean) << " " << std::setw(10) << util::format_duration(ts.ttfb.sd) << std::setw(9) << util::dtos(ts.ttfb.within_sd) << "%" - << "\nreq/s (client) : " << std::setw(10) << ts.rps.min << " " + << "\nreq/s : " << std::setw(10) << ts.rps.min << " " << std::setw(10) << ts.rps.max << " " << std::setw(10) << ts.rps.mean << " " << std::setw(10) << ts.rps.sd << std::setw(9) << util::dtos(ts.rps.within_sd) << "%" << std::endl; From dd93b293973743852c1075896324e58902ebc173 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 20 Dec 2015 12:48:39 +0900 Subject: [PATCH 25/66] clang-format --- src/asio_io_service_pool.cc | 3 ++- src/asio_io_service_pool.h | 3 ++- src/asio_server.cc | 3 ++- src/asio_server.h | 3 ++- src/asio_server_http2.cc | 3 ++- src/asio_server_http2_handler.cc | 2 +- src/asio_server_http2_impl.cc | 3 ++- src/asio_server_http2_impl.h | 3 ++- src/includes/nghttp2/asio_http2_server.h | 3 ++- tests/nghttp2_session_test.c | 3 +-- 10 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/asio_io_service_pool.cc b/src/asio_io_service_pool.cc index 916f6d1e..1f6c1353 100644 --- a/src/asio_io_service_pool.cc +++ b/src/asio_io_service_pool.cc @@ -92,7 +92,8 @@ boost::asio::io_service &io_service_pool::get_io_service() { return io_service; } -const std::vector> &io_service_pool::get_io_services() const { +const std::vector> & +io_service_pool::get_io_services() const { return io_services_; } diff --git a/src/asio_io_service_pool.h b/src/asio_io_service_pool.h index 9c29619d..e7aaa049 100644 --- a/src/asio_io_service_pool.h +++ b/src/asio_io_service_pool.h @@ -71,7 +71,8 @@ public: boost::asio::io_service &get_io_service(); /// Get access to all io_service objects. - const std::vector> &get_io_services() const; + const std::vector> & + get_io_services() const; private: /// The pool of io_services. diff --git a/src/asio_server.cc b/src/asio_server.cc index ff9bb3b5..a332d091 100644 --- a/src/asio_server.cc +++ b/src/asio_server.cc @@ -169,7 +169,8 @@ void server::stop() { io_service_pool_.stop(); } void server::join() { io_service_pool_.join(); } -const std::vector> &server::get_io_services() const { +const std::vector> & +server::get_io_services() const { return io_service_pool_.get_io_services(); } diff --git a/src/asio_server.h b/src/asio_server.h index 433f6530..49f99446 100644 --- a/src/asio_server.h +++ b/src/asio_server.h @@ -74,7 +74,8 @@ public: void stop(); /// Get access to all io_service objects. - const std::vector> &get_io_services() const; + const std::vector> & + get_io_services() const; private: /// Initiate an asynchronous accept operation. diff --git a/src/asio_server_http2.cc b/src/asio_server_http2.cc index ca270a25..2cc3495f 100644 --- a/src/asio_server_http2.cc +++ b/src/asio_server_http2.cc @@ -77,7 +77,8 @@ void http2::stop() { impl_->stop(); } void http2::join() { return impl_->join(); } -const std::vector> &http2::get_io_services() const { +const std::vector> & +http2::get_io_services() const { return impl_->get_io_services(); } diff --git a/src/asio_server_http2_handler.cc b/src/asio_server_http2_handler.cc index fced5248..48e0bd14 100644 --- a/src/asio_server_http2_handler.cc +++ b/src/asio_server_http2_handler.cc @@ -136,7 +136,7 @@ int on_frame_recv_callback(nghttp2_session *session, const nghttp2_frame *frame, break; } - auto& req = strm->request().impl(); + auto &req = strm->request().impl(); req.remote_endpoint(handler->remote_endpoint()); handler->call_on_request(*strm); diff --git a/src/asio_server_http2_impl.cc b/src/asio_server_http2_impl.cc index 62c0df29..db1e0105 100644 --- a/src/asio_server_http2_impl.cc +++ b/src/asio_server_http2_impl.cc @@ -59,7 +59,8 @@ void http2_impl::stop() { return server_->stop(); } void http2_impl::join() { return server_->join(); } -const std::vector> &http2_impl::get_io_services() const { +const std::vector> & +http2_impl::get_io_services() const { return server_->get_io_services(); } diff --git a/src/asio_server_http2_impl.h b/src/asio_server_http2_impl.h index 395310bd..21c2e924 100644 --- a/src/asio_server_http2_impl.h +++ b/src/asio_server_http2_impl.h @@ -50,7 +50,8 @@ public: bool handle(std::string pattern, request_cb cb); void stop(); void join(); - const std::vector> &get_io_services() const; + const std::vector> & + get_io_services() const; private: std::unique_ptr server_; diff --git a/src/includes/nghttp2/asio_http2_server.h b/src/includes/nghttp2/asio_http2_server.h index 94bdc911..7a580081 100644 --- a/src/includes/nghttp2/asio_http2_server.h +++ b/src/includes/nghttp2/asio_http2_server.h @@ -205,7 +205,8 @@ public: void join(); // Get access to the io_service objects. - const std::vector> &get_io_services() const; + const std::vector> & + get_io_services() const; private: std::unique_ptr impl_; diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 4902625a..8e53aff1 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -1449,8 +1449,7 @@ void test_nghttp2_session_server_recv_push_response(void) { nghttp2_hd_deflate_init(&deflater, mem); nghttp2_session_open_stream(session, 2, NGHTTP2_STREAM_FLAG_NONE, - &pri_spec_default, - NGHTTP2_STREAM_RESERVED, NULL); + &pri_spec_default, NGHTTP2_STREAM_RESERVED, NULL); nvlen = ARRLEN(resnv); nghttp2_nv_array_copy(&nva, resnv, nvlen, mem); From 9f0083309e469fa4802ff226af0b24ae02ff2407 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 20 Dec 2015 13:19:29 +0900 Subject: [PATCH 26/66] Fix strange use of session_detect_idle_stream --- lib/nghttp2_session.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 0e32fc18..97cc2553 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3429,6 +3429,8 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, return NGHTTP2_ERR_IGN_HEADER_BLOCK; } + assert(session->server); + if (!session_is_new_peer_stream_id(session, frame->hd.stream_id)) { /* The spec says if an endpoint receives a HEADERS with invalid stream ID, it MUST issue connection error with error code @@ -3437,7 +3439,8 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, Then connection error is too harsh. It means that we only use connection error if stream ID refers idle stream. OTherwise we just ignore HEADERS for now. */ - if (session_detect_idle_stream(session, frame->hd.stream_id)) { + if (frame->hd.stream_id == 0 || + nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) { return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "request HEADERS: invalid stream_id"); From 19146211d7bebb0ee016535c5a5be18c7ff4c4b4 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 20 Dec 2015 13:20:21 +0900 Subject: [PATCH 27/66] Update doc --- lib/nghttp2_session.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 97cc2553..04a24a14 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3437,7 +3437,7 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, PROTOCOL_ERROR. But we could get trailer HEADERS after we have sent RST_STREAM to this stream and peer have not received it. Then connection error is too harsh. It means that we only use - connection error if stream ID refers idle stream. OTherwise we + connection error if stream ID refers idle stream. Therwise we just ignore HEADERS for now. */ if (frame->hd.stream_id == 0 || nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) { From 6c1a76af6e466361593a194c27c02ad1b999910c Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 20 Dec 2015 14:00:58 +0900 Subject: [PATCH 28/66] asio: Use std::move for remote_endpoint assignment --- src/asio_server_request_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/asio_server_request_impl.cc b/src/asio_server_request_impl.cc index 915b03e8..64866fa2 100644 --- a/src/asio_server_request_impl.cc +++ b/src/asio_server_request_impl.cc @@ -59,7 +59,7 @@ const boost::asio::ip::tcp::endpoint &request_impl::remote_endpoint() const { } void request_impl::remote_endpoint(boost::asio::ip::tcp::endpoint ep) { - remote_ep_ = ep; + remote_ep_ = std::move(ep); } } // namespace server From 010726a87582e3a7c01304f654d1acdbf5f87841 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 20 Dec 2015 23:20:14 +0900 Subject: [PATCH 29/66] Simplify error handling in nghttp2_session_on_headers_received return session_inflate_handle_invalid_stream(...) case is for streams for INITIAL state, but this is rare case. In general, we'd like to reduce RST_STREAM transmission, and it is suffice to ignore this frame for now. --- lib/nghttp2_session.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 04a24a14..0b3f8ec9 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3593,15 +3593,9 @@ int nghttp2_session_on_headers_received(nghttp2_session *session, return rv; } return 0; - } else if (stream->state == NGHTTP2_STREAM_CLOSING) { - /* This is race condition. NGHTTP2_STREAM_CLOSING indicates - that we queued RST_STREAM but it has not been sent. It will - eventually sent, so we just ignore this frame. */ - return NGHTTP2_ERR_IGN_HEADER_BLOCK; - } else { - return session_inflate_handle_invalid_stream(session, frame, - NGHTTP2_ERR_PROTO); } + + return NGHTTP2_ERR_IGN_HEADER_BLOCK; } /* If this is remote peer initiated stream, it is OK unless it has sent END_STREAM frame already. But if stream is in From e957147249b4b843f008201089023e6bbc3c75f0 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 20 Dec 2015 23:29:24 +0900 Subject: [PATCH 30/66] Make obvious implementation error connection error --- lib/nghttp2_session.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 0b3f8ec9..67248904 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3509,9 +3509,11 @@ int nghttp2_session_on_response_headers_received(nghttp2_session *session, If an endpoint receives additional frames for a stream that is in this state it MUST respond with a stream error (Section 5.4.2) of type STREAM_CLOSED. + + We go further, and make it connection error. */ - return session_inflate_handle_invalid_stream(session, frame, - NGHTTP2_ERR_STREAM_CLOSED); + return session_inflate_handle_invalid_connection( + session, frame, NGHTTP2_ERR_STREAM_CLOSED, "HEADERS: stream closed"); } stream->state = NGHTTP2_STREAM_OPENED; rv = session_call_on_begin_headers(session, frame); @@ -3582,9 +3584,11 @@ int nghttp2_session_on_headers_received(nghttp2_session *session, If an endpoint receives additional frames for a stream that is in this state it MUST respond with a stream error (Section 5.4.2) of type STREAM_CLOSED. + + we go further, and make it connection error. */ - return session_inflate_handle_invalid_stream(session, frame, - NGHTTP2_ERR_STREAM_CLOSED); + return session_inflate_handle_invalid_connection( + session, frame, NGHTTP2_ERR_STREAM_CLOSED, "HEADERS: stream closed"); } if (nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) { if (stream->state == NGHTTP2_STREAM_OPENED) { @@ -4304,8 +4308,9 @@ session_on_connection_window_update_received(nghttp2_session *session, nghttp2_frame *frame) { /* Handle connection-level flow control */ if (frame->window_update.window_size_increment == 0) { - return session_handle_invalid_connection(session, frame, NGHTTP2_ERR_PROTO, - NULL); + return session_handle_invalid_connection( + session, frame, NGHTTP2_ERR_PROTO, + "WINDOW_UPDATE: window_size_increment == 0"); } if (NGHTTP2_MAX_WINDOW_SIZE - frame->window_update.window_size_increment < @@ -4335,7 +4340,9 @@ static int session_on_stream_window_update_received(nghttp2_session *session, session, frame, NGHTTP2_ERR_PROTO, "WINDOW_UPADATE to reserved stream"); } if (frame->window_update.window_size_increment == 0) { - return session_handle_invalid_stream(session, frame, NGHTTP2_ERR_PROTO); + return session_handle_invalid_connection( + session, frame, NGHTTP2_ERR_PROTO, + "WINDOW_UPDATE: window_size_increment == 0"); } if (NGHTTP2_MAX_WINDOW_SIZE - frame->window_update.window_size_increment < stream->remote_window_size) { From cb73ba948d1edb35916500c7023a8d76558465fd Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 20 Dec 2015 23:47:16 +0900 Subject: [PATCH 31/66] Simplify HEADERS handling; handle push response in one function --- lib/nghttp2_session.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 67248904..c22216d2 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3534,6 +3534,12 @@ int nghttp2_session_on_push_response_headers_received(nghttp2_session *session, "push response HEADERS: stream_id == 0"); } + if (session->server) { + return session_inflate_handle_invalid_connection( + session, frame, NGHTTP2_ERR_PROTO, + "HEADERS: no HEADERS allowed from client in reserved state"); + } + if (session_is_incoming_concurrent_streams_max(session)) { return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, @@ -3570,14 +3576,6 @@ int nghttp2_session_on_headers_received(nghttp2_session *session, return session_inflate_handle_invalid_connection( session, frame, NGHTTP2_ERR_PROTO, "HEADERS: stream_id == 0"); } - if (stream->state == NGHTTP2_STREAM_RESERVED) { - /* reserved. The valid push response HEADERS is processed by - nghttp2_session_on_push_response_headers_received(). This - generic HEADERS is called invalid cases for HEADERS against - reserved state. */ - return session_inflate_handle_invalid_connection( - session, frame, NGHTTP2_ERR_PROTO, "HEADERS: stream in reserved"); - } if ((stream->shut_flags & NGHTTP2_SHUT_RD)) { /* half closed (remote): from the spec: @@ -3634,22 +3632,18 @@ static int session_process_headers_frame(nghttp2_session *session) { return nghttp2_session_on_request_headers_received(session, frame); } - if (nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) { - if (stream->state == NGHTTP2_STREAM_OPENING) { - frame->headers.cat = NGHTTP2_HCAT_RESPONSE; - return nghttp2_session_on_response_headers_received(session, frame, - stream); - } - /* This handles the case when server received HEADERS for stream - in reserved stream from client. That is protocol error. */ - frame->headers.cat = NGHTTP2_HCAT_HEADERS; - return nghttp2_session_on_headers_received(session, frame, stream); - } if (stream->state == NGHTTP2_STREAM_RESERVED) { frame->headers.cat = NGHTTP2_HCAT_PUSH_RESPONSE; return nghttp2_session_on_push_response_headers_received(session, frame, stream); } + + if (stream->state == NGHTTP2_STREAM_OPENING && + nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) { + frame->headers.cat = NGHTTP2_HCAT_RESPONSE; + return nghttp2_session_on_response_headers_received(session, frame, stream); + } + frame->headers.cat = NGHTTP2_HCAT_HEADERS; return nghttp2_session_on_headers_received(session, frame, stream); } From ca4a40b8e07b7cf650bcb504bf8c49df572b4ad4 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 21 Dec 2015 21:33:58 +0900 Subject: [PATCH 32/66] Don't schedule response HEADERS with priority tree Previously we scheduled the transmission of response HEADERS using priority tree in the belief that it allows more better utilization of bandwidth for prioritized streams. But to reduce the overhead of reconstruction of priority queue when connection level flow control window is depleted, we just don't check priority tree in this case. This means that response HEADERS frames are not sent even though they are not flow controlled. This could waste bandwidth. To improve this situation, we stop scheduling response HEADERS with priority tree for now. Now they are just sent in the order they submitted. The response body DATA continued to be scheduled with priority tree as before. --- lib/nghttp2_outbound_item.h | 3 --- lib/nghttp2_session.c | 14 -------------- lib/nghttp2_submit.c | 20 ++++++++------------ 3 files changed, 8 insertions(+), 29 deletions(-) diff --git a/lib/nghttp2_outbound_item.h b/lib/nghttp2_outbound_item.h index c16d5dfe..f1daeb1c 100644 --- a/lib/nghttp2_outbound_item.h +++ b/lib/nghttp2_outbound_item.h @@ -43,9 +43,6 @@ typedef struct { /* nonzero if request HEADERS is canceled. The error code is stored in |error_code|. */ uint8_t canceled; - /* nonzero if this item should be attached to stream object to make - it under priority control */ - uint8_t attach_stream; } nghttp2_headers_aux_data; /* struct used for DATA frame */ diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index c22216d2..d9baa253 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -718,20 +718,6 @@ int nghttp2_session_add_item(nghttp2_session *session, break; } - if (stream && item->aux_data.headers.attach_stream) { - if (stream->item) { - return NGHTTP2_ERR_DATA_EXIST; - } - - rv = nghttp2_stream_attach_item(stream, item); - - if (rv != 0) { - return rv; - } - - break; - } - nghttp2_outbound_queue_push(&session->ob_reg, item); item->queued = 1; break; diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index 65226363..2800b60e 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -40,8 +40,7 @@ static int32_t submit_headers_shared(nghttp2_session *session, uint8_t flags, const nghttp2_priority_spec *pri_spec, nghttp2_nv *nva_copy, size_t nvlen, const nghttp2_data_provider *data_prd, - void *stream_user_data, - uint8_t attach_stream) { + void *stream_user_data) { int rv; uint8_t flags_copy; nghttp2_outbound_item *item = NULL; @@ -69,7 +68,6 @@ static int32_t submit_headers_shared(nghttp2_session *session, uint8_t flags, } item->aux_data.headers.stream_user_data = stream_user_data; - item->aux_data.headers.attach_stream = attach_stream; flags_copy = (uint8_t)((flags & (NGHTTP2_FLAG_END_STREAM | NGHTTP2_FLAG_PRIORITY)) | @@ -122,8 +120,7 @@ static int32_t submit_headers_shared_nva(nghttp2_session *session, const nghttp2_priority_spec *pri_spec, const nghttp2_nv *nva, size_t nvlen, const nghttp2_data_provider *data_prd, - void *stream_user_data, - uint8_t attach_stream) { + void *stream_user_data) { int rv; nghttp2_nv *nva_copy; nghttp2_priority_spec copy_pri_spec; @@ -144,15 +141,14 @@ static int32_t submit_headers_shared_nva(nghttp2_session *session, } return submit_headers_shared(session, flags, stream_id, ©_pri_spec, - nva_copy, nvlen, data_prd, stream_user_data, - attach_stream); + nva_copy, nvlen, data_prd, stream_user_data); } int nghttp2_submit_trailer(nghttp2_session *session, int32_t stream_id, const nghttp2_nv *nva, size_t nvlen) { return (int)submit_headers_shared_nva(session, NGHTTP2_FLAG_END_STREAM, - stream_id, NULL, nva, nvlen, NULL, NULL, - 0); + stream_id, NULL, nva, nvlen, NULL, + NULL); } int32_t nghttp2_submit_headers(nghttp2_session *session, uint8_t flags, @@ -169,7 +165,7 @@ int32_t nghttp2_submit_headers(nghttp2_session *session, uint8_t flags, } return submit_headers_shared_nva(session, flags, stream_id, pri_spec, nva, - nvlen, NULL, stream_user_data, 0); + nvlen, NULL, stream_user_data); } int nghttp2_submit_ping(nghttp2_session *session, uint8_t flags _U_, @@ -398,7 +394,7 @@ int32_t nghttp2_submit_request(nghttp2_session *session, flags = set_request_flags(pri_spec, data_prd); return submit_headers_shared_nva(session, flags, -1, pri_spec, nva, nvlen, - data_prd, stream_user_data, 0); + data_prd, stream_user_data); } static uint8_t set_response_flags(const nghttp2_data_provider *data_prd) { @@ -414,7 +410,7 @@ int nghttp2_submit_response(nghttp2_session *session, int32_t stream_id, const nghttp2_data_provider *data_prd) { uint8_t flags = set_response_flags(data_prd); return submit_headers_shared_nva(session, flags, stream_id, NULL, nva, nvlen, - data_prd, NULL, 1); + data_prd, NULL); } int nghttp2_submit_data(nghttp2_session *session, uint8_t flags, From 09bd9c94a3fdb6e74763f942b4bbec0459b6bc2b Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 22 Dec 2015 00:40:22 +0900 Subject: [PATCH 33/66] asio: client: Should call shutdown_socket() on read error --- src/asio_client_session_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/asio_client_session_impl.cc b/src/asio_client_session_impl.cc index 3be7b20d..4aeee74d 100644 --- a/src/asio_client_session_impl.cc +++ b/src/asio_client_session_impl.cc @@ -527,8 +527,9 @@ void session_impl::do_read() { if (ec) { if (!should_stop()) { call_error_cb(ec); - shutdown_socket(); } + shutdown_socket(); + return; } From 1ee1122d40a9ce8adebd62f81d33cc60e153ae89 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 22 Dec 2015 00:33:12 +0900 Subject: [PATCH 34/66] asio: Add configurable tls handshake/read timeout to server --- src/asio_server.cc | 18 +++- src/asio_server.h | 7 +- src/asio_server_connection.h | 107 +++++++++++++++++++---- src/asio_server_http2.cc | 8 ++ src/asio_server_http2_impl.cc | 17 +++- src/asio_server_http2_impl.h | 4 + src/includes/nghttp2/asio_http2_server.h | 6 ++ 7 files changed, 141 insertions(+), 26 deletions(-) diff --git a/src/asio_server.cc b/src/asio_server.cc index a332d091..002c947d 100644 --- a/src/asio_server.cc +++ b/src/asio_server.cc @@ -44,8 +44,12 @@ namespace nghttp2 { namespace asio_http2 { namespace server { -server::server(std::size_t io_service_pool_size) - : io_service_pool_(io_service_pool_size) {} +server::server(std::size_t io_service_pool_size, + const boost::posix_time::time_duration &tls_handshake_timeout, + const boost::posix_time::time_duration &read_timeout) + : io_service_pool_(io_service_pool_size), + tls_handshake_timeout_(tls_handshake_timeout), + read_timeout_(read_timeout) {} boost::system::error_code server::listen_and_serve(boost::system::error_code &ec, @@ -121,7 +125,8 @@ boost::system::error_code server::bind_and_listen(boost::system::error_code &ec, void server::start_accept(boost::asio::ssl::context &tls_context, tcp::acceptor &acceptor, serve_mux &mux) { auto new_connection = std::make_shared>( - mux, io_service_pool_.get_io_service(), tls_context); + mux, tls_handshake_timeout_, read_timeout_, + io_service_pool_.get_io_service(), tls_context); acceptor.async_accept( new_connection->socket().lowest_layer(), @@ -130,14 +135,17 @@ void server::start_accept(boost::asio::ssl::context &tls_context, if (!e) { new_connection->socket().lowest_layer().set_option( tcp::no_delay(true)); + new_connection->start_tls_handshake_deadline(); new_connection->socket().async_handshake( boost::asio::ssl::stream_base::server, [new_connection](const boost::system::error_code &e) { if (e) { + new_connection->stop(); return; } if (!tls_h2_negotiated(new_connection->socket())) { + new_connection->stop(); return; } @@ -151,13 +159,15 @@ void server::start_accept(boost::asio::ssl::context &tls_context, void server::start_accept(tcp::acceptor &acceptor, serve_mux &mux) { auto new_connection = std::make_shared>( - mux, io_service_pool_.get_io_service()); + mux, tls_handshake_timeout_, read_timeout_, + io_service_pool_.get_io_service()); acceptor.async_accept( new_connection->socket(), [this, &acceptor, &mux, new_connection]( const boost::system::error_code &e) { if (!e) { new_connection->socket().set_option(tcp::no_delay(true)); + new_connection->start_read_deadline(); new_connection->start(); } diff --git a/src/asio_server.h b/src/asio_server.h index 49f99446..e1c52792 100644 --- a/src/asio_server.h +++ b/src/asio_server.h @@ -63,7 +63,9 @@ using ssl_socket = boost::asio::ssl::stream; class server : private boost::noncopyable { public: - explicit server(std::size_t io_service_pool_size); + explicit server(std::size_t io_service_pool_size, + const boost::posix_time::time_duration &tls_handshake_timeout, + const boost::posix_time::time_duration &read_timeout); boost::system::error_code listen_and_serve(boost::system::error_code &ec, @@ -98,6 +100,9 @@ private: std::vector acceptors_; std::unique_ptr ssl_ctx_; + + boost::posix_time::time_duration tls_handshake_timeout_; + boost::posix_time::time_duration read_timeout_; }; } // namespace server diff --git a/src/asio_server_connection.h b/src/asio_server_connection.h index beac04f8..4cab44b4 100644 --- a/src/asio_server_connection.h +++ b/src/asio_server_connection.h @@ -64,9 +64,15 @@ class connection : public std::enable_shared_from_this>, public: /// Construct a connection with the given io_service. template - explicit connection(serve_mux &mux, SocketArgs &&... args) - : socket_(std::forward(args)...), mux_(mux), writing_(false) { - } + explicit connection( + serve_mux &mux, + const boost::posix_time::time_duration &tls_handshake_timeout, + const boost::posix_time::time_duration &read_timeout, + SocketArgs &&... args) + : socket_(std::forward(args)...), mux_(mux), + deadline_(socket_.get_io_service()), + tls_handshake_timeout_(tls_handshake_timeout), + read_timeout_(read_timeout), writing_(false), stopped_(false) {} /// Start the first asynchronous operation for the connection. void start() { @@ -74,6 +80,7 @@ public: socket_.get_io_service(), socket_.lowest_layer().remote_endpoint(), [this]() { do_write(); }, mux_); if (handler_->start() != 0) { + stop(); return; } do_read(); @@ -81,27 +88,62 @@ public: socket_type &socket() { return socket_; } + void start_tls_handshake_deadline() { + deadline_.expires_from_now(tls_handshake_timeout_); + deadline_.async_wait( + std::bind(&connection::handle_deadline, this->shared_from_this())); + } + + void start_read_deadline() { + deadline_.expires_from_now(read_timeout_); + deadline_.async_wait( + std::bind(&connection::handle_deadline, this->shared_from_this())); + } + + void handle_deadline() { + if (stopped_) { + return; + } + + if (deadline_.expires_at() <= + boost::asio::deadline_timer::traits_type::now()) { + stop(); + deadline_.expires_at(boost::posix_time::pos_infin); + return; + } + + deadline_.async_wait( + std::bind(&connection::handle_deadline, this->shared_from_this())); + } + void do_read() { auto self = this->shared_from_this(); + deadline_.expires_from_now(read_timeout_); + socket_.async_read_some( boost::asio::buffer(buffer_), [this, self](const boost::system::error_code &e, std::size_t bytes_transferred) { - if (!e) { - if (handler_->on_read(buffer_, bytes_transferred) != 0) { - return; - } - - do_write(); - - if (!writing_ && handler_->should_stop()) { - return; - } - - do_read(); + if (e) { + stop(); + return; } + if (handler_->on_read(buffer_, bytes_transferred) != 0) { + stop(); + return; + } + + do_write(); + + if (!writing_ && handler_->should_stop()) { + stop(); + return; + } + + do_read(); + // If an error occurs then no new asynchronous operations are // started. This means that all shared_ptr references to the // connection object will disappear and the object will be @@ -123,23 +165,34 @@ public: rv = handler_->on_write(outbuf_, nwrite); if (rv != 0) { + stop(); return; } if (nwrite == 0) { + if (handler_->should_stop()) { + stop(); + } return; } writing_ = true; + // Reset read deadline here, because normally client is sending + // something, it does not expect timeout while doing it. + deadline_.expires_from_now(read_timeout_); + boost::asio::async_write( socket_, boost::asio::buffer(outbuf_, nwrite), [this, self](const boost::system::error_code &e, std::size_t) { - if (!e) { - writing_ = false; - - do_write(); + if (e) { + stop(); + return; } + + writing_ = false; + + do_write(); }); // No new asynchronous operations are started. This means that all @@ -148,6 +201,17 @@ public: // returns. The connection class's destructor closes the socket. } + void stop() { + if (stopped_) { + return; + } + + stopped_ = true; + boost::system::error_code ignored_ec; + socket_.lowest_layer().close(ignored_ec); + deadline_.cancel(); + } + private: socket_type socket_; @@ -160,7 +224,12 @@ private: boost::array outbuf_; + boost::asio::deadline_timer deadline_; + boost::posix_time::time_duration tls_handshake_timeout_; + boost::posix_time::time_duration read_timeout_; + bool writing_; + bool stopped_; }; } // namespace server diff --git a/src/asio_server_http2.cc b/src/asio_server_http2.cc index 2cc3495f..5606d833 100644 --- a/src/asio_server_http2.cc +++ b/src/asio_server_http2.cc @@ -69,6 +69,14 @@ void http2::num_threads(size_t num_threads) { impl_->num_threads(num_threads); } void http2::backlog(int backlog) { impl_->backlog(backlog); } +void http2::tls_handshake_timeout(const boost::posix_time::time_duration &t) { + impl_->tls_handshake_timeout(t); +} + +void http2::read_timeout(const boost::posix_time::time_duration &t) { + impl_->read_timeout(t); +} + bool http2::handle(std::string pattern, request_cb cb) { return impl_->handle(std::move(pattern), std::move(cb)); } diff --git a/src/asio_server_http2_impl.cc b/src/asio_server_http2_impl.cc index db1e0105..5e420219 100644 --- a/src/asio_server_http2_impl.cc +++ b/src/asio_server_http2_impl.cc @@ -37,12 +37,16 @@ namespace asio_http2 { namespace server { -http2_impl::http2_impl() : num_threads_(1), backlog_(-1) {} +http2_impl::http2_impl() + : num_threads_(1), backlog_(-1), + tls_handshake_timeout_(boost::posix_time::seconds(60)), + read_timeout_(boost::posix_time::seconds(60)) {} boost::system::error_code http2_impl::listen_and_serve( boost::system::error_code &ec, boost::asio::ssl::context *tls_context, const std::string &address, const std::string &port, bool asynchronous) { - server_.reset(new server(num_threads_)); + server_.reset( + new server(num_threads_, tls_handshake_timeout_, read_timeout_)); return server_->listen_and_serve(ec, tls_context, address, port, backlog_, mux_, asynchronous); } @@ -51,6 +55,15 @@ void http2_impl::num_threads(size_t num_threads) { num_threads_ = num_threads; } void http2_impl::backlog(int backlog) { backlog_ = backlog; } +void http2_impl::tls_handshake_timeout( + const boost::posix_time::time_duration &t) { + tls_handshake_timeout_ = t; +} + +void http2_impl::read_timeout(const boost::posix_time::time_duration &t) { + read_timeout_ = t; +} + bool http2_impl::handle(std::string pattern, request_cb cb) { return mux_.handle(std::move(pattern), std::move(cb)); } diff --git a/src/asio_server_http2_impl.h b/src/asio_server_http2_impl.h index 21c2e924..a3b98552 100644 --- a/src/asio_server_http2_impl.h +++ b/src/asio_server_http2_impl.h @@ -47,6 +47,8 @@ public: const std::string &address, const std::string &port, bool asynchronous); void num_threads(size_t num_threads); void backlog(int backlog); + void tls_handshake_timeout(const boost::posix_time::time_duration &t); + void read_timeout(const boost::posix_time::time_duration &t); bool handle(std::string pattern, request_cb cb); void stop(); void join(); @@ -58,6 +60,8 @@ private: std::size_t num_threads_; int backlog_; serve_mux mux_; + boost::posix_time::time_duration tls_handshake_timeout_; + boost::posix_time::time_duration read_timeout_; }; } // namespace server diff --git a/src/includes/nghttp2/asio_http2_server.h b/src/includes/nghttp2/asio_http2_server.h index 7a580081..5b969593 100644 --- a/src/includes/nghttp2/asio_http2_server.h +++ b/src/includes/nghttp2/asio_http2_server.h @@ -198,6 +198,12 @@ public: // connections. void backlog(int backlog); + // Sets TLS handshake timeout, which defaults to 60 seconds. + void tls_handshake_timeout(const boost::posix_time::time_duration &t); + + // Sets read timeout, which defaults to 60 seconds. + void read_timeout(const boost::posix_time::time_duration &t); + // Gracefully stop http2 server void stop(); From 7c5ef0613dc9f540021ea97f4a53ce2841d0866f Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 21 Dec 2015 23:38:32 +0900 Subject: [PATCH 35/66] asio: Add configurable connect/read timeout for client This commit includes backward incompatible change, since we change private field in public API class. --- src/asio_client_session.cc | 16 ++- src/asio_client_session_impl.cc | 127 +++++++++++++++++++---- src/asio_client_session_impl.h | 13 ++- src/asio_client_session_tcp_impl.cc | 9 +- src/asio_client_session_tls_impl.cc | 5 +- src/includes/nghttp2/asio_http2_client.h | 8 +- 6 files changed, 146 insertions(+), 32 deletions(-) diff --git a/src/asio_client_session.cc b/src/asio_client_session.cc index aaee1e77..bc835c38 100644 --- a/src/asio_client_session.cc +++ b/src/asio_client_session.cc @@ -39,12 +39,16 @@ using boost::asio::ip::tcp; session::session(boost::asio::io_service &io_service, const std::string &host, const std::string &service) - : impl_(make_unique(io_service, host, service)) {} + : impl_(std::make_shared(io_service, host, service)) { + impl_->start_resolve(host, service); +} session::session(boost::asio::io_service &io_service, boost::asio::ssl::context &tls_ctx, const std::string &host, const std::string &service) - : impl_(make_unique(io_service, tls_ctx, host, service)) { + : impl_(std::make_shared(io_service, tls_ctx, host, + service)) { + impl_->start_resolve(host, service); } session::~session() {} @@ -93,6 +97,14 @@ const request *session::submit(boost::system::error_code &ec, return impl_->submit(ec, method, uri, std::move(cb), std::move(h)); } +void session::connect_timeout(const boost::posix_time::time_duration &t) { + impl_->connect_timeout(t); +} + +void session::read_timeout(const boost::posix_time::time_duration &t) { + impl_->read_timeout(t); +} + } // namespace client } // namespace asio_http2 } // nghttp2 diff --git a/src/asio_client_session_impl.cc b/src/asio_client_session_impl.cc index 4aeee74d..a4cce25a 100644 --- a/src/asio_client_session_impl.cc +++ b/src/asio_client_session_impl.cc @@ -40,8 +40,10 @@ namespace client { session_impl::session_impl(boost::asio::io_service &io_service) : wblen_(0), io_service_(io_service), resolver_(io_service), - session_(nullptr), data_pending_(nullptr), data_pendinglen_(0), - writing_(false), inside_callback_(false) {} + deadline_(io_service), connect_timeout_(boost::posix_time::seconds(60)), + read_timeout_(boost::posix_time::seconds(60)), session_(nullptr), + data_pending_(nullptr), data_pendinglen_(0), writing_(false), + inside_callback_(false), stopped_(false) {} session_impl::~session_impl() { // finish up all active stream @@ -56,9 +58,13 @@ session_impl::~session_impl() { void session_impl::start_resolve(const std::string &host, const std::string &service) { + deadline_.expires_from_now(connect_timeout_); + + auto self = this->shared_from_this(); + resolver_.async_resolve({host, service}, - [this](const boost::system::error_code &ec, - tcp::resolver::iterator endpoint_it) { + [this, self](const boost::system::error_code &ec, + tcp::resolver::iterator endpoint_it) { if (ec) { not_connected(ec); return; @@ -66,6 +72,25 @@ void session_impl::start_resolve(const std::string &host, start_connect(endpoint_it); }); + + deadline_.async_wait(std::bind(&session_impl::handle_deadline, self)); +} + +void session_impl::handle_deadline() { + if (stopped_) { + return; + } + + if (deadline_.expires_at() <= + boost::asio::deadline_timer::traits_type::now()) { + call_error_cb(boost::asio::error::timed_out); + stop(); + deadline_.expires_at(boost::posix_time::pos_infin); + return; + } + + deadline_.async_wait( + std::bind(&session_impl::handle_deadline, this->shared_from_this())); } void session_impl::connected(tcp::resolver::iterator endpoint_it) { @@ -86,6 +111,7 @@ void session_impl::connected(tcp::resolver::iterator endpoint_it) { void session_impl::not_connected(const boost::system::error_code &ec) { call_error_cb(ec); + stop(); } void session_impl::on_connect(connect_cb cb) { connect_cb_ = std::move(cb); } @@ -97,6 +123,9 @@ const connect_cb &session_impl::on_connect() const { return connect_cb_; } const error_cb &session_impl::on_error() const { return error_cb_; } void session_impl::call_error_cb(const boost::system::error_code &ec) { + if (stopped_) { + return; + } auto &error_cb = on_error(); if (!error_cb) { return; @@ -350,12 +379,20 @@ int session_impl::write_trailer(stream &strm, header_map h) { } void session_impl::cancel(stream &strm, uint32_t error_code) { + if (stopped_) { + return; + } + nghttp2_submit_rst_stream(session_, NGHTTP2_FLAG_NONE, strm.stream_id(), error_code); signal_write(); } void session_impl::resume(stream &strm) { + if (stopped_) { + return; + } + nghttp2_session_resume_data(session_, strm.stream_id()); signal_write(); } @@ -396,6 +433,11 @@ const request *session_impl::submit(boost::system::error_code &ec, header_map h) { ec.clear(); + if (stopped_) { + ec = make_error_code(static_cast(NGHTTP2_INTERNAL_ERROR)); + return nullptr; + } + http_parser_url u{}; // TODO Handle CONNECT method if (http_parser_parse_url(uri.c_str(), uri.size(), 0, &u) != 0) { @@ -485,6 +527,10 @@ const request *session_impl::submit(boost::system::error_code &ec, } void session_impl::shutdown() { + if (stopped_) { + return; + } + nghttp2_session_terminate_session(session_, NGHTTP2_NO_ERROR); signal_write(); } @@ -522,14 +568,21 @@ void session_impl::leave_callback() { } void session_impl::do_read() { - read_socket([this](const boost::system::error_code &ec, - std::size_t bytes_transferred) { + if (stopped_) { + return; + } + + deadline_.expires_from_now(read_timeout_); + + auto self = this->shared_from_this(); + + read_socket([this, self](const boost::system::error_code &ec, + std::size_t bytes_transferred) { if (ec) { if (!should_stop()) { call_error_cb(ec); } - shutdown_socket(); - + stop(); return; } @@ -542,7 +595,7 @@ void session_impl::do_read() { if (rv != static_cast(bytes_transferred)) { call_error_cb(make_error_code( static_cast(rv < 0 ? rv : NGHTTP2_ERR_PROTO))); - shutdown_socket(); + stop(); return; } } @@ -550,7 +603,7 @@ void session_impl::do_read() { do_write(); if (should_stop()) { - shutdown_socket(); + stop(); return; } @@ -559,6 +612,10 @@ void session_impl::do_read() { } void session_impl::do_write() { + if (stopped_) { + return; + } + if (writing_) { return; } @@ -580,7 +637,7 @@ void session_impl::do_write() { auto n = nghttp2_session_mem_send(session_, &data); if (n < 0) { call_error_cb(make_error_code(static_cast(n))); - shutdown_socket(); + stop(); return; } @@ -602,23 +659,51 @@ void session_impl::do_write() { } if (wblen_ == 0) { + if (should_stop()) { + stop(); + } return; } writing_ = true; - write_socket([this](const boost::system::error_code &ec, std::size_t n) { - if (ec) { - call_error_cb(ec); - shutdown_socket(); - return; - } + // Reset read deadline here, because normally client is sending + // something, it does not expect timeout while doing it. + deadline_.expires_from_now(read_timeout_); - wblen_ = 0; - writing_ = false; + auto self = this->shared_from_this(); - do_write(); - }); + write_socket( + [this, self](const boost::system::error_code &ec, std::size_t n) { + if (ec) { + call_error_cb(ec); + stop(); + return; + } + + wblen_ = 0; + writing_ = false; + + do_write(); + }); +} + +void session_impl::stop() { + if (stopped_) { + return; + } + + shutdown_socket(); + deadline_.cancel(); + stopped_ = true; +} + +void session_impl::connect_timeout(const boost::posix_time::time_duration &t) { + connect_timeout_ = t; +} + +void session_impl::read_timeout(const boost::posix_time::time_duration &t) { + read_timeout_ = t; } } // namespace client diff --git a/src/asio_client_session_impl.h b/src/asio_client_session_impl.h index 37032d68..5e936c37 100644 --- a/src/asio_client_session_impl.h +++ b/src/asio_client_session_impl.h @@ -41,7 +41,7 @@ class stream; using boost::asio::ip::tcp; -class session_impl { +class session_impl : public std::enable_shared_from_this { public: session_impl(boost::asio::io_service &io_service); virtual ~session_impl(); @@ -91,6 +91,11 @@ public: void do_read(); void do_write(); + void connect_timeout(const boost::posix_time::time_duration &t); + void read_timeout(const boost::posix_time::time_duration &t); + + void stop(); + protected: boost::array rb_; boost::array wb_; @@ -100,6 +105,7 @@ private: bool should_stop() const; bool setup_session(); void call_error_cb(const boost::system::error_code &ec); + void handle_deadline(); boost::asio::io_service &io_service_; tcp::resolver resolver_; @@ -109,6 +115,10 @@ private: connect_cb connect_cb_; error_cb error_cb_; + boost::asio::deadline_timer deadline_; + boost::posix_time::time_duration connect_timeout_; + boost::posix_time::time_duration read_timeout_; + nghttp2_session *session_; const uint8_t *data_pending_; @@ -116,6 +126,7 @@ private: bool writing_; bool inside_callback_; + bool stopped_; }; } // namespace client diff --git a/src/asio_client_session_tcp_impl.cc b/src/asio_client_session_tcp_impl.cc index c20a2303..478f1c16 100644 --- a/src/asio_client_session_tcp_impl.cc +++ b/src/asio_client_session_tcp_impl.cc @@ -31,9 +31,7 @@ namespace client { session_tcp_impl::session_tcp_impl(boost::asio::io_service &io_service, const std::string &host, const std::string &service) - : session_impl(io_service), socket_(io_service) { - start_resolve(host, service); -} + : session_impl(io_service), socket_(io_service) {} session_tcp_impl::~session_tcp_impl() {} @@ -62,7 +60,10 @@ void session_tcp_impl::write_socket( boost::asio::async_write(socket_, boost::asio::buffer(wb_, wblen_), h); } -void session_tcp_impl::shutdown_socket() { socket_.close(); } +void session_tcp_impl::shutdown_socket() { + boost::system::error_code ignored_ec; + socket_.close(ignored_ec); +} } // namespace client } // namespace asio_http2 diff --git a/src/asio_client_session_tls_impl.cc b/src/asio_client_session_tls_impl.cc index 8dc32d57..486c52d7 100644 --- a/src/asio_client_session_tls_impl.cc +++ b/src/asio_client_session_tls_impl.cc @@ -38,8 +38,6 @@ session_tls_impl::session_tls_impl(boost::asio::io_service &io_service, // ssl::context::set_verify_mode(boost::asio::ssl::verify_peer) is // not used, which is what we want. socket_.set_verify_callback(boost::asio::ssl::rfc2818_verification(host)); - - start_resolve(host, service); } session_tls_impl::~session_tls_impl() {} @@ -85,7 +83,8 @@ void session_tls_impl::write_socket( } void session_tls_impl::shutdown_socket() { - socket_.async_shutdown([](const boost::system::error_code &ec) {}); + boost::system::error_code ignored_ec; + socket_.lowest_layer().close(ignored_ec); } } // namespace client diff --git a/src/includes/nghttp2/asio_http2_client.h b/src/includes/nghttp2/asio_http2_client.h index a2b51ad7..55ef9f68 100644 --- a/src/includes/nghttp2/asio_http2_client.h +++ b/src/includes/nghttp2/asio_http2_client.h @@ -144,6 +144,12 @@ public: // and session is terminated. void on_error(error_cb cb) const; + // Sets connect timeout, which defaults to 60 seconds. + void connect_timeout(const boost::posix_time::time_duration &t); + + // Sets read timeout, which defaults to 60 seconds. + void read_timeout(const boost::posix_time::time_duration &t); + // Shutdowns connection. void shutdown() const; @@ -177,7 +183,7 @@ public: generator_cb cb, header_map h = header_map{}) const; private: - std::unique_ptr impl_; + std::shared_ptr impl_; }; // configure |tls_ctx| for client use. Currently, we just set NPN From 5de2c7a8c137f5d71c4a23533cd6bcd59920f554 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 14:21:31 +0900 Subject: [PATCH 36/66] Detect availability of initgroups --- configure.ac | 1 + src/shrpx_worker_process.cc | 2 ++ 2 files changed, 3 insertions(+) diff --git a/configure.ac b/configure.ac index d2aaa651..152a30e0 100644 --- a/configure.ac +++ b/configure.ac @@ -604,6 +604,7 @@ AC_CHECK_FUNCS([ \ dup2 \ getcwd \ getpwnam \ + initgroups \ localtime_r \ memchr \ memmove \ diff --git a/src/shrpx_worker_process.cc b/src/shrpx_worker_process.cc index c2358c58..04ff2698 100644 --- a/src/shrpx_worker_process.cc +++ b/src/shrpx_worker_process.cc @@ -64,12 +64,14 @@ void drop_privileges( #endif // HAVE_NEVERBLEED ) { if (getuid() == 0 && get_config()->uid != 0) { +#ifdef HAVE_INITGROUPS if (initgroups(get_config()->user.get(), get_config()->gid) != 0) { auto error = errno; LOG(FATAL) << "Could not change supplementary groups: " << strerror(error); exit(EXIT_FAILURE); } +#endif // HAVE_INITGROUPS if (setgid(get_config()->gid) != 0) { auto error = errno; LOG(FATAL) << "Could not change gid: " << strerror(error); From 92a56d034f201cbb609606184822cf1716677207 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 16:38:30 +0900 Subject: [PATCH 37/66] Fix bug that idle/closed stream may be destroyed while it is referenced --- lib/nghttp2_session.c | 133 ++++++++++++++++++++++------------- lib/nghttp2_session.h | 39 +++++----- tests/nghttp2_session_test.c | 26 ++++--- 3 files changed, 121 insertions(+), 77 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index d9baa253..f94a009a 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -753,6 +753,10 @@ int nghttp2_session_add_item(nghttp2_session *session, return NGHTTP2_ERR_NOMEM; } + /* We don't have to call nghttp2_session_adjust_closed_stream() + here, since stream->stream_id is local stream_id, and it does + not affect closed stream count. */ + nghttp2_outbound_queue_push(&session->ob_reg, item); item->queued = 1; @@ -884,15 +888,6 @@ nghttp2_stream *nghttp2_session_open_stream(nghttp2_session *session, return NULL; } } else { - if (session->server && initial_state != NGHTTP2_STREAM_IDLE && - !nghttp2_session_is_my_stream_id(session, stream_id)) { - - rv = nghttp2_session_adjust_closed_stream(session, 1); - if (rv != 0) { - return NULL; - } - } - stream = nghttp2_mem_malloc(mem, sizeof(nghttp2_stream)); if (stream == NULL) { return NULL; @@ -970,10 +965,7 @@ nghttp2_stream *nghttp2_session_open_stream(nghttp2_session *session, /* Idle stream does not count toward the concurrent streams limit. This is used as anchor node in dependency tree. */ assert(session->server); - rv = nghttp2_session_keep_idle_stream(session, stream); - if (rv != 0) { - return NULL; - } + nghttp2_session_keep_idle_stream(session, stream); break; default: if (nghttp2_session_is_my_stream_id(session, stream_id)) { @@ -1078,7 +1070,9 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id, /* On server side, retain stream at most MAX_CONCURRENT_STREAMS combined with the current active incoming streams to make dependency tree work better. */ - rv = nghttp2_session_keep_closed_stream(session, stream); + nghttp2_session_keep_closed_stream(session, stream); + + rv = nghttp2_session_adjust_closed_stream(session); } else { rv = nghttp2_session_destroy_stream(session, stream); } @@ -1113,10 +1107,8 @@ int nghttp2_session_destroy_stream(nghttp2_session *session, return 0; } -int nghttp2_session_keep_closed_stream(nghttp2_session *session, - nghttp2_stream *stream) { - int rv; - +void nghttp2_session_keep_closed_stream(nghttp2_session *session, + nghttp2_stream *stream) { DEBUGF(fprintf(stderr, "stream: keep closed stream(%p)=%d, state=%d\n", stream, stream->stream_id, stream->state)); @@ -1129,19 +1121,10 @@ int nghttp2_session_keep_closed_stream(nghttp2_session *session, session->closed_stream_tail = stream; ++session->num_closed_streams; - - rv = nghttp2_session_adjust_closed_stream(session, 0); - if (rv != 0) { - return rv; - } - - return 0; } -int nghttp2_session_keep_idle_stream(nghttp2_session *session, - nghttp2_stream *stream) { - int rv; - +void nghttp2_session_keep_idle_stream(nghttp2_session *session, + nghttp2_stream *stream) { DEBUGF(fprintf(stderr, "stream: keep idle stream(%p)=%d, state=%d\n", stream, stream->stream_id, stream->state)); @@ -1154,13 +1137,6 @@ int nghttp2_session_keep_idle_stream(nghttp2_session *session, session->idle_stream_tail = stream; ++session->num_idle_streams; - - rv = nghttp2_session_adjust_idle_stream(session); - if (rv != 0) { - return rv; - } - - return 0; } void nghttp2_session_detach_idle_stream(nghttp2_session *session, @@ -1191,8 +1167,7 @@ void nghttp2_session_detach_idle_stream(nghttp2_session *session, --session->num_idle_streams; } -int nghttp2_session_adjust_closed_stream(nghttp2_session *session, - size_t offset) { +int nghttp2_session_adjust_closed_stream(nghttp2_session *session) { size_t num_stream_max; int rv; @@ -1206,7 +1181,7 @@ int nghttp2_session_adjust_closed_stream(nghttp2_session *session, num_stream_max)); while (session->num_closed_streams > 0 && - session->num_closed_streams + session->num_incoming_streams + offset > + session->num_closed_streams + session->num_incoming_streams > num_stream_max) { nghttp2_stream *head_stream; nghttp2_stream *next; @@ -1242,13 +1217,11 @@ int nghttp2_session_adjust_idle_stream(nghttp2_session *session) { size_t max; int rv; - /* Make minimum number of idle streams 2 so that allocating 2 - streams at once is easy. This happens when PRIORITY frame to - idle stream, which depends on idle stream which does not - exist. */ - max = - nghttp2_max(2, nghttp2_min(session->local_settings.max_concurrent_streams, - session->pending_local_max_concurrent_stream)); + /* Make minimum number of idle streams 16, which is arbitrary chosen + number. */ + max = nghttp2_max(16, + nghttp2_min(session->local_settings.max_concurrent_streams, + session->pending_local_max_concurrent_stream)); DEBUGF(fprintf(stderr, "stream: adjusting kept idle streams " "num_idle_streams=%zu, max=%zu\n", @@ -1799,6 +1772,9 @@ static int session_prep_frame(nghttp2_session *session, return NGHTTP2_ERR_NOMEM; } + /* We don't call nghttp2_session_adjust_closed_stream() here, + since we don't keep closed stream in client side */ + estimated_payloadlen = session_estimate_headers_payload( session, frame->headers.nva, frame->headers.nvlen, NGHTTP2_PRIORITY_SPECLEN); @@ -2391,6 +2367,12 @@ static int session_after_frame_sent1(nghttp2_session *session) { return rv; } + rv = nghttp2_session_adjust_idle_stream(session); + + if (nghttp2_is_fatal(rv)) { + return rv; + } + break; } case NGHTTP2_RST_STREAM: @@ -2659,6 +2641,14 @@ static ssize_t nghttp2_session_mem_send_internal(nghttp2_session *session, aob = &session->aob; framebufs = &aob->framebufs; + /* We may have idle streams more than we expect (e.g., + nghttp2_session_change_stream_priority() or + nghttp2_session_create_idle_stream()). Adjust them here. */ + rv = nghttp2_session_adjust_idle_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } + *data_ptr = NULL; for (;;) { switch (aob->state) { @@ -3469,7 +3459,14 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, if (!stream) { return NGHTTP2_ERR_NOMEM; } + + rv = nghttp2_session_adjust_closed_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } + session->last_proc_stream_id = session->last_recv_stream_id; + rv = session_call_on_begin_headers(session, frame); if (rv != 0) { return rv; @@ -3670,6 +3667,11 @@ int nghttp2_session_on_priority_received(nghttp2_session *session, if (stream == NULL) { return NGHTTP2_ERR_NOMEM; } + + rv = nghttp2_session_adjust_idle_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } } else { rv = nghttp2_session_reprioritize_stream(session, stream, &frame->priority.pri_spec); @@ -3677,6 +3679,11 @@ int nghttp2_session_on_priority_received(nghttp2_session *session, if (nghttp2_is_fatal(rv)) { return rv; } + + rv = nghttp2_session_adjust_idle_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } } return session_call_on_frame_received(session, frame); @@ -4185,6 +4192,9 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, return NGHTTP2_ERR_NOMEM; } + /* We don't call nghttp2_session_adjust_closed_stream(), since we + don't keep closed stream in client side */ + session->last_proc_stream_id = session->last_recv_stream_id; rv = session_call_on_begin_headers(session, frame); if (rv != 0) { @@ -4809,6 +4819,14 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, mem = &session->mem; + /* We may have idle streams more than we expect (e.g., + nghttp2_session_change_stream_priority() or + nghttp2_session_create_idle_stream()). Adjust them here. */ + rv = nghttp2_session_adjust_idle_stream(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } + for (;;) { switch (iframe->state) { case NGHTTP2_IB_READ_CLIENT_MAGIC: @@ -6512,6 +6530,10 @@ static int nghttp2_session_upgrade_internal(nghttp2_session *session, if (stream == NULL) { return NGHTTP2_ERR_NOMEM; } + + /* We don't call nghttp2_session_adjust_closed_stream(), since this + should be the first stream open. */ + if (session->server) { nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); session->last_recv_stream_id = 1; @@ -6726,6 +6748,7 @@ int nghttp2_session_check_server_session(nghttp2_session *session) { int nghttp2_session_change_stream_priority( nghttp2_session *session, int32_t stream_id, const nghttp2_priority_spec *pri_spec) { + int rv; nghttp2_stream *stream; nghttp2_priority_spec pri_spec_copy; @@ -6741,7 +6764,18 @@ int nghttp2_session_change_stream_priority( pri_spec_copy = *pri_spec; nghttp2_priority_spec_normalize_weight(&pri_spec_copy); - return nghttp2_session_reprioritize_stream(session, stream, &pri_spec_copy); + rv = nghttp2_session_reprioritize_stream(session, stream, &pri_spec_copy); + + if (nghttp2_is_fatal(rv)) { + return rv; + } + + /* We don't intentionally call nghttp2_session_adjust_idle_stream() + so that idle stream created by this function, and existing ones + are kept for application. We will adjust number of idle stream + in nghttp2_session_mem_send or nghttp2_session_mem_recv is + called. */ + return 0; } int nghttp2_session_create_idle_stream(nghttp2_session *session, @@ -6770,5 +6804,10 @@ int nghttp2_session_create_idle_stream(nghttp2_session *session, return NGHTTP2_ERR_NOMEM; } + /* We don't intentionally call nghttp2_session_adjust_idle_stream() + so that idle stream created by this function, and existing ones + are kept for application. We will adjust number of idle stream + in nghttp2_session_mem_send or nghttp2_session_mem_recv is + called. */ return 0; } diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index dd69b5e3..5b47e94f 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -73,6 +73,10 @@ typedef struct { /* The default maximum number of incoming reserved streams */ #define NGHTTP2_MAX_INCOMING_RESERVED_STREAMS 200 +/* Even if we have less SETTINGS_MAX_CONCURRENT_STREAMS than this + number, we keep NGHTTP2_MIN_IDLE_STREAMS streams in idle state */ +#define NGHTTP2_MIN_IDLE_STREAMS 16 + /* The maximum number of items in outbound queue, which is considered as flooding caused by peer. All frames are not considered here. We only consider PING + ACK and SETTINGS + ACK. This is because @@ -443,6 +447,11 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags, * * This function returns a pointer to created new stream object, or * NULL. + * + * This function adjusts neither the number of closed streams or idle + * streams. The caller should manually call + * nghttp2_session_adjust_closed_stream() or + * nghttp2_session_adjust_idle_stream() respectively. */ nghttp2_stream *nghttp2_session_open_stream(nghttp2_session *session, int32_t stream_id, uint8_t flags, @@ -491,29 +500,17 @@ int nghttp2_session_destroy_stream(nghttp2_session *session, * limitation of maximum number of streams in memory, |stream| is not * closed and just deleted from memory (see * nghttp2_session_destroy_stream). - * - * This function returns 0 if it succeeds, or one the following - * negative error codes: - * - * NGHTTP2_ERR_NOMEM - * Out of memory */ -int nghttp2_session_keep_closed_stream(nghttp2_session *session, - nghttp2_stream *stream); +void nghttp2_session_keep_closed_stream(nghttp2_session *session, + nghttp2_stream *stream); /* * Appends |stream| to linked list |session->idle_stream_head|. We * apply fixed limit for list size. To fit into that limit, one or * more oldest streams are removed from list as necessary. - * - * This function returns 0 if it succeeds, or one the following - * negative error codes: - * - * NGHTTP2_ERR_NOMEM - * Out of memory */ -int nghttp2_session_keep_idle_stream(nghttp2_session *session, - nghttp2_stream *stream); +void nghttp2_session_keep_idle_stream(nghttp2_session *session, + nghttp2_stream *stream); /* * Detaches |stream| from idle streams linked list. @@ -524,9 +521,7 @@ void nghttp2_session_detach_idle_stream(nghttp2_session *session, /* * Deletes closed stream to ensure that number of incoming streams * including active and closed is in the maximum number of allowed - * stream. If |offset| is nonzero, it is decreased from the maximum - * number of allowed stream when comparing number of active and closed - * stream and the maximum number. + * stream. * * This function returns 0 if it succeeds, or one the following * negative error codes: @@ -534,8 +529,7 @@ void nghttp2_session_detach_idle_stream(nghttp2_session *session, * NGHTTP2_ERR_NOMEM * Out of memory */ -int nghttp2_session_adjust_closed_stream(nghttp2_session *session, - size_t offset); +int nghttp2_session_adjust_closed_stream(nghttp2_session *session); /* * Deletes idle stream to ensure that number of idle streams is in @@ -807,6 +801,9 @@ int nghttp2_session_update_local_settings(nghttp2_session *session, * |pri_spec|. Caller must ensure that stream->hd.stream_id != * pri_spec->stream_id. * + * This function does not adjust the number of idle streams. The + * caller should call nghttp2_session_adjust_idle_stream() later. + * * This function returns 0 if it succeeds, or one of the following * negative error codes: * diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 8e53aff1..2de4d028 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -7669,6 +7669,7 @@ void test_nghttp2_session_keep_closed_stream(void) { CU_ASSERT(NULL == session->closed_stream_head->closed_prev); open_stream(session, 11); + nghttp2_session_adjust_closed_stream(session); CU_ASSERT(1 == session->num_closed_streams); CU_ASSERT(5 == session->closed_stream_tail->stream_id); @@ -7677,6 +7678,7 @@ void test_nghttp2_session_keep_closed_stream(void) { CU_ASSERT(NULL == session->closed_stream_head->closed_next); open_stream(session, 13); + nghttp2_session_adjust_closed_stream(session); CU_ASSERT(0 == session->num_closed_streams); CU_ASSERT(NULL == session->closed_stream_tail); @@ -7689,6 +7691,7 @@ void test_nghttp2_session_keep_closed_stream(void) { /* server initiated stream is not counted to max concurrent limit */ open_stream(session, 2); + nghttp2_session_adjust_closed_stream(session); CU_ASSERT(1 == session->num_closed_streams); CU_ASSERT(3 == session->closed_stream_head->stream_id); @@ -7708,6 +7711,7 @@ void test_nghttp2_session_keep_idle_stream(void) { nghttp2_settings_entry iv = {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, max_concurrent_streams}; int i; + int32_t stream_id; memset(&callbacks, 0, sizeof(callbacks)); callbacks.send_callback = null_send_callback; @@ -7716,25 +7720,29 @@ void test_nghttp2_session_keep_idle_stream(void) { nghttp2_submit_settings(session, NGHTTP2_FLAG_NONE, &iv, 1); - /* We at least allow 2 idle streams even if max concurrent streams - is very low. */ - for (i = 0; i < 2; ++i) { + /* We at least allow NGHTTP2_MIN_IDLE_STREAM idle streams even if + max concurrent streams is very low. */ + for (i = 0; i < NGHTTP2_MIN_IDLE_STREAMS; ++i) { nghttp2_session_open_stream(session, i * 2 + 1, NGHTTP2_STREAM_FLAG_NONE, &pri_spec_default, NGHTTP2_STREAM_IDLE, NULL); + nghttp2_session_adjust_idle_stream(session); } - CU_ASSERT(2 == session->num_idle_streams); + CU_ASSERT(NGHTTP2_MIN_IDLE_STREAMS == session->num_idle_streams); + stream_id = (NGHTTP2_MIN_IDLE_STREAMS - 1) * 2 + 1; CU_ASSERT(1 == session->idle_stream_head->stream_id); - CU_ASSERT(3 == session->idle_stream_tail->stream_id); + CU_ASSERT(stream_id == session->idle_stream_tail->stream_id); - nghttp2_session_open_stream(session, 5, NGHTTP2_FLAG_NONE, &pri_spec_default, - NGHTTP2_STREAM_IDLE, NULL); + stream_id += 2; - CU_ASSERT(2 == session->num_idle_streams); + nghttp2_session_open_stream(session, stream_id, NGHTTP2_FLAG_NONE, + &pri_spec_default, NGHTTP2_STREAM_IDLE, NULL); + nghttp2_session_adjust_idle_stream(session); + CU_ASSERT(NGHTTP2_MIN_IDLE_STREAMS == session->num_idle_streams); CU_ASSERT(3 == session->idle_stream_head->stream_id); - CU_ASSERT(5 == session->idle_stream_tail->stream_id); + CU_ASSERT(stream_id == session->idle_stream_tail->stream_id); nghttp2_session_del(session); } From bd9a19e23be9a0114845389ce78c1ec590ff5ba6 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 17:07:39 +0900 Subject: [PATCH 38/66] Add test for 92a56d034f201cbb609606184822cf1716677207 --- tests/main.c | 2 ++ tests/nghttp2_session_test.c | 51 ++++++++++++++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 3 files changed, 54 insertions(+) diff --git a/tests/main.c b/tests/main.c index fb02ed5d..4bb7f041 100644 --- a/tests/main.c +++ b/tests/main.c @@ -290,6 +290,8 @@ int main(int argc _U_, char *argv[] _U_) { !CU_add_test(pSuite, "session_flooding", test_nghttp2_session_flooding) || !CU_add_test(pSuite, "session_change_stream_priority", test_nghttp2_session_change_stream_priority) || + !CU_add_test(pSuite, "session_repeated_priority_change", + test_nghttp2_session_repeated_priority_change) || !CU_add_test(pSuite, "http_mandatory_headers", test_nghttp2_http_mandatory_headers) || !CU_add_test(pSuite, "http_content_length", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 2de4d028..8e6f3a72 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -8642,6 +8642,57 @@ void test_nghttp2_session_create_idle_stream(void) { nghttp2_session_del(session); } +void test_nghttp2_session_repeated_priority_change(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + nghttp2_frame frame; + nghttp2_priority_spec pri_spec; + int32_t stream_id, last_stream_id; + int32_t max_streams = 20; + + memset(&callbacks, 0, sizeof(callbacks)); + + nghttp2_session_server_new(&session, &callbacks, NULL); + + session->local_settings.max_concurrent_streams = (uint32_t)max_streams; + + /* 1 -> 0 */ + nghttp2_priority_spec_init(&pri_spec, 0, 16, 0); + nghttp2_frame_priority_init(&frame.priority, 1, &pri_spec); + + CU_ASSERT(0 == nghttp2_session_on_priority_received(session, &frame)); + + nghttp2_frame_priority_free(&frame.priority); + + last_stream_id = max_streams * 2 + 1; + + for (stream_id = 3; stream_id < last_stream_id; stream_id += 2) { + /* 1 -> stream_id */ + nghttp2_priority_spec_init(&pri_spec, stream_id, 16, 0); + nghttp2_frame_priority_init(&frame.priority, 1, &pri_spec); + + CU_ASSERT(0 == nghttp2_session_on_priority_received(session, &frame)); + + nghttp2_frame_priority_free(&frame.priority); + } + + CU_ASSERT(20 == session->num_idle_streams); + CU_ASSERT(1 == session->idle_stream_head->stream_id); + + /* 1 -> last_stream_id */ + nghttp2_priority_spec_init(&pri_spec, last_stream_id, 16, 0); + nghttp2_frame_priority_init(&frame.priority, 1, &pri_spec); + + CU_ASSERT(0 == nghttp2_session_on_priority_received(session, &frame)); + + nghttp2_frame_priority_free(&frame.priority); + + CU_ASSERT(20 == session->num_idle_streams); + CU_ASSERT(3 == session->idle_stream_head->stream_id); + + nghttp2_session_del(session); +} + static void check_nghttp2_http_recv_headers_fail( nghttp2_session *session, nghttp2_hd_deflater *deflater, int32_t stream_id, int stream_state, const nghttp2_nv *nva, size_t nvlen) { diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index 1a0d47b3..cb23f4d4 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -138,6 +138,7 @@ void test_nghttp2_session_detach_item_from_closed_stream(void); void test_nghttp2_session_flooding(void); void test_nghttp2_session_change_stream_priority(void); void test_nghttp2_session_create_idle_stream(void); +void test_nghttp2_session_repeated_priority_change(void); void test_nghttp2_http_mandatory_headers(void); void test_nghttp2_http_content_length(void); void test_nghttp2_http_content_length_mismatch(void); From 8122bc5aeff0cbed025a78d888b9a3249d354067 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 17:14:36 +0900 Subject: [PATCH 39/66] Fix compile error with gcc ASAN enabled --- tests/nghttp2_session_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 8e6f3a72..b18c60ad 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -7634,7 +7634,7 @@ void test_nghttp2_session_keep_closed_stream(void) { nghttp2_session_callbacks callbacks; const size_t max_concurrent_streams = 5; nghttp2_settings_entry iv = {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, - max_concurrent_streams}; + (uint32_t)max_concurrent_streams}; size_t i; memset(&callbacks, 0, sizeof(callbacks)); @@ -7709,7 +7709,7 @@ void test_nghttp2_session_keep_idle_stream(void) { nghttp2_session_callbacks callbacks; const size_t max_concurrent_streams = 1; nghttp2_settings_entry iv = {NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS, - max_concurrent_streams}; + (uint32_t)max_concurrent_streams}; int i; int32_t stream_id; From 1b15bb7a5634e6f170bf14529b038b2c9c60edc7 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 22:48:37 +0900 Subject: [PATCH 40/66] Update neverbleed --- third-party/neverbleed | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third-party/neverbleed b/third-party/neverbleed index 81eff20b..5c47587b 160000 --- a/third-party/neverbleed +++ b/third-party/neverbleed @@ -1 +1 @@ -Subproject commit 81eff20bd84b4d0dce2cbbd1a5ad1384d086423b +Subproject commit 5c47587bc2855f2b9577a9bd369ed70088b77fec From 4988cd26b529c6fd3fb9699eec935c9809202416 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 22:49:39 +0900 Subject: [PATCH 41/66] Bump up version number to 1.6.0, LT revision to 18:0:4 --- configure.ac | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 152a30e0..28f41917 100644 --- a/configure.ac +++ b/configure.ac @@ -25,7 +25,7 @@ dnl Do not change user variables! dnl http://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html AC_PREREQ(2.61) -AC_INIT([nghttp2], [1.5.1-DEV], [t-tujikawa@users.sourceforge.net]) +AC_INIT([nghttp2], [1.6.0], [t-tujikawa@users.sourceforge.net]) AC_CONFIG_AUX_DIR([.]) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_HEADERS([config.h]) @@ -46,9 +46,9 @@ m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])]) dnl See versioning rule: dnl http://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html -AC_SUBST(LT_CURRENT, 17) +AC_SUBST(LT_CURRENT, 18) AC_SUBST(LT_REVISION, 0) -AC_SUBST(LT_AGE, 3) +AC_SUBST(LT_AGE, 4) major=`echo $PACKAGE_VERSION |cut -d. -f1 | sed -e "s/[^0-9]//g"` minor=`echo $PACKAGE_VERSION |cut -d. -f2 | sed -e "s/[^0-9]//g"` From fe8998ab5cfeeb32d391aa53533c483fb5d92bae Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 22:52:11 +0900 Subject: [PATCH 42/66] Update man pages --- doc/h2load.1 | 4 ++-- doc/h2load.1.rst | 2 +- doc/nghttp.1 | 2 +- doc/nghttpd.1 | 2 +- doc/nghttpx.1 | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/h2load.1 b/doc/h2load.1 index 272b9f0b..50ad4f8c 100644 --- a/doc/h2load.1 +++ b/doc/h2load.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "H2LOAD" "1" "December 08, 2015" "1.5.1-DEV" "nghttp2" +.TH "H2LOAD" "1" "December 23, 2015" "1.6.0" "nghttp2" .SH NAME h2load \- HTTP/2 benchmarking tool . @@ -375,7 +375,7 @@ deviation range (mean +/\- sd) against total number of successful connections. .UNINDENT .TP -.B req/s (client) +.B req/s .INDENT 7.0 .TP .B min diff --git a/doc/h2load.1.rst b/doc/h2load.1.rst index c6790728..368c6ab5 100644 --- a/doc/h2load.1.rst +++ b/doc/h2load.1.rst @@ -303,7 +303,7 @@ time for 1st byte (of (decrypted in case of TLS) application data) deviation range (mean +/- sd) against total number of successful connections. -req/s (client) +req/s min The minimum request per second among all clients. max diff --git a/doc/nghttp.1 b/doc/nghttp.1 index f2bdc527..6d7feb45 100644 --- a/doc/nghttp.1 +++ b/doc/nghttp.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "NGHTTP" "1" "December 08, 2015" "1.5.1-DEV" "nghttp2" +.TH "NGHTTP" "1" "December 23, 2015" "1.6.0" "nghttp2" .SH NAME nghttp \- HTTP/2 client . diff --git a/doc/nghttpd.1 b/doc/nghttpd.1 index 6f1978f9..6d841e82 100644 --- a/doc/nghttpd.1 +++ b/doc/nghttpd.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "NGHTTPD" "1" "December 08, 2015" "1.5.1-DEV" "nghttp2" +.TH "NGHTTPD" "1" "December 23, 2015" "1.6.0" "nghttp2" .SH NAME nghttpd \- HTTP/2 server . diff --git a/doc/nghttpx.1 b/doc/nghttpx.1 index a6f66efb..093a7fb9 100644 --- a/doc/nghttpx.1 +++ b/doc/nghttpx.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "NGHTTPX" "1" "December 08, 2015" "1.5.1-DEV" "nghttp2" +.TH "NGHTTPX" "1" "December 23, 2015" "1.6.0" "nghttp2" .SH NAME nghttpx \- HTTP/2 proxy . From 685f1772fc251c988fe307407be6019350c22d96 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 23 Dec 2015 22:56:39 +0900 Subject: [PATCH 43/66] Bump up version number to 1.6.1-DEV --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 28f41917..87fe49ec 100644 --- a/configure.ac +++ b/configure.ac @@ -25,7 +25,7 @@ dnl Do not change user variables! dnl http://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html AC_PREREQ(2.61) -AC_INIT([nghttp2], [1.6.0], [t-tujikawa@users.sourceforge.net]) +AC_INIT([nghttp2], [1.6.1-DEV], [t-tujikawa@users.sourceforge.net]) AC_CONFIG_AUX_DIR([.]) AC_CONFIG_MACRO_DIR([m4]) AC_CONFIG_HEADERS([config.h]) From 8919c8c139e3643d47fdd7dcd078453cb3be6f72 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 24 Dec 2015 21:15:46 +0900 Subject: [PATCH 44/66] Check initgroups with AC_CHECK_DECLS for cygwin --- configure.ac | 5 ++++- src/shrpx.h | 4 ++++ src/shrpx_worker_process.cc | 2 -- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 87fe49ec..23921b71 100644 --- a/configure.ac +++ b/configure.ac @@ -604,7 +604,6 @@ AC_CHECK_FUNCS([ \ dup2 \ getcwd \ getpwnam \ - initgroups \ localtime_r \ memchr \ memmove \ @@ -626,6 +625,10 @@ AC_CHECK_FUNCS([ \ AC_CHECK_FUNC([timerfd_create], [have_timerfd_create=yes], [have_timerfd_create=no]) +# For cygwin: we can link initgroups, so AC_CHECK_FUNCS succeeds, but +# cygwin disables initgroups due to feature test macro magic with our +# configuration. +AC_CHECK_DECLS([initgroups], [], [], [[#include ]]) # Checks for epoll availability, primarily for examples/tiny-nghttpd AX_HAVE_EPOLL([have_epoll=yes], [have_epoll=no]) diff --git a/src/shrpx.h b/src/shrpx.h index dfc52701..12e17179 100644 --- a/src/shrpx.h +++ b/src/shrpx.h @@ -44,4 +44,8 @@ #define DIE() _Exit(EXIT_FAILURE) +#if defined(HAVE_DECL_INITGROUPS) && !HAVE_DECL_INITGROUPS +inline int initgroups(const char *user, gid_t group) { return 0; } +#endif // defined(HAVE_DECL_INITGROUPS) && !HAVE_DECL_INITGROUPS + #endif // SHRPX_H diff --git a/src/shrpx_worker_process.cc b/src/shrpx_worker_process.cc index 04ff2698..c2358c58 100644 --- a/src/shrpx_worker_process.cc +++ b/src/shrpx_worker_process.cc @@ -64,14 +64,12 @@ void drop_privileges( #endif // HAVE_NEVERBLEED ) { if (getuid() == 0 && get_config()->uid != 0) { -#ifdef HAVE_INITGROUPS if (initgroups(get_config()->user.get(), get_config()->gid) != 0) { auto error = errno; LOG(FATAL) << "Could not change supplementary groups: " << strerror(error); exit(EXIT_FAILURE); } -#endif // HAVE_INITGROUPS if (setgid(get_config()->gid) != 0) { auto error = errno; LOG(FATAL) << "Could not change gid: " << strerror(error); From 894c1bd02ed71be5f684653d27749de541226f6f Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 24 Dec 2015 20:53:08 +0900 Subject: [PATCH 45/66] Create idle stream on client side With the presence of idle stream related API (e.g., nghttp2_create_idle_stream()), it is more predictable for client to create idle streams with its dependency to another idle stream. Previously, we didn't create complete parent idle stream in this case. Now we create idle streams as we do on server side. --- lib/nghttp2_session.c | 27 +++++---- tests/main.c | 2 + tests/nghttp2_session_test.c | 106 ++++++++++++++++++++++++++++++++++- tests/nghttp2_session_test.h | 1 + 4 files changed, 125 insertions(+), 11 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index f94a009a..e4dd4f97 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -630,7 +630,7 @@ int nghttp2_session_reprioritize_stream( if (pri_spec->stream_id != 0) { dep_stream = nghttp2_session_get_stream_raw(session, pri_spec->stream_id); - if (session->server && !dep_stream && + if (!dep_stream && session_detect_idle_stream(session, pri_spec->stream_id)) { nghttp2_priority_spec_default_init(&pri_spec_default); @@ -899,7 +899,7 @@ 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 (session->server && !dep_stream && + if (!dep_stream && session_detect_idle_stream(session, pri_spec->stream_id)) { /* Depends on idle stream, which does not exist in memory. Assign default priority for it. */ @@ -964,7 +964,6 @@ nghttp2_stream *nghttp2_session_open_stream(nghttp2_session *session, case NGHTTP2_STREAM_IDLE: /* Idle stream does not count toward the concurrent streams limit. This is used as anchor node in dependency tree. */ - assert(session->server); nghttp2_session_keep_idle_stream(session, stream); break; default: @@ -2357,14 +2356,22 @@ static int session_after_frame_sent1(nghttp2_session *session) { stream = nghttp2_session_get_stream_raw(session, frame->hd.stream_id); if (!stream) { - break; - } + if (!session_detect_idle_stream(session, frame->hd.stream_id)) { + break; + } - rv = nghttp2_session_reprioritize_stream(session, stream, - &frame->priority.pri_spec); - - if (nghttp2_is_fatal(rv)) { - return rv; + stream = nghttp2_session_open_stream( + session, frame->hd.stream_id, NGHTTP2_FLAG_NONE, + &frame->priority.pri_spec, NGHTTP2_STREAM_IDLE, NULL); + if (!stream) { + return NGHTTP2_ERR_NOMEM; + } + } else { + rv = nghttp2_session_reprioritize_stream(session, stream, + &frame->priority.pri_spec); + if (nghttp2_is_fatal(rv)) { + return rv; + } } rv = nghttp2_session_adjust_idle_stream(session); diff --git a/tests/main.c b/tests/main.c index 4bb7f041..5d1c2533 100644 --- a/tests/main.c +++ b/tests/main.c @@ -292,6 +292,8 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_session_change_stream_priority) || !CU_add_test(pSuite, "session_repeated_priority_change", test_nghttp2_session_repeated_priority_change) || + !CU_add_test(pSuite, "session_repeated_priority_submission", + test_nghttp2_session_repeated_priority_submission) || !CU_add_test(pSuite, "http_mandatory_headers", test_nghttp2_http_mandatory_headers) || !CU_add_test(pSuite, "http_content_length", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index b18c60ad..6fb2768e 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -8535,7 +8535,7 @@ void test_nghttp2_session_flooding(void) { void test_nghttp2_session_change_stream_priority(void) { nghttp2_session *session; nghttp2_session_callbacks callbacks; - nghttp2_stream *stream1, *stream2, *stream3; + nghttp2_stream *stream1, *stream2, *stream3, *stream5; nghttp2_priority_spec pri_spec; int rv; @@ -8568,6 +8568,41 @@ void test_nghttp2_session_change_stream_priority(void) { rv = nghttp2_session_change_stream_priority(session, 0, &pri_spec); CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == rv); + /* Depends on the non-existing idle stream. This creates that idle + stream. */ + nghttp2_priority_spec_init(&pri_spec, 5, 9, 1); + + rv = nghttp2_session_change_stream_priority(session, 2, &pri_spec); + + CU_ASSERT(0 == rv); + + stream5 = nghttp2_session_get_stream_raw(session, 5); + + CU_ASSERT(NULL != stream5); + CU_ASSERT(&session->root == stream5->dep_prev); + CU_ASSERT(stream5 == stream2->dep_prev); + CU_ASSERT(9 == stream2->weight); + + nghttp2_session_del(session); + + /* Check that this works in client session too */ + nghttp2_session_client_new(&session, &callbacks, NULL); + + stream1 = open_stream(session, 1); + + nghttp2_priority_spec_init(&pri_spec, 5, 9, 1); + + rv = nghttp2_session_change_stream_priority(session, 1, &pri_spec); + + CU_ASSERT(0 == rv); + + stream5 = nghttp2_session_get_stream_raw(session, 5); + + CU_ASSERT(NULL != stream5); + CU_ASSERT(&session->root == stream5->dep_prev); + CU_ASSERT(stream5 == stream1->dep_prev); + CU_ASSERT(9 == stream1->weight); + nghttp2_session_del(session); } @@ -8640,6 +8675,27 @@ void test_nghttp2_session_create_idle_stream(void) { CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == rv); nghttp2_session_del(session); + + /* Check that this works in client session too */ + nghttp2_session_client_new(&session, &callbacks, NULL); + + nghttp2_priority_spec_init(&pri_spec, 4, 99, 1); + + rv = nghttp2_session_create_idle_stream(session, 2, &pri_spec); + + CU_ASSERT(0 == rv); + + stream4 = nghttp2_session_get_stream_raw(session, 4); + stream2 = nghttp2_session_get_stream_raw(session, 2); + + CU_ASSERT(NULL != stream4); + CU_ASSERT(NULL != stream2); + CU_ASSERT(&session->root == stream4->dep_prev); + CU_ASSERT(NGHTTP2_DEFAULT_WEIGHT == stream4->weight); + CU_ASSERT(stream4 == stream2->dep_prev); + CU_ASSERT(99 == stream2->weight); + + nghttp2_session_del(session); } void test_nghttp2_session_repeated_priority_change(void) { @@ -8693,6 +8749,54 @@ void test_nghttp2_session_repeated_priority_change(void) { nghttp2_session_del(session); } +void test_nghttp2_session_repeated_priority_submission(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + nghttp2_priority_spec pri_spec; + int32_t stream_id, last_stream_id; + uint32_t max_streams = NGHTTP2_MIN_IDLE_STREAMS; + + memset(&callbacks, 0, sizeof(callbacks)); + + callbacks.send_callback = null_send_callback; + + nghttp2_session_client_new(&session, &callbacks, NULL); + + session->local_settings.max_concurrent_streams = max_streams; + + /* 1 -> 0 */ + nghttp2_priority_spec_init(&pri_spec, 0, 16, 0); + + CU_ASSERT(0 == + nghttp2_submit_priority(session, NGHTTP2_FLAG_NONE, 1, &pri_spec)); + + last_stream_id = (int32_t)(max_streams * 2 + 1); + + for (stream_id = 3; stream_id < last_stream_id; stream_id += 2) { + /* 1 -> stream_id */ + nghttp2_priority_spec_init(&pri_spec, stream_id, 16, 0); + + CU_ASSERT( + 0 == nghttp2_submit_priority(session, NGHTTP2_FLAG_NONE, 1, &pri_spec)); + } + + CU_ASSERT(0 == nghttp2_session_send(session)); + CU_ASSERT(max_streams == session->num_idle_streams); + CU_ASSERT(1 == session->idle_stream_head->stream_id); + + /* 1 -> last_stream_id */ + nghttp2_priority_spec_init(&pri_spec, last_stream_id, 16, 0); + + CU_ASSERT(0 == + nghttp2_submit_priority(session, NGHTTP2_FLAG_NONE, 1, &pri_spec)); + + CU_ASSERT(0 == nghttp2_session_send(session)); + CU_ASSERT(max_streams == session->num_idle_streams); + CU_ASSERT(3 == session->idle_stream_head->stream_id); + + nghttp2_session_del(session); +} + static void check_nghttp2_http_recv_headers_fail( nghttp2_session *session, nghttp2_hd_deflater *deflater, int32_t stream_id, int stream_state, const nghttp2_nv *nva, size_t nvlen) { diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index cb23f4d4..3a54ba00 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -139,6 +139,7 @@ void test_nghttp2_session_flooding(void); void test_nghttp2_session_change_stream_priority(void); void test_nghttp2_session_create_idle_stream(void); void test_nghttp2_session_repeated_priority_change(void); +void test_nghttp2_session_repeated_priority_submission(void); void test_nghttp2_http_mandatory_headers(void); void test_nghttp2_http_content_length(void); void test_nghttp2_http_content_length_mismatch(void); From 5ec6066fdd5c0f2591783913fe895a137e326388 Mon Sep 17 00:00:00 2001 From: ayanamist Date: Fri, 25 Dec 2015 11:03:55 +0800 Subject: [PATCH 46/66] header value should not be inp_strlower http header keys are case-insensitive, but header values are case-sensitive, so it should not be changed. --- src/shrpx_config.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index 565d3118..e9deed04 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -291,7 +291,6 @@ std::pair parse_header(const char *optarg) { auto p = std::make_pair(std::string(optarg, colon), std::string(value, strlen(value))); util::inp_strlower(p.first); - util::inp_strlower(p.second); return p; } From 486dba8d8a3d214d7284b4965722288e74ec7ee2 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 25 Dec 2015 20:57:24 +0900 Subject: [PATCH 47/66] nghttpx: Strict validation for header fields given in configuration --- src/shrpx_config.cc | 16 +++++++++++----- src/shrpx_config_test.cc | 9 +++++++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index e9deed04..2281d4ab 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -276,11 +276,9 @@ std::string read_passwd_from_file(const char *filename) { } std::pair parse_header(const char *optarg) { - // We skip possible ":" at the start of optarg. - const auto *colon = strchr(optarg + 1, ':'); + const auto *colon = strchr(optarg, ':'); - // name = ":" is not allowed - if (colon == nullptr || (optarg[0] == ':' && colon == optarg + 1)) { + if (colon == nullptr || colon == optarg) { return {"", ""}; } @@ -292,6 +290,14 @@ std::pair parse_header(const char *optarg) { std::string(value, strlen(value))); util::inp_strlower(p.first); + if (!nghttp2_check_header_name( + reinterpret_cast(p.first.c_str()), p.first.size()) || + !nghttp2_check_header_value( + reinterpret_cast(p.second.c_str()), + p.second.size())) { + return {"", ""}; + } + return p; } @@ -1799,7 +1805,7 @@ int parse_config(const char *opt, const char *optarg, case SHRPX_OPTID_ADD_RESPONSE_HEADER: { auto p = parse_header(optarg); if (p.first.empty()) { - LOG(ERROR) << opt << ": header field name is empty: " << optarg; + LOG(ERROR) << opt << ": invalid header field: " << optarg; return -1; } if (optid == SHRPX_OPTID_ADD_REQUEST_HEADER) { diff --git a/src/shrpx_config_test.cc b/src/shrpx_config_test.cc index 5138dba4..8ac0cda0 100644 --- a/src/shrpx_config_test.cc +++ b/src/shrpx_config_test.cc @@ -46,8 +46,7 @@ void test_shrpx_config_parse_header(void) { CU_ASSERT("b" == p.second); p = parse_header(":a: b"); - CU_ASSERT(":a" == p.first); - CU_ASSERT("b" == p.second); + CU_ASSERT(p.first.empty()); p = parse_header("a: :b"); CU_ASSERT("a" == p.first); @@ -59,6 +58,12 @@ void test_shrpx_config_parse_header(void) { p = parse_header("alpha: bravo charlie"); CU_ASSERT("alpha" == p.first); CU_ASSERT("bravo charlie" == p.second); + + p = parse_header("a,: b"); + CU_ASSERT(p.first.empty()); + + p = parse_header("a: b\x0a"); + CU_ASSERT(p.first.empty()); } void test_shrpx_config_parse_log_format(void) { From dd4d549dc1a68ad4ef53581f4ac42560e6cebb02 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 25 Dec 2015 21:06:25 +0900 Subject: [PATCH 48/66] asio: Rename http2::get_io_services() as http2::io_services() The naming convention in asio lib does not use get_something. --- src/asio_io_service_pool.cc | 2 +- src/asio_io_service_pool.h | 2 +- src/asio_server.cc | 4 ++-- src/asio_server.h | 2 +- src/asio_server_http2.cc | 4 ++-- src/asio_server_http2_impl.cc | 4 ++-- src/asio_server_http2_impl.h | 2 +- src/includes/nghttp2/asio_http2_server.h | 2 +- 8 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/asio_io_service_pool.cc b/src/asio_io_service_pool.cc index 1f6c1353..b35a8c82 100644 --- a/src/asio_io_service_pool.cc +++ b/src/asio_io_service_pool.cc @@ -93,7 +93,7 @@ boost::asio::io_service &io_service_pool::get_io_service() { } const std::vector> & -io_service_pool::get_io_services() const { +io_service_pool::io_services() const { return io_services_; } diff --git a/src/asio_io_service_pool.h b/src/asio_io_service_pool.h index e7aaa049..c9ce1de4 100644 --- a/src/asio_io_service_pool.h +++ b/src/asio_io_service_pool.h @@ -72,7 +72,7 @@ public: /// Get access to all io_service objects. const std::vector> & - get_io_services() const; + io_services() const; private: /// The pool of io_services. diff --git a/src/asio_server.cc b/src/asio_server.cc index 002c947d..ef5becb2 100644 --- a/src/asio_server.cc +++ b/src/asio_server.cc @@ -180,8 +180,8 @@ void server::stop() { io_service_pool_.stop(); } void server::join() { io_service_pool_.join(); } const std::vector> & -server::get_io_services() const { - return io_service_pool_.get_io_services(); +server::io_services() const { + return io_service_pool_.io_services(); } } // namespace server diff --git a/src/asio_server.h b/src/asio_server.h index e1c52792..ba840348 100644 --- a/src/asio_server.h +++ b/src/asio_server.h @@ -77,7 +77,7 @@ public: /// Get access to all io_service objects. const std::vector> & - get_io_services() const; + io_services() const; private: /// Initiate an asynchronous accept operation. diff --git a/src/asio_server_http2.cc b/src/asio_server_http2.cc index 5606d833..997fd402 100644 --- a/src/asio_server_http2.cc +++ b/src/asio_server_http2.cc @@ -86,8 +86,8 @@ void http2::stop() { impl_->stop(); } void http2::join() { return impl_->join(); } const std::vector> & -http2::get_io_services() const { - return impl_->get_io_services(); +http2::io_services() const { + return impl_->io_services(); } } // namespace server diff --git a/src/asio_server_http2_impl.cc b/src/asio_server_http2_impl.cc index 5e420219..6a047f66 100644 --- a/src/asio_server_http2_impl.cc +++ b/src/asio_server_http2_impl.cc @@ -73,8 +73,8 @@ void http2_impl::stop() { return server_->stop(); } void http2_impl::join() { return server_->join(); } const std::vector> & -http2_impl::get_io_services() const { - return server_->get_io_services(); +http2_impl::io_services() const { + return server_->io_services(); } } // namespace server diff --git a/src/asio_server_http2_impl.h b/src/asio_server_http2_impl.h index a3b98552..b55b68c5 100644 --- a/src/asio_server_http2_impl.h +++ b/src/asio_server_http2_impl.h @@ -53,7 +53,7 @@ public: void stop(); void join(); const std::vector> & - get_io_services() const; + io_services() const; private: std::unique_ptr server_; diff --git a/src/includes/nghttp2/asio_http2_server.h b/src/includes/nghttp2/asio_http2_server.h index 5b969593..5818e301 100644 --- a/src/includes/nghttp2/asio_http2_server.h +++ b/src/includes/nghttp2/asio_http2_server.h @@ -212,7 +212,7 @@ public: // Get access to the io_service objects. const std::vector> & - get_io_services() const; + io_services() const; private: std::unique_ptr impl_; From 8716dd05d44f3b4cf0ff719240297cec57359815 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 25 Dec 2015 21:37:18 +0900 Subject: [PATCH 49/66] Return error from nghttp2_submit_{headers,request} when self dependency is made Return NGHTTP2_ERR_INVALID_ARGUMENT from nghttp2_submit_headers() if given stream ID and pri_spec->stream_id are the same (thus trying to depend on itself). Also return NGHTTP2_ERR_INVALID_ARGUMENT from nghttp2_submit_request() and nghttp2_submit_headers() with stream_id == 1, when new stream ID equals to pri_spec->stream_id. Previously, these cases are not checked, and just sent to peer. --- lib/includes/nghttp2/nghttp2.h | 6 +++++- lib/nghttp2_submit.c | 10 ++++++++++ tests/nghttp2_session_test.c | 26 ++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 5b7d8a28..cc4844de 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -3161,6 +3161,9 @@ nghttp2_priority_spec_check_default(const nghttp2_priority_spec *pri_spec); * :enum:`NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE` * No stream ID is available because maximum stream ID was * reached. + * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` + * Trying to depend on itself (new stream ID equals + * ``pri_spec->stream_id``). * * .. warning:: * @@ -3371,7 +3374,8 @@ NGHTTP2_EXTERN int nghttp2_submit_trailer(nghttp2_session *session, * No stream ID is available because maximum stream ID was * reached. * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` - * The |stream_id| is 0. + * The |stream_id| is 0; or trying to depend on itself (stream ID + * equals ``pri_spec->stream_id``). * :enum:`NGHTTP2_ERR_DATA_EXIST` * DATA or HEADERS has been already submitted and not fully * processed yet. This happens if stream denoted by |stream_id| diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index 2800b60e..67ecf4f6 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -55,6 +55,16 @@ static int32_t submit_headers_shared(nghttp2_session *session, uint8_t flags, goto fail; } + if (stream_id == -1) { + if ((int32_t)session->next_stream_id == pri_spec->stream_id) { + rv = NGHTTP2_ERR_INVALID_ARGUMENT; + goto fail; + } + } else if (stream_id == pri_spec->stream_id) { + rv = NGHTTP2_ERR_INVALID_ARGUMENT; + goto fail; + } + item = nghttp2_mem_malloc(mem, sizeof(nghttp2_outbound_item)); if (item == NULL) { rv = NGHTTP2_ERR_NOMEM; diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 6fb2768e..c8ed91ef 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -3965,6 +3965,7 @@ void test_nghttp2_submit_request_without_data(void) { nva_out out; nghttp2_bufs bufs; nghttp2_mem *mem; + nghttp2_priority_spec pri_spec; mem = nghttp2_mem_default(); frame_pack_bufs_init(&bufs); @@ -3998,6 +3999,15 @@ void test_nghttp2_submit_request_without_data(void) { nghttp2_bufs_free(&bufs); nghttp2_hd_inflate_free(&inflater); + + /* Try to depend on itself is error */ + nghttp2_priority_spec_init(&pri_spec, (int32_t)session->next_stream_id, 16, + 0); + + CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + nghttp2_submit_request(session, &pri_spec, reqnv, ARRLEN(reqnv), + NULL, NULL)); + nghttp2_session_del(session); } @@ -4289,6 +4299,7 @@ void test_nghttp2_submit_headers(void) { nva_out out; nghttp2_bufs bufs; nghttp2_mem *mem; + nghttp2_priority_spec pri_spec; mem = nghttp2_mem_default(); frame_pack_bufs_init(&bufs); @@ -4343,6 +4354,21 @@ void test_nghttp2_submit_headers(void) { nghttp2_frame_headers_free(&frame.headers, mem); nghttp2_hd_inflate_free(&inflater); + + /* Try to depend on itself */ + nghttp2_priority_spec_init(&pri_spec, 3, 16, 0); + + CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, 3, &pri_spec, + reqnv, ARRLEN(reqnv), NULL)); + + session->next_stream_id = 5; + nghttp2_priority_spec_init(&pri_spec, 5, 16, 0); + + CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, -1, &pri_spec, + reqnv, ARRLEN(reqnv), NULL)); + nghttp2_session_del(session); } From 3a9cb85d7a359c7bb70b38574606a3f84e19332e Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 26 Dec 2015 00:30:55 +0900 Subject: [PATCH 50/66] Add test to make sure idle streams are reduced --- tests/nghttp2_session_test.c | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index c8ed91ef..6de1f9fc 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -8638,8 +8638,10 @@ void test_nghttp2_session_create_idle_stream(void) { nghttp2_stream *stream2, *stream4, *stream8, *stream10; nghttp2_priority_spec pri_spec; int rv; + int i; memset(&callbacks, 0, sizeof(callbacks)); + callbacks.send_callback = null_send_callback; nghttp2_session_server_new(&session, &callbacks, NULL); @@ -8722,6 +8724,50 @@ void test_nghttp2_session_create_idle_stream(void) { CU_ASSERT(99 == stream2->weight); nghttp2_session_del(session); + + /* Check that idle stream is reduced when nghttp2_session_send() is + called. */ + nghttp2_session_server_new(&session, &callbacks, NULL); + + session->local_settings.max_concurrent_streams = 30; + + nghttp2_priority_spec_init(&pri_spec, 0, 16, 0); + for (i = 0; i < 100; ++i) { + rv = nghttp2_session_create_idle_stream(session, i * 2 + 1, &pri_spec); + + CU_ASSERT(0 == rv); + + nghttp2_priority_spec_init(&pri_spec, i * 2 + 1, 16, 0); + } + + CU_ASSERT(100 == session->num_idle_streams); + CU_ASSERT(0 == nghttp2_session_send(session)); + CU_ASSERT(30 == session->num_idle_streams); + CU_ASSERT(141 == session->idle_stream_head->stream_id); + + nghttp2_session_del(session); + + /* Check that idle stream is reduced when nghttp2_session_mem_recv() is + called. */ + nghttp2_session_client_new(&session, &callbacks, NULL); + + session->local_settings.max_concurrent_streams = 30; + + nghttp2_priority_spec_init(&pri_spec, 0, 16, 0); + for (i = 0; i < 100; ++i) { + rv = nghttp2_session_create_idle_stream(session, i * 2 + 1, &pri_spec); + + CU_ASSERT(0 == rv); + + nghttp2_priority_spec_init(&pri_spec, i * 2 + 1, 16, 0); + } + + CU_ASSERT(100 == session->num_idle_streams); + CU_ASSERT(0 == nghttp2_session_mem_recv(session, NULL, 0)); + CU_ASSERT(30 == session->num_idle_streams); + CU_ASSERT(141 == session->idle_stream_head->stream_id); + + nghttp2_session_del(session); } void test_nghttp2_session_repeated_priority_change(void) { From 4f06ccd17dedbd2e6617e54b01b2acf5e2c8799c Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 31 Dec 2015 00:13:16 +0900 Subject: [PATCH 51/66] Add -ldl to APPLDFLAGS for static openssl linking --- configure.ac | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/configure.ac b/configure.ac index 23921b71..33f461cf 100644 --- a/configure.ac +++ b/configure.ac @@ -255,6 +255,11 @@ if test "x${have_zlib}" = "xno"; then AC_MSG_NOTICE($ZLIB_PKG_ERRORS) fi +# dl: openssl requires libdl when it is statically linked. +LIBS_OLD=$LIBS +AC_SEARCH_LIBS([dlopen], [dl], [APPLDFLAGS="-ldl $APPLDFLAGS"], [], []) +LIBS=$LIBS_OLD + # cunit PKG_CHECK_MODULES([CUNIT], [cunit >= 2.1], [have_cunit=yes], [have_cunit=no]) # If pkg-config does not find cunit, check it using AC_CHECK_LIB. We From 848f8fbe54ec876351c7ab4da22fdb43ead45c39 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 5 Jan 2016 16:41:34 +0900 Subject: [PATCH 52/66] nghttpx: Don't apply max_header_fields and header_field_buffer limit to response We modeled max_header_fields and header_field_buffer limit from Apache configuration directives. In Apache, they are only applied to request header fields, while we applied both request and response. Since nghttpx is used as reverse proxy and backend server is relatively "trusted", this commit removes the application to response header fields. --- src/shrpx.cc | 9 ++++--- src/shrpx_http2_session.cc | 35 ------------------------ src/shrpx_http_downstream_connection.cc | 36 ------------------------- 3 files changed, 5 insertions(+), 75 deletions(-) diff --git a/src/shrpx.cc b/src/shrpx.cc index a83062f3..16daa620 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -1610,14 +1610,15 @@ HTTP: used several times to specify multiple header fields. Example: --add-response-header="foo: bar" --header-field-buffer= - Set maximum buffer size for incoming HTTP header field - list. This is the sum of header name and value in + Set maximum buffer size for incoming HTTP request header + field list. This is the sum of header name and value in bytes. Default: )" << util::utos_with_unit(get_config()->header_field_buffer) << R"( --max-header-fields= - Set maximum number of incoming HTTP header fields, which - appear in one request or response header field list. + Set maximum number of incoming HTTP request header + fields, which appear in one request or response header + field list. Default: )" << get_config()->max_header_fields << R"( Debug: diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index dd6ed02d..7ab65ed8 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -757,26 +757,6 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, auto trailer = frame->headers.cat == NGHTTP2_HCAT_HEADERS && !downstream->get_expect_final_response(); - if (downstream->get_response_headers_sum() + namelen + valuelen > - get_config()->header_field_buffer || - downstream->get_response_headers().size() >= - get_config()->max_header_fields) { - if (LOG_ENABLED(INFO)) { - DLOG(INFO, downstream) - << "Too large or many header field size=" - << downstream->get_response_headers_sum() + namelen + valuelen - << ", num=" << downstream->get_response_headers().size() + 1; - } - - if (trailer) { - // we don't care trailer part exceeds header size limit; just - // discard it. - return 0; - } - - return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; - } - if (trailer) { // just store header fields for trailer part downstream->add_response_trailer(name, namelen, value, valuelen, @@ -803,21 +783,6 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, assert(promised_downstream); - if (promised_downstream->get_request_headers_sum() + namelen + valuelen > - get_config()->header_field_buffer || - promised_downstream->get_request_headers().size() >= - get_config()->max_header_fields) { - if (LOG_ENABLED(INFO)) { - DLOG(INFO, promised_downstream) - << "Too large or many header field size=" - << promised_downstream->get_request_headers_sum() + namelen + - valuelen << ", num=" - << promised_downstream->get_request_headers().size() + 1; - } - - return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; - } - auto token = http2::lookup_token(name, namelen); promised_downstream->add_request_header(name, namelen, value, valuelen, flags & NGHTTP2_NV_FLAG_NO_INDEX, diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index 720f5a34..bab1c902 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/src/shrpx_http_downstream_connection.cc @@ -562,29 +562,10 @@ int htp_hdrs_completecb(http_parser *htp) { namespace { int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { auto downstream = static_cast(htp->data); - - if (downstream->get_response_headers_sum() + len > - get_config()->header_field_buffer) { - if (LOG_ENABLED(INFO)) { - DLOG(INFO, downstream) << "Too large header block size=" - << downstream->get_response_headers_sum() + len; - } - return -1; - } - if (downstream->get_response_state() == Downstream::INITIAL) { if (downstream->get_response_header_key_prev()) { downstream->append_last_response_header_key(data, len); } else { - if (downstream->get_response_headers().size() >= - get_config()->max_header_fields) { - if (LOG_ENABLED(INFO)) { - DLOG(INFO, downstream) - << "Too many header field num=" - << downstream->get_response_headers().size() + 1; - } - return -1; - } downstream->add_response_header(std::string(data, len), ""); } } else { @@ -592,15 +573,6 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { if (downstream->get_response_trailer_key_prev()) { downstream->append_last_response_trailer_key(data, len); } else { - if (downstream->get_response_headers().size() >= - get_config()->max_header_fields) { - if (LOG_ENABLED(INFO)) { - DLOG(INFO, downstream) - << "Too many header field num=" - << downstream->get_response_headers().size() + 1; - } - return -1; - } downstream->add_response_trailer(std::string(data, len), ""); } } @@ -611,14 +583,6 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { namespace { int htp_hdr_valcb(http_parser *htp, const char *data, size_t len) { auto downstream = static_cast(htp->data); - if (downstream->get_response_headers_sum() + len > - get_config()->header_field_buffer) { - if (LOG_ENABLED(INFO)) { - DLOG(INFO, downstream) << "Too large header block size=" - << downstream->get_response_headers_sum() + len; - } - return -1; - } if (downstream->get_response_state() == Downstream::INITIAL) { if (downstream->get_response_header_key_prev()) { downstream->set_last_response_header_value(data, len); From 2f50bc1b3ce4c938f517bea8d0f465e6b9d3543c Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 5 Jan 2016 16:47:44 +0900 Subject: [PATCH 53/66] nghttpx: Take into account of trailers when applying max_header_fields --- src/shrpx_http2_upstream.cc | 3 ++- src/shrpx_https_upstream.cc | 3 ++- src/shrpx_spdy_upstream.cc | 2 ++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index bc0de69c..de1dec18 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -169,7 +169,8 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, if (downstream->get_request_headers_sum() + namelen + valuelen > get_config()->header_field_buffer || - downstream->get_request_headers().size() >= + downstream->get_request_headers().size() + + downstream->get_request_trailers().size() >= get_config()->max_header_fields) { if (downstream->get_response_state() == Downstream::MSG_COMPLETE) { return 0; diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 1acd8f4a..8f584749 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -146,7 +146,8 @@ int htp_hdr_keycb(http_parser *htp, const char *data, size_t len) { if (downstream->get_request_trailer_key_prev()) { downstream->append_last_request_trailer_key(data, len); } else { - if (downstream->get_request_headers().size() >= + if (downstream->get_request_headers().size() + + downstream->get_request_trailers().size() >= get_config()->max_header_fields) { if (LOG_ENABLED(INFO)) { ULOG(INFO, upstream) << "Too many header field num=" diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index d7d2269a..604fb612 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -169,6 +169,8 @@ void on_ctrl_recv_callback(spdylay_session *session, spdylay_frame_type type, header_buffer += strlen(nv[i]) + strlen(nv[i + 1]); } + // spdy does not define usage of trailer fields, and we ignores + // them. if (header_buffer > get_config()->header_field_buffer || num_headers > get_config()->max_header_fields) { upstream->rst_stream(downstream, SPDYLAY_INTERNAL_ERROR); From f3a37b2ef1f299db1086525ad2952ee3ce0fa6ab Mon Sep 17 00:00:00 2001 From: kumagi Date: Tue, 5 Jan 2016 22:14:54 +0900 Subject: [PATCH 54/66] fix typos: heder->header alreay->already reponse->response --- README.rst | 2 +- lib/includes/nghttp2/nghttp2.h | 8 ++++---- lib/nghttp2_session.c | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/README.rst b/README.rst index 5b29e3d2..aa40c628 100644 --- a/README.rst +++ b/README.rst @@ -274,7 +274,7 @@ for this 24 bytes byte string and updated API. ``NGHTTP2_CLIENT_MAGIC_LEN``. * ``NGHTTP2_BAD_PREFACE`` was renamed as ``NGHTTP2_BAD_CLIENT_MAGIC`` -The alreay deprecated ``NGHTTP2_CLIENT_CONNECTION_HEADER`` and +The already deprecated ``NGHTTP2_CLIENT_CONNECTION_HEADER`` and ``NGHTTP2_CLIENT_CONNECTION_HEADER_LEN`` were removed. If application uses these macros, just replace old ones with new ones. diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index cc4844de..3c985332 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -3275,7 +3275,7 @@ nghttp2_submit_response(nghttp2_session *session, int32_t stream_id, * has already sent and if `nghttp2_submit_trailer()` is called before * any response HEADERS submission (usually by * `nghttp2_submit_response()`), the content of |nva| will be sent as - * reponse headers, which will result in error. + * response headers, which will result in error. * * This function has the same effect with `nghttp2_submit_headers()`, * with flags = :enum:`NGHTTP2_FLAG_END_HEADERS` and both pri_spec and @@ -3418,7 +3418,7 @@ nghttp2_submit_headers(nghttp2_session *session, uint8_t flags, * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` * The |stream_id| is 0. * :enum:`NGHTTP2_ERR_STREAM_CLOSED` - * The stream was alreay closed; or the |stream_id| is invalid. + * The stream was already closed; or the |stream_id| is invalid. * * .. note:: * @@ -3587,7 +3587,7 @@ NGHTTP2_EXTERN int nghttp2_submit_settings(nghttp2_session *session, * The |stream_id| is 0; The |stream_id| does not designate stream * that peer initiated. * :enum:`NGHTTP2_ERR_STREAM_CLOSED` - * The stream was alreay closed; or the |stream_id| is invalid. + * The stream was already closed; or the |stream_id| is invalid. * * .. warning:: * @@ -4174,7 +4174,7 @@ typedef enum { * :enum:`NGHTTP2_ERR_HEADER_COMP` * Inflation process has failed. * :enum:`NGHTTP2_ERR_BUFFER_ERROR` - * The heder field name or value is too large. + * The header field name or value is too large. * * Example follows:: * diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index e4dd4f97..6fda5442 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -1823,7 +1823,7 @@ static int session_prep_frame(nghttp2_session *session, } if (rv != 0) { - // If stream was alreay closed, nghttp2_session_get_stream() + // If stream was already closed, nghttp2_session_get_stream() // returns NULL, but item is still attached to the stream. // Search stream including closed again. stream = nghttp2_session_get_stream_raw(session, frame->hd.stream_id); @@ -1975,7 +1975,7 @@ static int session_prep_frame(nghttp2_session *session, rv = nghttp2_session_predicate_data_send(session, stream); if (rv != 0) { - // If stream was alreay closed, nghttp2_session_get_stream() + // If stream was already closed, nghttp2_session_get_stream() // returns NULL, but item is still attached to the stream. // Search stream including closed again. stream = nghttp2_session_get_stream_raw(session, frame->hd.stream_id); From 60bbb5cae058cc5133af00f14f8f6f56bc5b1d03 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 5 Jan 2016 22:36:37 +0900 Subject: [PATCH 55/66] h2load: Perform sampling for request timings to reduce memory consumption --- src/h2load.cc | 62 ++++++++++++++++++++++++++++++------- src/h2load.h | 13 ++++++-- src/h2load_http1_session.cc | 18 ++++++----- src/h2load_http1_session.h | 3 +- src/h2load_http2_session.cc | 37 +++++++++++++++------- src/h2load_http2_session.h | 2 +- src/h2load_session.h | 2 +- src/h2load_spdy_session.cc | 15 +++------ src/h2load_spdy_session.h | 2 +- 9 files changed, 106 insertions(+), 48 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index 1b8bd343..b1a5edda 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -98,11 +98,15 @@ Config config; RequestStat::RequestStat() : data_offset(0), completed(false) {} +constexpr size_t MAX_STATS = 1000000; + Stats::Stats(size_t req_todo, size_t nclients) : req_todo(0), req_started(0), req_done(0), req_success(0), req_status_success(0), req_failed(0), req_error(0), req_timedout(0), bytes_total(0), bytes_head(0), bytes_head_decomp(0), bytes_body(0), - status(), req_stats(req_todo), client_stats(nclients) {} + status(), client_stats(nclients) { + req_stats.reserve(std::min(req_todo, MAX_STATS)); +} Stream::Stream() : status_success(-1) {} @@ -420,9 +424,8 @@ void Client::disconnect() { } int Client::submit_request() { - auto req_stat = &worker->stats.req_stats[worker->stats.req_started++]; - - if (session->submit_request(req_stat) != 0) { + ++worker->stats.req_started; + if (session->submit_request() != 0) { return -1; } @@ -607,8 +610,12 @@ void Client::on_status_code(int32_t stream_id, uint16_t status) { } } -void Client::on_stream_close(int32_t stream_id, bool success, - RequestStat *req_stat, bool final) { +void Client::on_stream_close(int32_t stream_id, bool success, bool final) { + auto req_stat = get_req_stat(stream_id); + if (!req_stat) { + return; + } + req_stat->stream_close_time = std::chrono::steady_clock::now(); if (success) { req_stat->completed = true; @@ -628,6 +635,12 @@ void Client::on_stream_close(int32_t stream_id, bool success, ++worker->stats.req_failed; ++worker->stats.req_error; } + + if (req_stat->completed && + (worker->stats.req_done % worker->request_times_sampling_step) == 0) { + worker->sample_req_stat(req_stat); + } + report_progress(); streams.erase(stream_id); if (req_done == req_todo) { @@ -645,6 +658,15 @@ void Client::on_stream_close(int32_t stream_id, bool success, } } +RequestStat *Client::get_req_stat(int32_t stream_id) { + auto it = streams.find(stream_id); + if (it == std::end(streams)) { + return nullptr; + } + + return &(*it).second.req_stat; +} + int Client::connection_made() { if (ssl) { report_tls_info(); @@ -750,7 +772,6 @@ int Client::connection_made() { if (!config.timing_script) { auto nreq = std::min(req_todo - req_started, (size_t)config.max_concurrent_streams); - for (; nreq > 0; --nreq) { if (submit_request() != 0) { process_request_failure(); @@ -1060,6 +1081,10 @@ Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, clients.push_back(make_unique(next_client_id++, this, req_todo)); } } + + auto request_times_max_stats = std::min(req_todo, MAX_STATS); + request_times_sampling_step = + (req_todo + request_times_max_stats - 1) / request_times_max_stats; } Worker::~Worker() { @@ -1088,6 +1113,11 @@ void Worker::run() { ev_run(loop, 0); } +void Worker::sample_req_stat(RequestStat *req_stat) { + stats.req_stats.push_back(*req_stat); + assert(stats.req_stats.size() <= MAX_STATS); +} + namespace { // Returns percentage of number of samples within mean +/- sd. double within_sd(const std::vector &samples, double mean, double sd) { @@ -1106,12 +1136,15 @@ double within_sd(const std::vector &samples, double mean, double sd) { namespace { // Computes statistics using |samples|. The min, max, mean, sd, and // percentage of number of samples within mean +/- sd are computed. -SDStat compute_time_stat(const std::vector &samples) { +// If |sampling| is true, this computes sample variance. Otherwise, +// population variance. +SDStat compute_time_stat(const std::vector &samples, + bool sampling = false) { if (samples.empty()) { return {0.0, 0.0, 0.0, 0.0, 0.0}; } // standard deviation calculated using Rapid calculation method: - // http://en.wikipedia.org/wiki/Standard_deviation#Rapid_calculation_methods + // https://en.wikipedia.org/wiki/Standard_deviation#Rapid_calculation_methods double a = 0, q = 0; size_t n = 0; double sum = 0; @@ -1130,7 +1163,7 @@ SDStat compute_time_stat(const std::vector &samples) { assert(n > 0); res.mean = sum / n; - res.sd = sqrt(q / n); + res.sd = sqrt(q / (sampling && n > 1 ? n - 1 : n)); res.within_sd = within_sd(samples, res.mean, res.sd); return res; @@ -1140,9 +1173,13 @@ SDStat compute_time_stat(const std::vector &samples) { namespace { SDStats process_time_stats(const std::vector> &workers) { + auto request_times_sampling = false; size_t nrequest_times = 0; for (const auto &w : workers) { nrequest_times += w->stats.req_stats.size(); + if (w->request_times_sampling_step != 1) { + request_times_sampling = true; + } } std::vector request_times; @@ -1195,8 +1232,9 @@ process_time_stats(const std::vector> &workers) { } } - return {compute_time_stat(request_times), compute_time_stat(connect_times), - compute_time_stat(ttfb_times), compute_time_stat(rps_values)}; + return {compute_time_stat(request_times, request_times_sampling), + compute_time_stat(connect_times), compute_time_stat(ttfb_times), + compute_time_stat(rps_values)}; } } // namespace diff --git a/src/h2load.h b/src/h2load.h index a7e07ea4..a6366f6d 100644 --- a/src/h2load.h +++ b/src/h2load.h @@ -226,6 +226,9 @@ struct Worker { // at most nreqs_rem clients get an extra request size_t nreqs_rem; size_t rate; + // every successful request_times_sampling_step-th request's + // req_stat will get sampled. + size_t request_times_sampling_step; ev_timer timeout_watcher; // The next client ID this worker assigns uint32_t next_client_id; @@ -235,9 +238,11 @@ struct Worker { ~Worker(); Worker(Worker &&o) = default; void run(); + void sample_req_stat(RequestStat *req_stat); }; struct Stream { + RequestStat req_stat; int status_success; Stream(); }; @@ -318,8 +323,12 @@ struct Client { // |success| == true means that the request/response was exchanged // |successfully, but it does not mean response carried successful // |HTTP status code. - void on_stream_close(int32_t stream_id, bool success, RequestStat *req_stat, - bool final = false); + void on_stream_close(int32_t stream_id, bool success, bool final = false); + // Returns RequestStat for |stream_id|. This function must be + // called after on_request(stream_id), and before + // on_stream_close(stream_id, ...). Otherwise, this will return + // nullptr. + RequestStat *get_req_stat(int32_t stream_id); void record_request_time(RequestStat *req_stat); void record_connect_start_time(); diff --git a/src/h2load_http1_session.cc b/src/h2load_http1_session.cc index aa43f90d..a295c625 100644 --- a/src/h2load_http1_session.cc +++ b/src/h2load_http1_session.cc @@ -80,9 +80,11 @@ int htp_msg_completecb(http_parser *htp) { auto client = session->get_client(); auto final = http_should_keep_alive(htp) == 0; - client->on_stream_close(session->stream_resp_counter_, true, - session->req_stats_[session->stream_resp_counter_], - final); + auto req_stat = client->get_req_stat(session->stream_resp_counter_); + + assert(req_stat); + + client->on_stream_close(session->stream_resp_counter_, true, final); session->stream_resp_counter_ += 2; @@ -150,7 +152,7 @@ http_parser_settings htp_hooks = { void Http1Session::on_connect() { client_->signal_write(); } -int Http1Session::submit_request(RequestStat *req_stat) { +int Http1Session::submit_request() { auto config = client_->worker->config; const auto &req = config->h1reqs[client_->reqidx]; client_->reqidx++; @@ -159,13 +161,13 @@ int Http1Session::submit_request(RequestStat *req_stat) { client_->reqidx = 0; } - assert(req_stat); + client_->on_request(stream_req_counter_); + + auto req_stat = client_->get_req_stat(stream_req_counter_); + client_->record_request_time(req_stat); client_->wb.write(req.c_str(), req.size()); - client_->on_request(stream_req_counter_); - req_stats_[stream_req_counter_] = req_stat; - // increment for next request stream_req_counter_ += 2; diff --git a/src/h2load_http1_session.h b/src/h2load_http1_session.h index a19ba65e..1b48c9c4 100644 --- a/src/h2load_http1_session.h +++ b/src/h2load_http1_session.h @@ -38,14 +38,13 @@ public: Http1Session(Client *client); virtual ~Http1Session(); virtual void on_connect(); - virtual int submit_request(RequestStat *req_stat); + virtual int submit_request(); virtual int on_read(const uint8_t *data, size_t len); virtual int on_write(); virtual void terminate(); Client *get_client(); int32_t stream_req_counter_; int32_t stream_resp_counter_; - std::unordered_map req_stats_; private: Client *client_; diff --git a/src/h2load_http2_session.cc b/src/h2load_http2_session.cc index 0bbbe8e8..7883131d 100644 --- a/src/h2load_http2_session.cc +++ b/src/h2load_http2_session.cc @@ -89,12 +89,25 @@ namespace { int on_stream_close_callback(nghttp2_session *session, int32_t stream_id, uint32_t error_code, void *user_data) { auto client = static_cast(user_data); - auto req_stat = static_cast( - nghttp2_session_get_stream_user_data(session, stream_id)); - if (!req_stat) { + client->on_stream_close(stream_id, error_code == NGHTTP2_NO_ERROR); + + return 0; +} +} // namespace + +namespace { +int on_frame_not_send_callback(nghttp2_session *session, + const nghttp2_frame *frame, int lib_error_code, + void *user_data) { + if (frame->hd.type != NGHTTP2_HEADERS || + frame->headers.cat != NGHTTP2_HCAT_REQUEST) { return 0; } - client->on_stream_close(stream_id, error_code == NGHTTP2_NO_ERROR, req_stat); + + auto client = static_cast(user_data); + // request was not sent. Mark it as error. + client->on_stream_close(frame->hd.stream_id, false); + return 0; } } // namespace @@ -108,9 +121,7 @@ int before_frame_send_callback(nghttp2_session *session, } auto client = static_cast(user_data); - client->on_request(frame->hd.stream_id); - auto req_stat = static_cast( - nghttp2_session_get_stream_user_data(session, frame->hd.stream_id)); + auto req_stat = client->get_req_stat(frame->hd.stream_id); assert(req_stat); client->record_request_time(req_stat); @@ -124,8 +135,7 @@ ssize_t file_read_callback(nghttp2_session *session, int32_t stream_id, nghttp2_data_source *source, void *user_data) { auto client = static_cast(user_data); auto config = client->worker->config; - auto req_stat = static_cast( - nghttp2_session_get_stream_user_data(session, stream_id)); + auto req_stat = client->get_req_stat(stream_id); assert(req_stat); ssize_t nread; while ((nread = pread(config->data_fd, buf, length, req_stat->data_offset)) == @@ -183,6 +193,9 @@ void Http2Session::on_connect() { nghttp2_session_callbacks_set_on_header_callback(callbacks, on_header_callback); + nghttp2_session_callbacks_set_on_frame_not_send_callback( + callbacks, on_frame_not_send_callback); + nghttp2_session_callbacks_set_before_frame_send_callback( callbacks, before_frame_send_callback); @@ -212,7 +225,7 @@ void Http2Session::on_connect() { client_->signal_write(); } -int Http2Session::submit_request(RequestStat *req_stat) { +int Http2Session::submit_request() { if (nghttp2_session_check_request_allowed(session_) == 0) { return -1; } @@ -228,11 +241,13 @@ int Http2Session::submit_request(RequestStat *req_stat) { auto stream_id = nghttp2_submit_request(session_, nullptr, nva.data(), nva.size(), - config->data_fd == -1 ? nullptr : &prd, req_stat); + config->data_fd == -1 ? nullptr : &prd, nullptr); if (stream_id < 0) { return -1; } + client_->on_request(stream_id); + return 0; } diff --git a/src/h2load_http2_session.h b/src/h2load_http2_session.h index c3a3344b..2d8b1fa8 100644 --- a/src/h2load_http2_session.h +++ b/src/h2load_http2_session.h @@ -38,7 +38,7 @@ public: Http2Session(Client *client); virtual ~Http2Session(); virtual void on_connect(); - virtual int submit_request(RequestStat *req_stat); + virtual int submit_request(); virtual int on_read(const uint8_t *data, size_t len); virtual int on_write(); virtual void terminate(); diff --git a/src/h2load_session.h b/src/h2load_session.h index 1f8ed60a..17103aa9 100644 --- a/src/h2load_session.h +++ b/src/h2load_session.h @@ -41,7 +41,7 @@ public: // Called when the connection was made. virtual void on_connect() = 0; // Called when one request must be issued. - virtual int submit_request(RequestStat *req_stat) = 0; + virtual int submit_request() = 0; // Called when incoming bytes are available. The subclass has to // return the number of bytes read. virtual int on_read(const uint8_t *data, size_t len) = 0; diff --git a/src/h2load_spdy_session.cc b/src/h2load_spdy_session.cc index a3b85591..4afa13e1 100644 --- a/src/h2load_spdy_session.cc +++ b/src/h2load_spdy_session.cc @@ -48,9 +48,7 @@ void before_ctrl_send_callback(spdylay_session *session, return; } client->on_request(frame->syn_stream.stream_id); - auto req_stat = - static_cast(spdylay_session_get_stream_user_data( - session, frame->syn_stream.stream_id)); + auto req_stat = client->get_req_stat(frame->syn_stream.stream_id); client->record_request_time(req_stat); } } // namespace @@ -104,9 +102,7 @@ void on_stream_close_callback(spdylay_session *session, int32_t stream_id, spdylay_status_code status_code, void *user_data) { auto client = static_cast(user_data); - auto req_stat = static_cast( - spdylay_session_get_stream_user_data(session, stream_id)); - client->on_stream_close(stream_id, status_code == SPDYLAY_OK, req_stat); + client->on_stream_close(stream_id, status_code == SPDYLAY_OK); } } // namespace @@ -130,8 +126,7 @@ ssize_t file_read_callback(spdylay_session *session, int32_t stream_id, spdylay_data_source *source, void *user_data) { auto client = static_cast(user_data); auto config = client->worker->config; - auto req_stat = static_cast( - spdylay_session_get_stream_user_data(session, stream_id)); + auto req_stat = client->get_req_stat(stream_id); ssize_t nread; while ((nread = pread(config->data_fd, buf, length, req_stat->data_offset)) == @@ -185,7 +180,7 @@ void SpdySession::on_connect() { client_->signal_write(); } -int SpdySession::submit_request(RequestStat *req_stat) { +int SpdySession::submit_request() { int rv; auto config = client_->worker->config; auto &nv = config->nv[client_->reqidx++]; @@ -197,7 +192,7 @@ int SpdySession::submit_request(RequestStat *req_stat) { spdylay_data_provider prd{{0}, file_read_callback}; rv = spdylay_submit_request(session_, 0, nv.data(), - config->data_fd == -1 ? nullptr : &prd, req_stat); + config->data_fd == -1 ? nullptr : &prd, nullptr); if (rv != 0) { return -1; diff --git a/src/h2load_spdy_session.h b/src/h2load_spdy_session.h index 288be1df..44fd4d55 100644 --- a/src/h2load_spdy_session.h +++ b/src/h2load_spdy_session.h @@ -40,7 +40,7 @@ public: SpdySession(Client *client, uint16_t spdy_version); virtual ~SpdySession(); virtual void on_connect(); - virtual int submit_request(RequestStat *req_stat); + virtual int submit_request(); virtual int on_read(const uint8_t *data, size_t len); virtual int on_write(); virtual void terminate(); From 7ed26afe7583d6192b6ca7799c9dcdcbbe85612a Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 5 Jan 2016 23:25:09 +0900 Subject: [PATCH 56/66] h2load: Delete Client when it finished --- src/h2load.cc | 39 +++++++++++++++++++-------------------- src/h2load.h | 1 - 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index b1a5edda..6403a783 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -122,12 +122,14 @@ void writecb(struct ev_loop *loop, ev_io *w, int revents) { rv = client->connect(); if (rv != 0) { client->fail(); + delete client; return; } return; } if (rv != 0) { client->fail(); + delete client; } } } // namespace @@ -138,6 +140,7 @@ void readcb(struct ev_loop *loop, ev_io *w, int revents) { client->restart_timeout(); if (client->do_read() != 0) { client->fail(); + delete client; return; } writecb(loop, &client->wev, revents); @@ -159,14 +162,17 @@ void rate_period_timeout_w_cb(struct ev_loop *loop, ev_timer *w, int revents) { ++req_todo; --worker->nreqs_rem; } - worker->clients.push_back( - make_unique(worker->next_client_id++, worker, req_todo)); - auto &client = worker->clients.back(); + auto client = + make_unique(worker->next_client_id++, worker, req_todo); + + ++worker->nconns_made; + if (client->connect() != 0) { std::cerr << "client could not connect to host" << std::endl; client->fail(); + } else { + client.release(); } - ++worker->nconns_made; } if (worker->nconns_made >= worker->nclients) { ev_timer_stop(worker->loop, w); @@ -1071,17 +1077,6 @@ Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, config->rate_period); timeout_watcher.data = this; - if (!config->is_rate_mode()) { - for (size_t i = 0; i < nclients; ++i) { - auto req_todo = nreqs_per_client; - if (nreqs_rem > 0) { - ++req_todo; - --nreqs_rem; - } - clients.push_back(make_unique(next_client_id++, this, req_todo)); - } - } - auto request_times_max_stats = std::min(req_todo, MAX_STATS); request_times_sampling_step = (req_todo + request_times_max_stats - 1) / request_times_max_stats; @@ -1089,19 +1084,23 @@ Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, Worker::~Worker() { ev_timer_stop(loop, &timeout_watcher); - - // first clear clients so that io watchers are stopped before - // destructing ev_loop. - clients.clear(); ev_loop_destroy(loop); } void Worker::run() { if (!config->is_rate_mode()) { - for (auto &client : clients) { + for (size_t i = 0; i < nclients; ++i) { + auto req_todo = nreqs_per_client; + if (nreqs_rem > 0) { + ++req_todo; + --nreqs_rem; + } + auto client = make_unique(next_client_id++, this, req_todo); if (client->connect() != 0) { std::cerr << "client could not connect to host" << std::endl; client->fail(); + } else { + client.release(); } } } else { diff --git a/src/h2load.h b/src/h2load.h index a6366f6d..2c4ad997 100644 --- a/src/h2load.h +++ b/src/h2load.h @@ -209,7 +209,6 @@ enum ClientState { CLIENT_IDLE, CLIENT_CONNECTED }; struct Client; struct Worker { - std::vector> clients; Stats stats; struct ev_loop *loop; SSL_CTX *ssl_ctx; From 23ac0429bedc589633a4b26b012b8108081bfde3 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 5 Jan 2016 23:56:31 +0900 Subject: [PATCH 57/66] h2load: Show progress in rate mode --- src/h2load.cc | 36 +++++++++++++++++++++++++----------- src/h2load.h | 3 ++- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index 6403a783..8c980efb 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -173,6 +173,7 @@ void rate_period_timeout_w_cb(struct ev_loop *loop, ev_timer *w, int revents) { } else { client.release(); } + worker->report_rate_progress(); } if (worker->nconns_made >= worker->nclients) { ev_timer_stop(worker->loop, w); @@ -484,15 +485,6 @@ void Client::process_request_failure() { } } -void Client::report_progress() { - if (!worker->config->is_rate_mode() && worker->id == 0 && - worker->stats.req_done % worker->progress_interval == 0) { - std::cout << "progress: " - << worker->stats.req_done * 100 / worker->stats.req_todo - << "% done" << std::endl; - } -} - namespace { void print_server_tmp_key(SSL *ssl) { // libressl does not have SSL_get_server_tmp_key @@ -647,7 +639,7 @@ void Client::on_stream_close(int32_t stream_id, bool success, bool final) { worker->sample_req_stat(req_stat); } - report_progress(); + worker->report_progress(); streams.erase(stream_id); if (req_done == req_todo) { terminate_session(); @@ -1070,7 +1062,11 @@ Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, nreqs_per_client(req_todo / nclients), nreqs_rem(req_todo % nclients), rate(rate), next_client_id(0) { stats.req_todo = req_todo; - progress_interval = std::max(static_cast(1), req_todo / 10); + if (!config->is_rate_mode()) { + progress_interval = std::max(static_cast(1), req_todo / 10); + } else { + progress_interval = std::max(static_cast(1), nclients / 10); + } // create timer that will go off every rate_period ev_timer_init(&timeout_watcher, rate_period_timeout_w_cb, 0., @@ -1117,6 +1113,24 @@ void Worker::sample_req_stat(RequestStat *req_stat) { assert(stats.req_stats.size() <= MAX_STATS); } +void Worker::report_progress() { + if (id != 0 || config->is_rate_mode() || stats.req_done % progress_interval) { + return; + } + + std::cout << "progress: " << stats.req_done * 100 / stats.req_todo << "% done" + << std::endl; +} + +void Worker::report_rate_progress() { + if (id != 0 || nconns_made % progress_interval) { + return; + } + + std::cout << "progress: " << nconns_made * 100 / nclients + << "% of clients started" << std::endl; +} + namespace { // Returns percentage of number of samples within mean +/- sd. double within_sd(const std::vector &samples, double mean, double sd) { diff --git a/src/h2load.h b/src/h2load.h index 2c4ad997..7910bf66 100644 --- a/src/h2load.h +++ b/src/h2load.h @@ -238,6 +238,8 @@ struct Worker { Worker(Worker &&o) = default; void run(); void sample_req_stat(RequestStat *req_stat); + void report_progress(); + void report_rate_progress(); }; struct Stream { @@ -292,7 +294,6 @@ struct Client { void process_request_failure(); void process_timedout_streams(); void process_abandoned_streams(); - void report_progress(); void report_tls_info(); void report_app_info(); void terminate_session(); From 9cbb8174bb1d2e247713be42129f7eb77a143004 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 6 Jan 2016 22:43:09 +0900 Subject: [PATCH 58/66] h2load: Refactor systematic sampling method --- src/h2load.cc | 62 +++++++++++++++++++++++++++++++++++++++++---------- src/h2load.h | 15 ++++++++++--- 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index 8c980efb..ae9ad2f2 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -44,6 +44,7 @@ #include #include #include +#include #ifdef HAVE_SPDYLAY #include @@ -110,6 +111,42 @@ Stats::Stats(size_t req_todo, size_t nclients) Stream::Stream() : status_success(-1) {} +namespace { +std::random_device rd; +} // namespace + +namespace { +std::mt19937 gen(rd()); +} // namespace + +namespace { +void sampling_init(Sampling &smp, size_t total, size_t max_samples) { + smp.n = 0; + + if (total <= max_samples) { + smp.interval = 0.; + smp.point = 0.; + return; + } + + smp.interval = static_cast(total) / max_samples; + + std::uniform_real_distribution<> dis(0., smp.interval); + + smp.point = dis(gen); +} +} // namespace + +namespace { +bool sampling_should_pick(Sampling &smp) { + return smp.interval == 0. || smp.n == ceil(smp.point); +} +} // namespace + +namespace { +void sampling_advance_point(Sampling &smp) { smp.point += smp.interval; } +} // namespace + namespace { void writecb(struct ev_loop *loop, ev_io *w, int revents) { auto client = static_cast(w->data); @@ -620,24 +657,27 @@ void Client::on_stream_close(int32_t stream_id, bool success, bool final) { ++worker->stats.req_success; auto &cstat = worker->stats.client_stats[id]; ++cstat.req_success; - } - ++worker->stats.req_done; - ++req_done; - if (success) { + if (streams[stream_id].status_success == 1) { ++worker->stats.req_status_success; } else { ++worker->stats.req_failed; } + + if (sampling_should_pick(worker->request_times_smp)) { + sampling_advance_point(worker->request_times_smp); + worker->sample_req_stat(req_stat); + } + + // Count up in successful cases only + ++worker->request_times_smp.n; } else { ++worker->stats.req_failed; ++worker->stats.req_error; } - if (req_stat->completed && - (worker->stats.req_done % worker->request_times_sampling_step) == 0) { - worker->sample_req_stat(req_stat); - } + ++worker->stats.req_done; + ++req_done; worker->report_progress(); streams.erase(stream_id); @@ -1073,9 +1113,7 @@ Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, config->rate_period); timeout_watcher.data = this; - auto request_times_max_stats = std::min(req_todo, MAX_STATS); - request_times_sampling_step = - (req_todo + request_times_max_stats - 1) / request_times_max_stats; + sampling_init(request_times_smp, req_todo, MAX_STATS); } Worker::~Worker() { @@ -1190,7 +1228,7 @@ process_time_stats(const std::vector> &workers) { size_t nrequest_times = 0; for (const auto &w : workers) { nrequest_times += w->stats.req_stats.size(); - if (w->request_times_sampling_step != 1) { + if (w->request_times_smp.interval != 0.) { request_times_sampling = true; } } diff --git a/src/h2load.h b/src/h2load.h index 7910bf66..994c307c 100644 --- a/src/h2load.h +++ b/src/h2load.h @@ -208,8 +208,20 @@ enum ClientState { CLIENT_IDLE, CLIENT_CONNECTED }; struct Client; +// We use systematic sampling method +struct Sampling { + // sampling interval + double interval; + // cumulative value of interval, and the next point is the integer + // rounded up from this value. + double point; + // number of samples seen, including discarded samples. + size_t n; +}; + struct Worker { Stats stats; + Sampling request_times_smp; struct ev_loop *loop; SSL_CTX *ssl_ctx; Config *config; @@ -225,9 +237,6 @@ struct Worker { // at most nreqs_rem clients get an extra request size_t nreqs_rem; size_t rate; - // every successful request_times_sampling_step-th request's - // req_stat will get sampled. - size_t request_times_sampling_step; ev_timer timeout_watcher; // The next client ID this worker assigns uint32_t next_client_id; From a52920cec008190ee66b9bf6afe14747cf26a033 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 6 Jan 2016 23:03:37 +0900 Subject: [PATCH 59/66] h2load: Perform Sampling for client as well --- src/h2load.cc | 27 +++++++++++++++------------ src/h2load.h | 3 +++ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index ae9ad2f2..6beaa5bd 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -105,8 +105,9 @@ Stats::Stats(size_t req_todo, size_t nclients) : req_todo(0), req_started(0), req_done(0), req_success(0), req_status_success(0), req_failed(0), req_error(0), req_timedout(0), bytes_total(0), bytes_head(0), bytes_head_decomp(0), bytes_body(0), - status(), client_stats(nclients) { + status() { req_stats.reserve(std::min(req_todo, MAX_STATS)); + client_stats.reserve(std::min(nclients, MAX_STATS)); } Stream::Stream() : status_success(-1) {} @@ -288,7 +289,7 @@ void client_request_timeout_cb(struct ev_loop *loop, ev_timer *w, int revents) { } // namespace Client::Client(uint32_t id, Worker *worker, size_t req_todo) - : worker(worker), ssl(nullptr), next_addr(config.addrs), + : cstat{}, worker(worker), ssl(nullptr), next_addr(config.addrs), current_addr(nullptr), reqidx(0), state(CLIENT_IDLE), req_todo(req_todo), req_started(0), req_done(0), id(id), fd(-1), new_connection_requested(false) { @@ -316,6 +317,12 @@ Client::~Client() { if (ssl) { SSL_free(ssl); } + + if (sampling_should_pick(worker->client_smp)) { + sampling_advance_point(worker->client_smp); + worker->sample_client_stat(&cstat); + } + ++worker->client_smp.n; } int Client::do_read() { return readfn(*this); } @@ -655,7 +662,6 @@ void Client::on_stream_close(int32_t stream_id, bool success, bool final) { if (success) { req_stat->completed = true; ++worker->stats.req_success; - auto &cstat = worker->stats.client_stats[id]; ++cstat.req_success; if (streams[stream_id].status_success == 1) { @@ -1044,17 +1050,14 @@ void Client::record_request_time(RequestStat *req_stat) { } void Client::record_connect_start_time() { - auto &cstat = worker->stats.client_stats[id]; cstat.connect_start_time = std::chrono::steady_clock::now(); } void Client::record_connect_time() { - auto &cstat = worker->stats.client_stats[id]; cstat.connect_time = std::chrono::steady_clock::now(); } void Client::record_ttfb() { - auto &cstat = worker->stats.client_stats[id]; if (recorded(cstat.ttfb)) { return; } @@ -1063,16 +1066,12 @@ void Client::record_ttfb() { } void Client::clear_connect_times() { - auto &cstat = worker->stats.client_stats[id]; - cstat.connect_start_time = std::chrono::steady_clock::time_point(); cstat.connect_time = std::chrono::steady_clock::time_point(); cstat.ttfb = std::chrono::steady_clock::time_point(); } void Client::record_client_start_time() { - auto &cstat = worker->stats.client_stats[id]; - // Record start time only once at the very first connection is going // to be made. if (recorded(cstat.client_start_time)) { @@ -1083,8 +1082,6 @@ void Client::record_client_start_time() { } void Client::record_client_end_time() { - auto &cstat = worker->stats.client_stats[id]; - // Unlike client_start_time, we overwrite client_end_time. This // handles multiple connect/disconnect for HTTP/1.1 benchmark. cstat.client_end_time = std::chrono::steady_clock::now(); @@ -1114,6 +1111,7 @@ Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, timeout_watcher.data = this; sampling_init(request_times_smp, req_todo, MAX_STATS); + sampling_init(client_smp, nclients, MAX_STATS); } Worker::~Worker() { @@ -1151,6 +1149,11 @@ void Worker::sample_req_stat(RequestStat *req_stat) { assert(stats.req_stats.size() <= MAX_STATS); } +void Worker::sample_client_stat(ClientStat *cstat) { + stats.client_stats.push_back(*cstat); + assert(stats.client_stats.size() <= MAX_STATS); +} + void Worker::report_progress() { if (id != 0 || config->is_rate_mode() || stats.req_done % progress_interval) { return; diff --git a/src/h2load.h b/src/h2load.h index 994c307c..6be57e3c 100644 --- a/src/h2load.h +++ b/src/h2load.h @@ -222,6 +222,7 @@ struct Sampling { struct Worker { Stats stats; Sampling request_times_smp; + Sampling client_smp; struct ev_loop *loop; SSL_CTX *ssl_ctx; Config *config; @@ -247,6 +248,7 @@ struct Worker { Worker(Worker &&o) = default; void run(); void sample_req_stat(RequestStat *req_stat); + void sample_client_stat(ClientStat *cstat); void report_progress(); void report_rate_progress(); }; @@ -259,6 +261,7 @@ struct Stream { struct Client { std::unordered_map streams; + ClientStat cstat; std::unique_ptr session; ev_io wev; ev_io rev; From 13bd566eb714d368c1b6b0e05c1e8d1184cca405 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 6 Jan 2016 23:10:46 +0900 Subject: [PATCH 60/66] h2load: Remove RequestStat ctor --- src/h2load.cc | 4 +--- src/h2load.h | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index 6beaa5bd..b061d870 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -97,8 +97,6 @@ bool Config::is_rate_mode() const { return (this->rate != 0); } bool Config::has_base_uri() const { return (!this->base_uri.empty()); } Config config; -RequestStat::RequestStat() : data_offset(0), completed(false) {} - constexpr size_t MAX_STATS = 1000000; Stats::Stats(size_t req_todo, size_t nclients) @@ -110,7 +108,7 @@ Stats::Stats(size_t req_todo, size_t nclients) client_stats.reserve(std::min(nclients, MAX_STATS)); } -Stream::Stream() : status_success(-1) {} +Stream::Stream() : req_stat{}, status_success(-1) {} namespace { std::random_device rd; diff --git a/src/h2load.h b/src/h2load.h index 6be57e3c..e5b17f0f 100644 --- a/src/h2load.h +++ b/src/h2load.h @@ -112,7 +112,6 @@ struct Config { }; struct RequestStat { - RequestStat(); // time point when request was sent std::chrono::steady_clock::time_point request_time; // time point when stream was closed From acac5ec6ea0306a82722fa108531c57d5f9f55db Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 6 Jan 2016 23:16:53 +0900 Subject: [PATCH 61/66] h2load: Remove "auto" for -m option Because http/1.1 support, default "auto" behaviour of -m option is not desirable, since it is used as HTTP pipelining, and it is not used in practice. --- src/h2load.cc | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index b061d870..2e429411 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -1538,10 +1538,11 @@ Options: URIs, if present, are ignored. Those in the first URI are used solely. Definition of a base URI overrides all scheme, host or port values. - -m, --max-concurrent-streams=(auto|) - Max concurrent streams to issue per session. If "auto" - is given, the number of given URIs is used. - Default: auto + -m, --max-concurrent-streams= + Max concurrent streams to issue per session. When + http/1.1 is used, this specifies the number of HTTP + pipelining requests in-flight. + Default: 1 -w, --window-bits= Sets the stream level initial window size to (2**)-1. For SPDY, 2** is used instead. @@ -1716,11 +1717,7 @@ int main(int argc, char **argv) { #endif // NOTHREADS break; case 'm': - if (util::strieq("auto", optarg)) { - config.max_concurrent_streams = -1; - } else { - config.max_concurrent_streams = strtoul(optarg, nullptr, 10); - } + config.max_concurrent_streams = strtoul(optarg, nullptr, 10); break; case 'w': case 'W': { @@ -1943,11 +1940,6 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } - if (config.max_concurrent_streams == -1) { - config.max_concurrent_streams = reqlines.size(); - } - - assert(config.max_concurrent_streams > 0); if (config.nreqs == 0) { std::cerr << "-n: the number of requests must be strictly greater than 0." << std::endl; From 425c794f89f2eaa7d2b03a9ecf26c33875a1c33a Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 7 Jan 2016 22:21:39 +0900 Subject: [PATCH 62/66] h2load: Rename MAX_STATS as MAX_SAMPLES --- src/h2load.cc | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index 2e429411..128c235f 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -97,15 +97,17 @@ bool Config::is_rate_mode() const { return (this->rate != 0); } bool Config::has_base_uri() const { return (!this->base_uri.empty()); } Config config; -constexpr size_t MAX_STATS = 1000000; +namespace { +constexpr size_t MAX_SAMPLES = 1000000; +} // namespace Stats::Stats(size_t req_todo, size_t nclients) : req_todo(0), req_started(0), req_done(0), req_success(0), req_status_success(0), req_failed(0), req_error(0), req_timedout(0), bytes_total(0), bytes_head(0), bytes_head_decomp(0), bytes_body(0), status() { - req_stats.reserve(std::min(req_todo, MAX_STATS)); - client_stats.reserve(std::min(nclients, MAX_STATS)); + req_stats.reserve(std::min(req_todo, MAX_SAMPLES)); + client_stats.reserve(std::min(nclients, MAX_SAMPLES)); } Stream::Stream() : req_stat{}, status_success(-1) {} @@ -1108,8 +1110,8 @@ Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, config->rate_period); timeout_watcher.data = this; - sampling_init(request_times_smp, req_todo, MAX_STATS); - sampling_init(client_smp, nclients, MAX_STATS); + sampling_init(request_times_smp, req_todo, MAX_SAMPLES); + sampling_init(client_smp, nclients, MAX_SAMPLES); } Worker::~Worker() { @@ -1144,12 +1146,12 @@ void Worker::run() { void Worker::sample_req_stat(RequestStat *req_stat) { stats.req_stats.push_back(*req_stat); - assert(stats.req_stats.size() <= MAX_STATS); + assert(stats.req_stats.size() <= MAX_SAMPLES); } void Worker::sample_client_stat(ClientStat *cstat) { stats.client_stats.push_back(*cstat); - assert(stats.client_stats.size() <= MAX_STATS); + assert(stats.client_stats.size() <= MAX_SAMPLES); } void Worker::report_progress() { From 027256d0b10c51bcc78f8098a70b891c672eb868 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 7 Jan 2016 22:41:37 +0900 Subject: [PATCH 63/66] h2load: Distribute MAX_SAMPLES across workers --- src/h2load.cc | 55 +++++++++++++++++++++++++++++++-------------------- src/h2load.h | 4 +++- 2 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/h2load.cc b/src/h2load.cc index 128c235f..e04700c6 100644 --- a/src/h2load.cc +++ b/src/h2load.cc @@ -102,13 +102,10 @@ constexpr size_t MAX_SAMPLES = 1000000; } // namespace Stats::Stats(size_t req_todo, size_t nclients) - : req_todo(0), req_started(0), req_done(0), req_success(0), + : req_todo(req_todo), req_started(0), req_done(0), req_success(0), req_status_success(0), req_failed(0), req_error(0), req_timedout(0), bytes_total(0), bytes_head(0), bytes_head_decomp(0), bytes_body(0), - status() { - req_stats.reserve(std::min(req_todo, MAX_SAMPLES)); - client_stats.reserve(std::min(nclients, MAX_SAMPLES)); -} + status() {} Stream::Stream() : req_stat{}, status_success(-1) {} @@ -1092,13 +1089,12 @@ void Client::signal_write() { ev_io_start(worker->loop, &wev); } void Client::try_new_connection() { new_connection_requested = true; } Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, - size_t rate, Config *config) + size_t rate, size_t max_samples, Config *config) : stats(req_todo, nclients), loop(ev_loop_new(0)), ssl_ctx(ssl_ctx), config(config), id(id), tls_info_report_done(false), app_info_report_done(false), nconns_made(0), nclients(nclients), nreqs_per_client(req_todo / nclients), nreqs_rem(req_todo % nclients), - rate(rate), next_client_id(0) { - stats.req_todo = req_todo; + rate(rate), max_samples(max_samples), next_client_id(0) { if (!config->is_rate_mode()) { progress_interval = std::max(static_cast(1), req_todo / 10); } else { @@ -1110,8 +1106,11 @@ Worker::Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t req_todo, size_t nclients, config->rate_period); timeout_watcher.data = this; - sampling_init(request_times_smp, req_todo, MAX_SAMPLES); - sampling_init(client_smp, nclients, MAX_SAMPLES); + stats.req_stats.reserve(std::min(req_todo, max_samples)); + stats.client_stats.reserve(std::min(nclients, max_samples)); + + sampling_init(request_times_smp, req_todo, max_samples); + sampling_init(client_smp, nclients, max_samples); } Worker::~Worker() { @@ -1146,12 +1145,12 @@ void Worker::run() { void Worker::sample_req_stat(RequestStat *req_stat) { stats.req_stats.push_back(*req_stat); - assert(stats.req_stats.size() <= MAX_SAMPLES); + assert(stats.req_stats.size() <= max_samples); } void Worker::sample_client_stat(ClientStat *cstat) { stats.client_stats.push_back(*cstat); - assert(stats.client_stats.size() <= MAX_SAMPLES); + assert(stats.client_stats.size() <= max_samples); } void Worker::report_progress() { @@ -1228,21 +1227,28 @@ namespace { SDStats process_time_stats(const std::vector> &workers) { auto request_times_sampling = false; + auto client_times_sampling = false; size_t nrequest_times = 0; + size_t nclient_times = 0; for (const auto &w : workers) { nrequest_times += w->stats.req_stats.size(); if (w->request_times_smp.interval != 0.) { request_times_sampling = true; } + + nclient_times += w->stats.client_stats.size(); + if (w->client_smp.interval != 0.) { + client_times_sampling = true; + } } std::vector request_times; request_times.reserve(nrequest_times); std::vector connect_times, ttfb_times, rps_values; - connect_times.reserve(config.nclients); - ttfb_times.reserve(config.nclients); - rps_values.reserve(config.nclients); + connect_times.reserve(nclient_times); + ttfb_times.reserve(nclient_times); + rps_values.reserve(nclient_times); for (const auto &w : workers) { for (const auto &req_stat : w->stats.req_stats) { @@ -1287,8 +1293,9 @@ process_time_stats(const std::vector> &workers) { } return {compute_time_stat(request_times, request_times_sampling), - compute_time_stat(connect_times), compute_time_stat(ttfb_times), - compute_time_stat(rps_values)}; + compute_time_stat(connect_times, client_times_sampling), + compute_time_stat(ttfb_times, client_times_sampling), + compute_time_stat(rps_values, client_times_sampling)}; } } // namespace @@ -1466,7 +1473,7 @@ void read_script_from_file(std::istream &infile, namespace { std::unique_ptr create_worker(uint32_t id, SSL_CTX *ssl_ctx, size_t nreqs, size_t nclients, - size_t rate) { + size_t rate, size_t max_samples) { std::stringstream rate_report; if (config.is_rate_mode() && nclients > rate) { rate_report << "Up to " << rate << " client(s) will be created every " @@ -1477,7 +1484,8 @@ std::unique_ptr create_worker(uint32_t id, SSL_CTX *ssl_ctx, << " total client(s). " << rate_report.str() << nreqs << " total requests" << std::endl; - return make_unique(id, ssl_ctx, nreqs, nclients, rate, &config); + return make_unique(id, ssl_ctx, nreqs, nclients, rate, max_samples, + &config); } } // namespace @@ -2183,6 +2191,9 @@ int main(int argc, char **argv) { size_t rate_per_thread = config.rate / config.nthreads; ssize_t rate_per_thread_rem = config.rate % config.nthreads; + size_t max_samples_per_thread = + std::max(static_cast(256), MAX_SAMPLES / config.nthreads); + std::mutex mu; std::condition_variable cv; auto ready = false; @@ -2215,7 +2226,8 @@ int main(int argc, char **argv) { } } - workers.push_back(create_worker(i, ssl_ctx, nreqs, nclients, rate)); + workers.push_back(create_worker(i, ssl_ctx, nreqs, nclients, rate, + max_samples_per_thread)); auto &worker = workers.back(); futures.push_back( std::async(std::launch::async, [&worker, &mu, &cv, &ready]() { @@ -2245,7 +2257,8 @@ int main(int argc, char **argv) { auto nreqs = config.timing_script ? config.nreqs * config.nclients : config.nreqs; - workers.push_back(create_worker(0, ssl_ctx, nreqs, nclients, rate)); + workers.push_back( + create_worker(0, ssl_ctx, nreqs, nclients, rate, MAX_SAMPLES)); auto start = std::chrono::steady_clock::now(); diff --git a/src/h2load.h b/src/h2load.h index e5b17f0f..0d95b8cd 100644 --- a/src/h2load.h +++ b/src/h2load.h @@ -237,12 +237,14 @@ struct Worker { // at most nreqs_rem clients get an extra request size_t nreqs_rem; size_t rate; + // maximum number of samples in this worker thread + size_t max_samples; ev_timer timeout_watcher; // The next client ID this worker assigns uint32_t next_client_id; Worker(uint32_t id, SSL_CTX *ssl_ctx, size_t nreq_todo, size_t nclients, - size_t rate, Config *config); + size_t rate, size_t max_samples, Config *config); ~Worker(); Worker(Worker &&o) = default; void run(); From b64fc3ac4964447462b0930f73cbde11c12fe8eb Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 7 Jan 2016 22:51:47 +0900 Subject: [PATCH 64/66] nghttpd: Add --no-content-length option to omit content-length in response --- src/HttpServer.cc | 14 +++++++++----- src/HttpServer.h | 1 + src/nghttpd.cc | 7 +++++++ 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/HttpServer.cc b/src/HttpServer.cc index c331d12c..b1e0b0fe 100644 --- a/src/HttpServer.cc +++ b/src/HttpServer.cc @@ -106,7 +106,7 @@ Config::Config() max_concurrent_streams(100), header_table_size(-1), port(0), verbose(false), daemon(false), verify_client(false), no_tls(false), error_gzip(false), early_response(false), hexdump(false), - echo_upload(false) {} + echo_upload(false), no_content_length(false) {} Config::~Config() {} @@ -873,12 +873,14 @@ int Http2Handler::submit_file_response(const std::string &status, std::string last_modified_str; auto nva = make_array(http2::make_nv_ls(":status", status), http2::make_nv_ll("server", NGHTTPD_SERVER), - http2::make_nv_ls("content-length", content_length), http2::make_nv_ll("cache-control", "max-age=3600"), http2::make_nv_ls("date", sessions_->get_cached_date()), http2::make_nv_ll("", ""), http2::make_nv_ll("", ""), - http2::make_nv_ll("", "")); - size_t nvlen = 5; + http2::make_nv_ll("", ""), http2::make_nv_ll("", "")); + size_t nvlen = 4; + if (!get_config()->no_content_length) { + nva[nvlen++] = http2::make_nv_ls("content-length", content_length); + } if (last_modified != 0) { last_modified_str = util::http_date(last_modified); nva[nvlen++] = http2::make_nv_ls("last-modified", last_modified_str); @@ -1088,7 +1090,9 @@ void prepare_echo_response(Stream *stream, Http2Handler *hd) { Headers headers; headers.emplace_back("nghttpd-response", "echo"); - headers.emplace_back("content-length", util::utos(length)); + if (!hd->get_config()->no_content_length) { + headers.emplace_back("content-length", util::utos(length)); + } hd->submit_response("200", stream->stream_id, headers, &data_prd); } diff --git a/src/HttpServer.h b/src/HttpServer.h index 3c1dc73e..3e889711 100644 --- a/src/HttpServer.h +++ b/src/HttpServer.h @@ -76,6 +76,7 @@ struct Config { bool early_response; bool hexdump; bool echo_upload; + bool no_content_length; Config(); ~Config(); }; diff --git a/src/nghttpd.cc b/src/nghttpd.cc index 58179aea..8add1951 100644 --- a/src/nghttpd.cc +++ b/src/nghttpd.cc @@ -164,6 +164,8 @@ Options: Path to file that contains MIME media types and the extensions that represent them. Default: )" << config.mime_types_file << R"( + --no-content-length + Don't send content-length header field. --version Display version information and exit. -h, --help Display this help and exit. @@ -209,6 +211,7 @@ int main(int argc, char **argv) { {"hexdump", no_argument, &flag, 7}, {"echo-upload", no_argument, &flag, 8}, {"mime-types-file", required_argument, &flag, 9}, + {"no-content-length", no_argument, &flag, 10}, {nullptr, 0, nullptr, 0}}; int option_index = 0; int c = getopt_long(argc, argv, "DVb:c:d:ehm:n:p:va:", long_options, @@ -340,6 +343,10 @@ int main(int argc, char **argv) { mime_types_file_set_manually = true; config.mime_types_file = optarg; break; + case 10: + // no-content-length option + config.no_content_length = true; + break; } break; default: From c58a621fc7ff66f75e181b293e667884b3ee4acf Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 9 Jan 2016 18:26:45 +0900 Subject: [PATCH 65/66] Add LICENSE file to point to COPYING Some people feel uncomfortable when they could not find LICENSE file. --- LICENSE | 1 + 1 file changed, 1 insertion(+) create mode 100644 LICENSE diff --git a/LICENSE b/LICENSE new file mode 100644 index 00000000..45d9408f --- /dev/null +++ b/LICENSE @@ -0,0 +1 @@ +See COPYING From a7ec90506f6de2a86d7fd656aaf43414ccebdc1c Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 9 Jan 2016 18:28:38 +0900 Subject: [PATCH 66/66] Happy new year! --- COPYING | 2 +- doc/conf.py.in | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/COPYING b/COPYING index a43e8375..6e079b80 100644 --- a/COPYING +++ b/COPYING @@ -1,6 +1,6 @@ The MIT License -Copyright (c) 2012, 2014, 2015 Tatsuhiro Tsujikawa +Copyright (c) 2012, 2014, 2015, 2016 Tatsuhiro Tsujikawa Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the diff --git a/doc/conf.py.in b/doc/conf.py.in index 66022335..9c192827 100644 --- a/doc/conf.py.in +++ b/doc/conf.py.in @@ -66,7 +66,7 @@ master_doc = 'index' # General information about the project. project = u'nghttp2' -copyright = u'2012, 2015, Tatsuhiro Tsujikawa' +copyright = u'2012, 2015, 2016, Tatsuhiro Tsujikawa' # The version info for the project you're documenting, acts as replacement for # |version| and |release|, also used in various other places throughout the