From feb3d1b47812a28cb1450c4df8ad483f34a4a061 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 21 Mar 2016 11:34:09 -0700 Subject: [PATCH 1/8] nghttp: add an --expect-continue option Add a placeholder for the expect-continue option, which will perform an Expect/Continue handshake for DATA uploads. --- src/nghttp.cc | 13 ++++++++++++- src/nghttp.h | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/nghttp.cc b/src/nghttp.cc index d219d1da..56607bc9 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -113,7 +113,8 @@ Config::Config() no_content_length(false), no_dep(false), hexdump(false), - no_push(false) { + no_push(false), + expect_continue(false) { nghttp2_option_new(&http2_option); nghttp2_option_set_peer_max_concurrent_streams(http2_option, peer_max_concurrent_streams); @@ -2505,6 +2506,11 @@ Options: --max-concurrent-streams= The number of concurrent pushed streams this client accepts. + --expect-continue + Perform an Expect/Continue handshake: wait to send DATA + (up to a short timeout) until the server sends a 100 + Continue interim response. This option is ignored unless + combined with the -d option. --version Display version information and exit. -h, --help Display this help and exit. @@ -2556,6 +2562,7 @@ int main(int argc, char **argv) { {"hexdump", no_argument, &flag, 10}, {"no-push", no_argument, &flag, 11}, {"max-concurrent-streams", required_argument, &flag, 12}, + {"expect-continue", no_argument, &flag, 13}, {nullptr, 0, nullptr, 0}}; int option_index = 0; int c = getopt_long(argc, argv, "M:Oab:c:d:gm:np:r:hH:vst:uw:W:", @@ -2753,6 +2760,10 @@ int main(int argc, char **argv) { // max-concurrent-streams option config.max_concurrent_streams = strtoul(optarg, nullptr, 10); break; + case 13: + // expect-continue option + config.expect_continue = true; + break; } break; default: diff --git a/src/nghttp.h b/src/nghttp.h index df4ca685..42bea16e 100644 --- a/src/nghttp.h +++ b/src/nghttp.h @@ -91,6 +91,7 @@ struct Config { bool no_dep; bool hexdump; bool no_push; + bool expect_continue; }; enum class RequestState { INITIAL, ON_REQUEST, ON_RESPONSE, ON_COMPLETE }; From f4c7ebcbcad1881256db2ae19dee0a8f1b44e535 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 21 Mar 2016 14:43:48 -0700 Subject: [PATCH 2/8] nghttp: implement Expect/Continue handshake Requests that expect a 100 Continue will not submit their DATA frames until the server sends the interim response. --- src/nghttp.cc | 46 +++++++++++++++++++++++++++++++++++++++------- src/nghttp.h | 1 + 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/nghttp.cc b/src/nghttp.cc index 56607bc9..99ae4b4c 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -160,7 +160,8 @@ Request::Request(const std::string &uri, const http_parser_url &u, stream_id(-1), status(0), level(level), - expect_final_response(false) { + expect_final_response(false), + expect_continue(false) { http2::init_hdidx(res_hdidx); http2::init_hdidx(req_hdidx); } @@ -360,10 +361,19 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { std::string(4_k, '-')); } } + auto num_initial_headers = build_headers.size(); - if (!config.no_content_length && req->data_prd) { - build_headers.emplace_back("content-length", util::utos(req->data_length)); + + if (req->data_prd) { + if (!config.no_content_length) { + build_headers.emplace_back("content-length", util::utos(req->data_length)); + } + if (config.expect_continue) { + req->expect_continue = true; + build_headers.emplace_back("expect", "100-continue"); + } } + for (auto &kv : headers) { size_t i; for (i = 0; i < num_initial_headers; ++i) { @@ -401,11 +411,21 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { nva.push_back(http2::make_nv_ls("trailer", trailer_names)); } - auto stream_id = - nghttp2_submit_request(client->session, &req->pri_spec, nva.data(), - nva.size(), req->data_prd, req); + int32_t stream_id; + + if (req->expect_continue) { + stream_id = nghttp2_submit_headers(client->session, 0, -1, &req->pri_spec, + nva.data(), nva.size(), req); + } else { + stream_id = + nghttp2_submit_request(client->session, &req->pri_spec, nva.data(), + nva.size(), req->data_prd, req); + } + if (stream_id < 0) { - std::cerr << "[ERROR] nghttp2_submit_request() returned error: " + std::cerr << "[ERROR] nghttp2_submit_" + << (req->expect_continue ? "headers" : "request") + << "() returned error: " << nghttp2_strerror(stream_id) << std::endl; return -1; } @@ -1619,6 +1639,18 @@ void check_response_header(nghttp2_session *session, Request *req) { } if (req->status / 100 == 1) { + if (req->expect_continue && (req->status == 100)) { + int error = nghttp2_submit_data(session, NGHTTP2_FLAG_END_STREAM, + req->stream_id, req->data_prd); + if (error) { + std::cerr << "[ERROR] nghttp2_submit_data() returned error: " + << nghttp2_strerror(error) << std::endl; + nghttp2_submit_rst_stream(session, NGHTTP2_FLAG_NONE, req->stream_id, + NGHTTP2_INTERNAL_ERROR); + return; + } + } + req->expect_final_response = true; req->status = 0; req->res_nva.clear(); diff --git a/src/nghttp.h b/src/nghttp.h index 42bea16e..f8022760 100644 --- a/src/nghttp.h +++ b/src/nghttp.h @@ -157,6 +157,7 @@ struct Request { // used for incoming PUSH_PROMISE http2::HeaderIndex req_hdidx; bool expect_final_response; + bool expect_continue; }; struct SessionTiming { From 1bc5cf5ee42ff772e099bf4d4c53688104f0d376 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Tue, 22 Mar 2016 12:41:26 -0700 Subject: [PATCH 3/8] nghttp: time out on long Expect/Continue waits To deal with servers that don't conform to RFC 7231 (or, potentially, connections with a large round-trip time), don't wait forever for a 100 Continue status to come back. Currently, the timeout is hard-coded to one second. A ContinueTimer encapsulates the handshake timeout logic for a single request. Somewhat counterintuitively, ContinueTimers are owned by the HttpClient instead of the Request object, because their lifetime must be bound to the life of the connection (which is owned by the HttpClient and not the Requests). A Request is associated with its corresponding ContinueTimer through a std::weak_ptr. --- src/nghttp.cc | 81 ++++++++++++++++++++++++++++++++++++++++++--------- src/nghttp.h | 22 +++++++++++++- 2 files changed, 88 insertions(+), 15 deletions(-) diff --git a/src/nghttp.cc b/src/nghttp.cc index 99ae4b4c..fa20521b 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -160,8 +160,7 @@ Request::Request(const std::string &uri, const http_parser_url &u, stream_id(-1), status(0), level(level), - expect_final_response(false), - expect_continue(false) { + expect_final_response(false) { http2::init_hdidx(res_hdidx); http2::init_hdidx(req_hdidx); } @@ -305,6 +304,49 @@ void Request::record_response_end_time() { timing.response_end_time = get_time(); } +namespace { +void continue_timeout_cb(struct ev_loop *loop, ev_timer *w, int revents) { + auto client = static_cast(ev_userdata(loop)); + auto req = static_cast(w->data); + int error; + + error = nghttp2_submit_data(client->session, NGHTTP2_FLAG_END_STREAM, + req->stream_id, req->data_prd); + + if (error) { + std::cerr << "[ERROR] nghttp2_submit_data() returned error: " + << nghttp2_strerror(error) << std::endl; + nghttp2_submit_rst_stream(client->session, NGHTTP2_FLAG_NONE, + req->stream_id, NGHTTP2_INTERNAL_ERROR); + } + + client->signal_write(); +} +} // namespace + +ContinueTimer::ContinueTimer(struct ev_loop *loop, Request *req) + : loop(loop), + req(req) { + ev_timer_init(&timer, continue_timeout_cb, 1., 0.); +} + +ContinueTimer::~ContinueTimer() { + stop(); +} + +void ContinueTimer::start() { + timer.data = req; + ev_timer_start(loop, &timer); +} + +void ContinueTimer::stop() { + ev_timer_stop(loop, &timer); +} + +void ContinueTimer::dispatch_continue() { + ev_feed_event(loop, &timer, 0); +} + namespace { int htp_msg_begincb(http_parser *htp) { if (config.verbose) { @@ -355,6 +397,8 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { {"accept", "*/*"}, {"accept-encoding", "gzip, deflate"}, {"user-agent", "nghttp2/" NGHTTP2_VERSION}}; + bool expect_continue = false; + if (config.continuation) { for (size_t i = 0; i < 6; ++i) { build_headers.emplace_back("continuation-test-" + util::utos(i + 1), @@ -369,7 +413,7 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { build_headers.emplace_back("content-length", util::utos(req->data_length)); } if (config.expect_continue) { - req->expect_continue = true; + expect_continue = true; build_headers.emplace_back("expect", "100-continue"); } } @@ -413,7 +457,7 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { int32_t stream_id; - if (req->expect_continue) { + if (expect_continue) { stream_id = nghttp2_submit_headers(client->session, 0, -1, &req->pri_spec, nva.data(), nva.size(), req); } else { @@ -424,7 +468,7 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { if (stream_id < 0) { std::cerr << "[ERROR] nghttp2_submit_" - << (req->expect_continue ? "headers" : "request") + << (expect_continue ? "headers" : "request") << "() returned error: " << nghttp2_strerror(stream_id) << std::endl; return -1; @@ -435,6 +479,14 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { req->req_nva = std::move(build_headers); + if (expect_continue) { + auto timer = std::make_shared(client->loop, req); + timer->start(); + + req->continue_timer = timer; + client->continue_timers.push_back(timer); + } + return 0; } } // namespace @@ -635,6 +687,8 @@ int HttpClient::initiate_connection() { void HttpClient::disconnect() { state = ClientState::IDLE; + continue_timers.clear(); + ev_timer_stop(loop, &settings_timer); ev_timer_stop(loop, &rt); @@ -1639,15 +1693,11 @@ void check_response_header(nghttp2_session *session, Request *req) { } if (req->status / 100 == 1) { - if (req->expect_continue && (req->status == 100)) { - int error = nghttp2_submit_data(session, NGHTTP2_FLAG_END_STREAM, - req->stream_id, req->data_prd); - if (error) { - std::cerr << "[ERROR] nghttp2_submit_data() returned error: " - << nghttp2_strerror(error) << std::endl; - nghttp2_submit_rst_stream(session, NGHTTP2_FLAG_NONE, req->stream_id, - NGHTTP2_INTERNAL_ERROR); - return; + if (req->status == 100) { + // If the request is waiting for a 100 Continue, complete the handshake. + std::shared_ptr timer = req->continue_timer.lock(); + if (timer) { + timer->dispatch_continue(); } } @@ -2173,7 +2223,10 @@ int communicate( << std::endl; goto fin; } + + ev_set_userdata(loop, &client); ev_run(loop, 0); + ev_set_userdata(loop, nullptr); #ifdef HAVE_JANSSON if (!config.harfile.empty()) { diff --git a/src/nghttp.h b/src/nghttp.h index f8022760..b16b3189 100644 --- a/src/nghttp.h +++ b/src/nghttp.h @@ -110,6 +110,23 @@ struct RequestTiming { RequestTiming() : state(RequestState::INITIAL) {} }; +struct Request; // forward declaration for ContinueTimer + +struct ContinueTimer { + ContinueTimer(struct ev_loop *loop, Request *req); + ~ContinueTimer(); + + void start(); + void stop(); + + // Schedules an immediate run of the continue callback on the loop + void dispatch_continue(); + + struct ev_loop *loop; + Request *req; + ev_timer timer; +}; + struct Request { // For pushed request, |uri| is empty and |u| is zero-cleared. Request(const std::string &uri, const http_parser_url &u, @@ -157,7 +174,8 @@ struct Request { // used for incoming PUSH_PROMISE http2::HeaderIndex req_hdidx; bool expect_final_response; - bool expect_continue; + // only alive if this request is using Expect/Continue + std::weak_ptr continue_timer; }; struct SessionTiming { @@ -266,6 +284,8 @@ struct HttpClient { Buffer<64_k> wb; // SETTINGS payload sent as token68 in HTTP Upgrade std::array settings_payload; + // List of timers for outstanding expect/continue handshakes + std::vector> continue_timers; enum { ERR_CONNECT_FAIL = -100 }; }; From 3b7b6a660eb0a5cdd53b47624ba1d38cfff7f10d Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Wed, 23 Mar 2016 09:09:13 -0700 Subject: [PATCH 4/8] nghttp: prevent ContinueTimer double-invocation If a 100 Continue interim response was received after the continue timeout was reached, dispatch_continue() would force a double submission of DATA frames. This patch prevents dispatch_continue() from doing anything if the timer callback has already been invoked. This makes ContinueTimer a single-shot mechanism, as originally intended. --- src/nghttp.cc | 5 ++++- src/nghttp.h | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/nghttp.cc b/src/nghttp.cc index fa20521b..c950a111 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -344,7 +344,10 @@ void ContinueTimer::stop() { } void ContinueTimer::dispatch_continue() { - ev_feed_event(loop, &timer, 0); + // Only dispatch the timeout callback if it hasn't already been called. + if (ev_is_active(&timer)) { + ev_feed_event(loop, &timer, 0); + } } namespace { diff --git a/src/nghttp.h b/src/nghttp.h index b16b3189..4f454f38 100644 --- a/src/nghttp.h +++ b/src/nghttp.h @@ -119,7 +119,8 @@ struct ContinueTimer { void start(); void stop(); - // Schedules an immediate run of the continue callback on the loop + // Schedules an immediate run of the continue callback on the loop, if the + // callback has not already been run void dispatch_continue(); struct ev_loop *loop; From edb874e659ca3c5d7cc414d8cd3768b989b06706 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 28 Mar 2016 15:24:20 -0700 Subject: [PATCH 5/8] nghttp: move ContinueTimer start to on_frame_send The ContinueTimer could expire before the full HEADERS frame was actually sent. By moving the call to timer->start() to the on_frame_send_callback(), this race is fixed. --- src/nghttp.cc | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/nghttp.cc b/src/nghttp.cc index c950a111..04869ded 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -484,7 +484,6 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { if (expect_continue) { auto timer = std::make_shared(client->loop, req); - timer->start(); req->continue_timer = timer; client->continue_timers.push_back(timer); @@ -1983,6 +1982,35 @@ int before_frame_send_callback(nghttp2_session *session, } // namespace +namespace { +int on_frame_send_callback(nghttp2_session *session, + const nghttp2_frame *frame, + void *user_data) { + if (config.verbose) { + verbose_on_frame_send_callback(session, frame, user_data); + } + + if (frame->hd.type != NGHTTP2_HEADERS || + frame->headers.cat != NGHTTP2_HCAT_REQUEST) { + return 0; + } + + auto req = static_cast( + nghttp2_session_get_stream_user_data(session, frame->hd.stream_id)); + if (!req) { + return 0; + } + + // If this request is using Expect/Continue, start its ContinueTimer. + std::shared_ptr timer = req->continue_timer.lock(); + if (timer) { + timer->start(); + } + + return 0; +} +} // namespace + namespace { int on_frame_not_send_callback(nghttp2_session *session, const nghttp2_frame *frame, int lib_error_code, @@ -2342,9 +2370,6 @@ int run(char **uris, int n) { on_frame_recv_callback2); if (config.verbose) { - nghttp2_session_callbacks_set_on_frame_send_callback( - callbacks, verbose_on_frame_send_callback); - nghttp2_session_callbacks_set_on_invalid_frame_recv_callback( callbacks, verbose_on_invalid_frame_recv_callback); @@ -2364,6 +2389,9 @@ int run(char **uris, int n) { nghttp2_session_callbacks_set_before_frame_send_callback( callbacks, before_frame_send_callback); + nghttp2_session_callbacks_set_on_frame_send_callback( + callbacks, on_frame_send_callback); + nghttp2_session_callbacks_set_on_frame_not_send_callback( callbacks, on_frame_not_send_callback); From aa64e7ad3c749ff27ba224d20c694c8169389c38 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Tue, 29 Mar 2016 12:41:28 -0700 Subject: [PATCH 6/8] nghttp: stop ContinueTimers on response or reset If the stream itself is reset, or the server sends a final response immediately, any Expect/Continue handshake should be cancelled. --- src/nghttp.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/nghttp.cc b/src/nghttp.cc index 04869ded..e3d215ec 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -1708,6 +1708,12 @@ void check_response_header(nghttp2_session *session, Request *req) { req->res_nva.clear(); http2::init_hdidx(req->res_hdidx); return; + } else { + // A final response stops any pending Expect/Continue handshake. + std::shared_ptr timer = req->continue_timer.lock(); + if (timer) { + timer->stop(); + } } if (gzip) { @@ -2044,6 +2050,12 @@ int on_stream_close_callback(nghttp2_session *session, int32_t stream_id, return 0; } + // If this request is using Expect/Continue, stop its ContinueTimer. + std::shared_ptr timer = req->continue_timer.lock(); + if (timer) { + timer->stop(); + } + update_html_parser(client, req, nullptr, 0, 1); ++client->complete; From 4bed7854b59f85ad99f024eb2e3fb3d32924f225 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Tue, 29 Mar 2016 13:11:27 -0700 Subject: [PATCH 7/8] nghttp: move ownership of ContinueTimer to Request Each Request now owns its own (optional) ContinueTimer for Expect/Continue handshakes. This removes the need for shared_ptr/weak_ptr logic. --- src/nghttp.cc | 39 +++++++++++++++------------------------ src/nghttp.h | 7 ++----- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/src/nghttp.cc b/src/nghttp.cc index e3d215ec..78f87f84 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -325,9 +325,9 @@ void continue_timeout_cb(struct ev_loop *loop, ev_timer *w, int revents) { } // namespace ContinueTimer::ContinueTimer(struct ev_loop *loop, Request *req) - : loop(loop), - req(req) { + : loop(loop) { ev_timer_init(&timer, continue_timeout_cb, 1., 0.); + timer.data = req; } ContinueTimer::~ContinueTimer() { @@ -335,7 +335,6 @@ ContinueTimer::~ContinueTimer() { } void ContinueTimer::start() { - timer.data = req; ev_timer_start(loop, &timer); } @@ -483,10 +482,8 @@ int submit_request(HttpClient *client, const Headers &headers, Request *req) { req->req_nva = std::move(build_headers); if (expect_continue) { - auto timer = std::make_shared(client->loop, req); - - req->continue_timer = timer; - client->continue_timers.push_back(timer); + auto timer = make_unique(client->loop, req); + req->continue_timer = std::move(timer); } return 0; @@ -689,7 +686,9 @@ int HttpClient::initiate_connection() { void HttpClient::disconnect() { state = ClientState::IDLE; - continue_timers.clear(); + for (auto req = std::begin(reqvec); req != std::end(reqvec); ++req) { + (*req)->continue_timer->stop(); + } ev_timer_stop(loop, &settings_timer); @@ -1695,12 +1694,9 @@ void check_response_header(nghttp2_session *session, Request *req) { } if (req->status / 100 == 1) { - if (req->status == 100) { + if (req->continue_timer && (req->status == 100)) { // If the request is waiting for a 100 Continue, complete the handshake. - std::shared_ptr timer = req->continue_timer.lock(); - if (timer) { - timer->dispatch_continue(); - } + req->continue_timer->dispatch_continue(); } req->expect_final_response = true; @@ -1708,12 +1704,9 @@ void check_response_header(nghttp2_session *session, Request *req) { req->res_nva.clear(); http2::init_hdidx(req->res_hdidx); return; - } else { + } else if (req->continue_timer) { // A final response stops any pending Expect/Continue handshake. - std::shared_ptr timer = req->continue_timer.lock(); - if (timer) { - timer->stop(); - } + req->continue_timer->stop(); } if (gzip) { @@ -2008,9 +2001,8 @@ int on_frame_send_callback(nghttp2_session *session, } // If this request is using Expect/Continue, start its ContinueTimer. - std::shared_ptr timer = req->continue_timer.lock(); - if (timer) { - timer->start(); + if (req->continue_timer) { + req->continue_timer->start(); } return 0; @@ -2051,9 +2043,8 @@ int on_stream_close_callback(nghttp2_session *session, int32_t stream_id, } // If this request is using Expect/Continue, stop its ContinueTimer. - std::shared_ptr timer = req->continue_timer.lock(); - if (timer) { - timer->stop(); + if (req->continue_timer) { + req->continue_timer->stop(); } update_html_parser(client, req, nullptr, 0, 1); diff --git a/src/nghttp.h b/src/nghttp.h index 4f454f38..fd433b84 100644 --- a/src/nghttp.h +++ b/src/nghttp.h @@ -124,7 +124,6 @@ struct ContinueTimer { void dispatch_continue(); struct ev_loop *loop; - Request *req; ev_timer timer; }; @@ -175,8 +174,8 @@ struct Request { // used for incoming PUSH_PROMISE http2::HeaderIndex req_hdidx; bool expect_final_response; - // only alive if this request is using Expect/Continue - std::weak_ptr continue_timer; + // only assigned if this request is using Expect/Continue + std::unique_ptr continue_timer; }; struct SessionTiming { @@ -285,8 +284,6 @@ struct HttpClient { Buffer<64_k> wb; // SETTINGS payload sent as token68 in HTTP Upgrade std::array settings_payload; - // List of timers for outstanding expect/continue handshakes - std::vector> continue_timers; enum { ERR_CONNECT_FAIL = -100 }; }; From dfdeeb3815b2760ad27498dcba5a5e3da51f750d Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Tue, 29 Mar 2016 16:02:10 -0700 Subject: [PATCH 8/8] nghttp: only stop ContinueTimers if they exist Fix a crash on disconnect if --expect-continue isn't actually in use. --- src/nghttp.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/nghttp.cc b/src/nghttp.cc index 78f87f84..c6d1b3fa 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -687,7 +687,9 @@ void HttpClient::disconnect() { state = ClientState::IDLE; for (auto req = std::begin(reqvec); req != std::end(reqvec); ++req) { - (*req)->continue_timer->stop(); + if ((*req)->continue_timer) { + (*req)->continue_timer->stop(); + } } ev_timer_stop(loop, &settings_timer);