From 497ffc63873b25e57661389fa97489d5674ec6c4 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 20 Jan 2016 11:16:49 +0900 Subject: [PATCH] nghttpx: Change pushed stream's priority By default, as RFC 7540 calls for, pushed stream depends on its associated (parent) stream. There are some situations that this is sub-optimal. For example, if associated stream is HTML, and server is configured to push css and javascript files which are in critical rendering path. Then the default priority scheme is sub-optimal, since browser typically blocks rendering while waiting for critical resources. In this case, it is better to at least give pushed stream the same priority of associated stream, and interleave these streams. This change gives pushed stream the same priority of associated stream if pushed stream has content-type "application/javascript" or "text/css". The pushed stream now depends on the stream which associated stream depends on. We use the same weight of associated stream. --- genheaderfunc.py | 1 + src/http2.cc | 9 ++++ src/http2.h | 1 + src/shrpx.cc | 2 +- src/shrpx_downstream.cc | 8 +++- src/shrpx_downstream.h | 5 +++ src/shrpx_http2_upstream.cc | 88 +++++++++++++++++++++++++++++++++---- src/shrpx_http2_upstream.h | 4 ++ 8 files changed, 107 insertions(+), 11 deletions(-) diff --git a/genheaderfunc.py b/genheaderfunc.py index 543ced50..4f342364 100755 --- a/genheaderfunc.py +++ b/genheaderfunc.py @@ -30,6 +30,7 @@ HEADERS = [ "cache-control", "user-agent", "date", + "content-type", # disallowed h1 headers 'connection', 'keep-alive', diff --git a/src/http2.cc b/src/http2.cc index 1b2fa347..eba7aa54 100644 --- a/src/http2.cc +++ b/src/http2.cc @@ -673,6 +673,15 @@ int lookup_token(const uint8_t *name, size_t namelen) { break; } break; + case 12: + switch (name[11]) { + case 'e': + if (util::streq_l("content-typ", name, 11)) { + return HD_CONTENT_TYPE; + } + break; + } + break; case 13: switch (name[12]) { case 'l': diff --git a/src/http2.h b/src/http2.h index 0706b0ab..80be3fc9 100644 --- a/src/http2.h +++ b/src/http2.h @@ -239,6 +239,7 @@ enum { HD_CACHE_CONTROL, HD_CONNECTION, HD_CONTENT_LENGTH, + HD_CONTENT_TYPE, HD_COOKIE, HD_DATE, HD_EXPECT, diff --git a/src/shrpx.cc b/src/shrpx.cc index 4ce54338..2f464fd7 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -1020,7 +1020,7 @@ void fill_default_config() { downstreamconf.connections_per_host = 8; downstreamconf.request_buffer_size = 16_k; - downstreamconf.response_buffer_size = 16_k; + downstreamconf.response_buffer_size = 128_k; } } diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc index c818f056..0880b0d5 100644 --- a/src/shrpx_downstream.cc +++ b/src/shrpx_downstream.cc @@ -117,7 +117,7 @@ Downstream::Downstream(Upstream *upstream, MemchunkPool *mcpool, request_start_time_(std::chrono::high_resolution_clock::now()), request_buf_(mcpool), response_buf_(mcpool), upstream_(upstream), blocked_link_(nullptr), num_retry_(0), stream_id_(stream_id), - downstream_stream_id_(-1), + assoc_stream_id_(-1), downstream_stream_id_(-1), response_rst_stream_error_code_(NGHTTP2_NO_ERROR), request_state_(INITIAL), response_state_(INITIAL), dispatch_state_(DISPATCH_NONE), upgraded_(false), chunked_request_(false), @@ -897,4 +897,10 @@ DefaultMemchunks Downstream::pop_response_buf() { return std::move(response_buf_); } +void Downstream::set_assoc_stream_id(int32_t stream_id) { + assoc_stream_id_ = stream_id; +} + +int32_t Downstream::get_assoc_stream_id() const { return assoc_stream_id_; } + } // namespace shrpx diff --git a/src/shrpx_downstream.h b/src/shrpx_downstream.h index 7e8c1f45..c5fecae9 100644 --- a/src/shrpx_downstream.h +++ b/src/shrpx_downstream.h @@ -201,6 +201,8 @@ public: Upstream *get_upstream() const; void set_stream_id(int32_t stream_id); int32_t get_stream_id() const; + void set_assoc_stream_id(int32_t stream_id); + int32_t get_assoc_stream_id() const; void pause_read(IOCtrlReason reason); int resume_read(IOCtrlReason reason, size_t consumed); void force_resume_read(); @@ -406,6 +408,9 @@ private: size_t num_retry_; // The stream ID in frontend connection int32_t stream_id_; + // The associated stream ID in frontend connection if this is pushed + // stream. + int32_t assoc_stream_id_; // stream ID in backend connection int32_t downstream_stream_id_; // RST_STREAM error_code from downstream HTTP2 connection diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 9ca24d4f..2f175e65 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -555,18 +555,19 @@ int on_frame_send_callback(nghttp2_session *session, const nghttp2_frame *frame, return 0; } - auto downstream = make_unique(upstream, handler->get_mcpool(), - promised_stream_id); - auto &req = downstream->request(); + auto promised_downstream = make_unique( + upstream, handler->get_mcpool(), promised_stream_id); + auto &req = promised_downstream->request(); // As long as we use nghttp2_session_mem_send(), setting stream // user data here should not fail. This is because this callback // is called just after frame was serialized. So no worries about // hanging Downstream. nghttp2_session_set_stream_user_data(session, promised_stream_id, - downstream.get()); + promised_downstream.get()); - downstream->disable_upstream_rtimer(); + promised_downstream->set_assoc_stream_id(frame->hd.stream_id); + promised_downstream->disable_upstream_rtimer(); req.http_major = 2; req.http_minor = 0; @@ -592,14 +593,14 @@ int on_frame_send_callback(nghttp2_session *session, const nghttp2_frame *frame, nv.flags & NGHTTP2_NV_FLAG_NO_INDEX, token); } - downstream->inspect_http2_request(); + promised_downstream->inspect_http2_request(); - downstream->set_request_state(Downstream::MSG_COMPLETE); + promised_downstream->set_request_state(Downstream::MSG_COMPLETE); // a bit weird but start_downstream() expects that given // downstream is in pending queue. - auto ptr = downstream.get(); - upstream->add_pending_downstream(std::move(downstream)); + auto ptr = promised_downstream.get(); + upstream->add_pending_downstream(std::move(promised_downstream)); #ifdef HAVE_MRUBY auto worker = handler->get_worker(); @@ -1499,6 +1500,13 @@ int Http2Upstream::on_downstream_header_complete(Downstream *downstream) { return 0; } + if (downstream->get_assoc_stream_id() != -1) { + rv = adjust_pushed_stream_priority(downstream); + if (rv != 0) { + return -1; + } + } + http2::copy_headers_to_nva_nocopy(nva, resp.fs.headers()); if (!get_config()->http2_proxy && !get_config()->client_proxy) { @@ -1611,6 +1619,66 @@ int Http2Upstream::on_downstream_body(Downstream *downstream, return 0; } +int Http2Upstream::adjust_pushed_stream_priority(Downstream *downstream) { + int rv; + + // We only change pushed stream. The pushed stream has + // assoc_stream_id which is not -1. + auto assoc_stream_id = downstream->get_assoc_stream_id(); + auto stream_id = downstream->get_stream_id(); + + auto assoc_stream = nghttp2_session_find_stream(session_, assoc_stream_id); + auto stream = nghttp2_session_find_stream(session_, stream_id); + + // By default, downstream depends on assoc_stream. If its + // relationship is changed, then we don't change priority. + if (!assoc_stream || assoc_stream != nghttp2_stream_get_parent(stream)) { + return 0; + } + + // We are going to make stream depend on dep_stream which is the + // parent stream of assoc_stream, if the content-type of stream + // indicates javascript or css. + auto dep_stream = nghttp2_stream_get_parent(assoc_stream); + if (!dep_stream) { + return 0; + } + + const auto &resp = downstream->response(); + auto ct = resp.fs.header(http2::HD_CONTENT_TYPE); + if (!ct) { + return 0; + } + + if (!util::istarts_with_l(ct->value, "application/javascript") && + !util::istarts_with_l(ct->value, "text/css")) { + return 0; + } + + auto dep_stream_id = nghttp2_stream_get_stream_id(dep_stream); + auto weight = nghttp2_stream_get_weight(assoc_stream); + + nghttp2_priority_spec pri_spec; + nghttp2_priority_spec_init(&pri_spec, dep_stream_id, weight, 0); + + rv = nghttp2_session_change_stream_priority(session_, stream_id, &pri_spec); + if (nghttp2_is_fatal(rv)) { + ULOG(FATAL, this) << "nghttp2_session_change_stream_priority() failed: " + << nghttp2_strerror(rv); + return -1; + } + + if (rv == 0) { + if (LOG_ENABLED(INFO)) { + ULOG(INFO, this) << "Changed pushed stream priority: pushed stream(" + << stream_id << ") now depends on stream(" + << dep_stream_id << ") with weight " << weight; + } + } + + return 0; +} + // WARNING: Never call directly or indirectly nghttp2_session_send or // nghttp2_session_recv. These calls may delete downstream. int Http2Upstream::on_downstream_body_complete(Downstream *downstream) { @@ -1953,6 +2021,8 @@ Http2Upstream::on_downstream_push_promise(Downstream *downstream, auto &promised_req = promised_downstream->request(); promised_downstream->set_downstream_stream_id(promised_stream_id); + // Set associated stream in frontend + promised_downstream->set_assoc_stream_id(downstream->get_stream_id()); promised_downstream->disable_upstream_rtimer(); diff --git a/src/shrpx_http2_upstream.h b/src/shrpx_http2_upstream.h index e2527626..0839dc66 100644 --- a/src/shrpx_http2_upstream.h +++ b/src/shrpx_http2_upstream.h @@ -125,6 +125,10 @@ public: void set_pending_data_downstream(Downstream *downstream, size_t n, size_t padlen); + // Changes stream priority of |downstream|, which is assumed to be a + // pushed stream. + int adjust_pushed_stream_priority(Downstream *downstream); + private: WriteBuffer wb_; std::unique_ptr pre_upstream_;