From 8b1c785348d51002fb7c6887520eb8bd6dbc6d1f Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 7 Jun 2018 16:59:28 -0400 Subject: [PATCH 1/2] Delaying filter stack creation until full headers have been received Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 21 +++++++++++++++------ source/common/http/conn_manager_impl.h | 3 +++ test/common/http/conn_manager_impl_test.cc | 6 ++---- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 6b47b3d8bf2fa..55461b6432716 100644 --- 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); + lazilyCreateFilterChain(); + + maybeEndDecode(end_stream); ENVOY_STREAM_LOG(debug, "request headers complete (end_stream={}):\n{}", *this, end_stream, *request_headers_); @@ -681,6 +678,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte } void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, bool end_stream) { + ASSERT(filter_chain_created_); maybeEndDecode(end_stream); request_info_.bytes_received_ += data.length(); @@ -1114,6 +1112,17 @@ void ConnectionManagerImpl::ActiveStream::setBufferLimit(uint32_t new_limit) { } } +void ConnectionManagerImpl::ActiveStream::lazilyCreateFilterChain() { + if (!filter_chain_created_) { + 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(); + } + filter_chain_created_ = true; + } +} + 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 index 9cefbfd254ef4..51bf4a7be9d97 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -360,6 +360,8 @@ class ConnectionManagerImpl : Logger::Loggable, // Possibly increases buffer_limit_ to the value of limit. void setBufferLimit(uint32_t limit); + // Set up the Encoder/Decoder filter chain if it doesn't already exist. + void lazilyCreateFilterChain(); ConnectionManagerImpl& connection_manager_; Router::ConfigConstSharedPtr snapped_route_config_; @@ -387,6 +389,7 @@ class ConnectionManagerImpl : Logger::Loggable, // By default, we will assume there are no 100-Continue headers. If encode100ContinueHeaders // is ever called, this is set to true so commonContinue resumes processing the 100-Continue. bool has_continue_headers_{}; + bool filter_chain_created_{}; }; typedef std::unique_ptr ActiveStreamPtr; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 50b7761e66a62..21c7343626ec1 100644 --- 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. From 1d2facb62cd866249f8882013e6d78f639827a4b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 11 Jun 2018 12:09:17 -0400 Subject: [PATCH 2/2] Making the code less lazy? Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 16 ++++++---------- source/common/http/conn_manager_impl.h | 5 ++--- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 55461b6432716..a0f5d184785ea 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -441,7 +441,7 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() { void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { request_headers_ = std::move(headers); - lazilyCreateFilterChain(); + createFilterChain(); maybeEndDecode(end_stream); @@ -678,7 +678,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte } void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, bool end_stream) { - ASSERT(filter_chain_created_); maybeEndDecode(end_stream); request_info_.bytes_received_ += data.length(); @@ -1112,14 +1111,11 @@ void ConnectionManagerImpl::ActiveStream::setBufferLimit(uint32_t new_limit) { } } -void ConnectionManagerImpl::ActiveStream::lazilyCreateFilterChain() { - if (!filter_chain_created_) { - 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(); - } - filter_chain_created_ = true; +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(); } } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 51bf4a7be9d97..2bfa88ceeb295 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -360,8 +360,8 @@ class ConnectionManagerImpl : Logger::Loggable, // Possibly increases buffer_limit_ to the value of limit. void setBufferLimit(uint32_t limit); - // Set up the Encoder/Decoder filter chain if it doesn't already exist. - void lazilyCreateFilterChain(); + // Set up the Encoder/Decoder filter chain. + void createFilterChain(); ConnectionManagerImpl& connection_manager_; Router::ConfigConstSharedPtr snapped_route_config_; @@ -389,7 +389,6 @@ class ConnectionManagerImpl : Logger::Loggable, // By default, we will assume there are no 100-Continue headers. If encode100ContinueHeaders // is ever called, this is set to true so commonContinue resumes processing the 100-Continue. bool has_continue_headers_{}; - bool filter_chain_created_{}; }; typedef std::unique_ptr ActiveStreamPtr;