From 5f654609447074902d19f15e7ffc1e0890677df5 Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Sat, 20 Aug 2016 22:09:18 +0900
Subject: [PATCH] nghttpx: Don't change pushed stream's priority

There is a discussion in httpbis mailing list which argues that
dependency tree is for client, and changing it in server side is not
what client expects.
https://lists.w3.org/Archives/Public/ietf-http-wg/2016JulSep/0416.html

Currently, we make pushed stream depend on the parent stream of
associated stream (that is main HTML in most of the cases), so that
associated stream and pushed stream become siblings.  In this case, we
also observed that these resources complete each other to get its
parent weight.  This means that the delivery of associated stream is
delayed by pushed streams.

So at this moment, it is not a good idea to change pushed stream
priority in a way we do currently.
---
 src/shrpx_http2_upstream.cc | 69 -------------------------------------
 src/shrpx_http2_upstream.h  |  4 ---
 2 files changed, 73 deletions(-)

diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc
index 83af04fa..8801deeb 100644
--- a/src/shrpx_http2_upstream.cc
+++ b/src/shrpx_http2_upstream.cc
@@ -1503,13 +1503,6 @@ 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) {
@@ -1605,68 +1598,6 @@ 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") &&
-      // for polymer...
-      !util::istarts_with_l(ct->value, "text/html")) {
-    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) {
diff --git a/src/shrpx_http2_upstream.h b/src/shrpx_http2_upstream.h
index 80585031..b7bf628a 100644
--- a/src/shrpx_http2_upstream.h
+++ b/src/shrpx_http2_upstream.h
@@ -118,10 +118,6 @@ public:
 
   DefaultMemchunks *get_response_buf();
 
-  // Changes stream priority of |downstream|, which is assumed to be a
-  // pushed stream.
-  int adjust_pushed_stream_priority(Downstream *downstream);
-
 private:
   DefaultMemchunks wb_;
   std::unique_ptr<HttpsUpstream> pre_upstream_;