From 1bc5cf5ee42ff772e099bf4d4c53688104f0d376 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Tue, 22 Mar 2016 12:41:26 -0700 Subject: [PATCH] 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 }; };