diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 3edc763c2113d..85efa2c111024 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -1080,11 +1080,9 @@ void ClientConnectionImpl::onAboveHighWatermark() { } void ClientConnectionImpl::onBelowLowWatermark() { - // This can get called without an active stream/request when upstream decides to do bad things - // such as sending multiple responses to the same request, causing us to close the connection, but - // in doing so go below low watermark. - if (pending_response_.has_value()) { - ASSERT(!pending_response_done_); + // This can get called without an active stream/request when the response completion causes us to + // close the connection, but in doing so go below low watermark. + if (pending_response_.has_value() && !pending_response_done_) { pending_response_.value().encoder_.runLowWatermarkCallbacks(); } } diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 614a634047139..c51c48021f471 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -529,6 +529,11 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { } absl::optional pending_response_; + // TODO(mattklein123): The following bool tracks whether a pending response is complete before + // dispatching callbacks. This is needed so that pending_response_ stays valid during callbacks + // in order to access the stream, but to avoid invoking callbacks that shouldn't be called once + // the response is complete. The existence of this variable is hard to reason about and it should + // be combined with pending_response_ somehow in a follow up cleanup. bool pending_response_done_{true}; // Set true between receiving 100-Continue headers and receiving the spurious onMessageComplete. bool ignore_message_complete_for_100_continue_{}; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index ed90780449d65..0c26f621f9e8c 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1931,6 +1931,37 @@ TEST_F(Http1ClientConnectionImplTest, HighwatermarkMultipleResponses) { ->onUnderlyingConnectionBelowWriteBufferLowWatermark(); } +// Regression test for https://github.com/envoyproxy/envoy/issues/10655. Make sure we correctly +// handle going below low watermark when closing the connection during a completion callback. +TEST_F(Http1ClientConnectionImplTest, LowWatermarkDuringClose) { + initialize(); + + InSequence s; + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + Http::MockStreamCallbacks stream_callbacks; + request_encoder.getStream().addCallbacks(stream_callbacks); + + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + // Fake a call from the underlying Network::Connection and verify the stream is notified. + EXPECT_CALL(stream_callbacks, onAboveWriteBufferHighWatermark()); + static_cast(codec_.get()) + ->onUnderlyingConnectionAboveWriteBufferHighWatermark(); + + EXPECT_CALL(response_decoder, decodeHeaders_(_, true)) + .WillOnce(Invoke([&](ResponseHeaderMapPtr&, bool) { + // Fake a call for going below the low watermark. Make sure no stream callbacks get called. + EXPECT_CALL(stream_callbacks, onBelowWriteBufferLowWatermark()).Times(0); + static_cast(codec_.get()) + ->onUnderlyingConnectionBelowWriteBufferLowWatermark(); + })); + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n"); + codec_->dispatch(response); +} + TEST_F(Http1ServerConnectionImplTest, LargeTrailersRejected) { // Default limit of 60 KiB std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n";