http: delaying filter stack creation until full headers have been received (#3574)

alyssawilk · web-flow · commit 1d14b9e46cf6 · 2018-06-11T14:15:08.000-04:00
This is a precursor to creating a custom filter stack for websocket vs HTTP. Risk Level: High (All of the integration test pass. I am still wary) Testing: existing tests pass Docs Changes: n/a Release Notes: not included Part of #3301 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -189,11 +189,6 @@ StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder)
   new_stream->response_encoder_ = &response_encoder;
   new_stream->response_encoder_->getStream().addCallbacks(*new_stream);
   new_stream->buffer_limit_ = new_stream->response_encoder_->getStream().bufferLimit();
-  config_.filterFactory().createFilterChain(*new_stream);
-  // Make sure new streams are apprised that the underlying connection is blocked.
-  if (read_callbacks_->connection().aboveHighWatermark()) {
-    new_stream->callHighWatermarkCallbacks();
-  }
   new_stream->moveIntoList(std::move(new_stream), streams_);
   return **streams_.begin();
 }
@@ -445,8 +440,10 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() {
 }
 
 void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) {
-  maybeEndDecode(end_stream);
   request_headers_ = std::move(headers);
+  createFilterChain();
+
+  maybeEndDecode(end_stream);
 
   ENVOY_STREAM_LOG(debug, "request headers complete (end_stream={}):\n{}", *this, end_stream,
                    *request_headers_);
@@ -1114,6 +1111,14 @@ void ConnectionManagerImpl::ActiveStream::setBufferLimit(uint32_t new_limit) {
   }
 }
 
+void ConnectionManagerImpl::ActiveStream::createFilterChain() {
+  connection_manager_.config_.filterFactory().createFilterChain(*this);
+  // Make sure new streams are apprised that the underlying connection is blocked.
+  if (connection_manager_.read_callbacks_->connection().aboveHighWatermark()) {
+    callHighWatermarkCallbacks();
+  }
+}
+
 void ConnectionManagerImpl::ActiveStreamFilterBase::commonContinue() {
   // TODO(mattklein123): Raise an error if this is called during a callback.
   if (!canContinue()) {
diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h
@@ -360,6 +360,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
 
     // Possibly increases buffer_limit_ to the value of limit.
     void setBufferLimit(uint32_t limit);
+    // Set up the Encoder/Decoder filter chain.
+    void createFilterChain();
 
     ConnectionManagerImpl& connection_manager_;
     Router::ConfigConstSharedPtr snapped_route_config_;
diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc
@@ -1712,14 +1712,13 @@ TEST_F(HttpConnectionManagerImplTest, DownstreamDisconnect) {
     data.drain(2);
   }));
 
-  setupFilterChain(1, 0);
+  EXPECT_CALL(filter_factory_, createFilterChain(_)).Times(0);
 
   // Kick off the incoming data.
   Buffer::OwnedImpl fake_input("1234");
   conn_manager_->onData(fake_input, false);
 
   // Now raise a remote disconnection, we should see the filter get reset called.
-  EXPECT_CALL(*decoder_filters_[0], onDestroy());
   conn_manager_->onEvent(Network::ConnectionEvent::RemoteClose);
 }
 
@@ -1732,10 +1731,9 @@ TEST_F(HttpConnectionManagerImplTest, DownstreamProtocolError) {
     throw CodecProtocolException("protocol error");
   }));
 
-  setupFilterChain(1, 0);
+  EXPECT_CALL(filter_factory_, createFilterChain(_)).Times(0);
 
   // A protocol exception should result in reset of the streams followed by a local close.
-  EXPECT_CALL(*decoder_filters_[0], onDestroy());
   EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite));
 
   // Kick off the incoming data.
