diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 5ba15b0fd77b..47c623e1a332 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -155,9 +155,16 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data) { try { codec_->dispatch(data); } catch (const CodecProtocolException& e) { - // HTTP/1.1 codec has already sent a 400 response if possible. + // HTTP/1.1 codec has already sent a 400 response if possible. HTTP/2 codec has already sent + // GOAWAY. conn_log_debug("dispatch error: {}", read_callbacks_->connection(), e.what()); config_.stats().named_.downstream_cx_protocol_error_.inc(); + + // In the protocol error case, we need to reset all streams now. Since we do a flush write, + // the connection might stick around long enough for a pending stream to come back and try + // to encode. + resetAllStreams(); + read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); return Network::FilterStatus::StopIteration; } @@ -192,6 +199,13 @@ void ConnectionManagerImpl::onBufferChange(Network::ConnectionBufferType type, u config_.stats().named_.downstream_cx_tx_bytes_buffered_); } +void ConnectionManagerImpl::resetAllStreams() { + while (!streams_.empty()) { + // Mimic a downstream reset in this case. + streams_.front()->onResetStream(StreamResetReason::ConnectionTermination); + } +} + void ConnectionManagerImpl::onEvent(uint32_t events) { if (events & Network::ConnectionEvent::LocalClose) { config_.stats().named_.downstream_cx_destroy_local_.inc(); @@ -224,11 +238,7 @@ void ConnectionManagerImpl::onEvent(uint32_t events) { config_.stats().named_.downstream_cx_destroy_active_rq_.inc(); user_agent_.onConnectionDestroy(events, true); - - while (!streams_.empty()) { - // Mimic a downstream reset in this case. - streams_.front()->onResetStream(StreamResetReason::ConnectionTermination); - } + resetAllStreams(); } } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index b60936610d5e..fb2c9a0a78ef 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -389,6 +389,7 @@ class ConnectionManagerImpl : Logger::Loggable, */ void destroyStream(ActiveStream& stream); + void resetAllStreams(); void onIdleTimeout(); void onDrainTimeout(); void startDrainSequence(); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index ad7d91645dc4..6bf710250ac3 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -362,11 +362,11 @@ TEST_F(HttpConnectionManagerImplTest, DownstreamProtocolError) { callbacks.addStreamDecoderFilter(Http::StreamDecoderFilterPtr{filter}); })); - // A protocol exception should result in a local close followed by reset of the streams. + // A protocol exception should result in reset of the streams followed by a local close. Sequence s; + EXPECT_CALL(filter->reset_stream_called_, ready()).InSequence(s); EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)) .InSequence(s); - EXPECT_CALL(filter->reset_stream_called_, ready()).InSequence(s); NiceMock encoder; EXPECT_CALL(*codec_, dispatch(_))