From cae252a465ee35329af3433d210bba4ed71722ce Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Mon, 19 Jun 2017 16:02:52 -0700 Subject: [PATCH 1/2] http/2: fix deferred reset behavior The logic was broken and would not invoke the deferred reset stream cleanup logic in all cases. This now does that and the test coverage has been improved for both the client and server case. Fixes https://github.com/lyft/envoy/issues/1138 --- source/common/http/http2/codec_impl.cc | 7 +- test/common/http/http2/codec_impl_test.cc | 102 +++++++++++++++++----- 2 files changed, 87 insertions(+), 22 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index aa51a379f93a..c21da5514d15 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -206,10 +206,15 @@ void ConnectionImpl::StreamImpl::resetStream(StreamResetReason reason) { if (local_end_stream_ && !local_end_stream_sent_) { parent_.pending_deferred_reset_ = true; deferred_reset_.value(reason); + ENVOY_CONN_LOG(trace, "deferred reset stream", parent_.connection_); } else { resetStreamWorker(reason); - parent_.sendPendingFrames(); } + + // We must still call sendPendingFrames() in both the deferred and not deferred path. This forces + // the cleanup logic to run which will reset the stream in all cases if all data frames could not + // be sent. + parent_.sendPendingFrames(); } void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) { diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index b8d5df88db96..bf0e585b5a4b 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -15,14 +15,14 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -namespace Envoy { - using testing::_; using testing::AtLeast; using testing::InSequence; using testing::Invoke; +using testing::InvokeWithoutArgs; using testing::NiceMock; +namespace Envoy { namespace Http { namespace Http2 { @@ -65,11 +65,14 @@ class Http2CodecImplTest : public testing::TestWithParam server_(server_connection_, server_callbacks_, stats_store_, server_http2settings_), request_encoder_(client_.newStream(response_decoder_)) { setupDefaultConnectionMocks(); + expectServerStream(); + } + void expectServerStream() { EXPECT_CALL(server_callbacks_, newStream(_)) .WillOnce(Invoke([&](StreamEncoder& encoder) -> StreamDecoder& { response_encoder_ = &encoder; - encoder.getStream().addCallbacks(callbacks_); + encoder.getStream().addCallbacks(server_stream_callbacks_); return request_decoder_; })); } @@ -98,7 +101,7 @@ class Http2CodecImplTest : public testing::TestWithParam StreamEncoder& request_encoder_; MockStreamDecoder request_decoder_; StreamEncoder* response_encoder_{}; - MockStreamCallbacks callbacks_; + MockStreamCallbacks server_stream_callbacks_; }; TEST_P(Http2CodecImplTest, ExpectContinueHeadersOnlyResponse) { @@ -168,7 +171,7 @@ TEST_P(Http2CodecImplTest, RefusedStreamReset) { MockStreamCallbacks callbacks; request_encoder_.getStream().addCallbacks(callbacks); - EXPECT_CALL(callbacks_, onResetStream(StreamResetReason::LocalRefusedStreamReset)); + EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalRefusedStreamReset)); EXPECT_CALL(callbacks, onResetStream(StreamResetReason::RemoteRefusedStreamReset)); response_encoder_->getStream().resetStream(StreamResetReason::LocalRefusedStreamReset); } @@ -232,42 +235,99 @@ TEST_P(Http2CodecImplTest, TrailingHeadersLargeBody) { response_encoder_->encodeTrailers(TestHeaderMapImpl{{"trailing", "header"}}); } -TEST_P(Http2CodecImplTest, DeferredReset) { +class Http2CodecImplDeferredResetTest : public Http2CodecImplTest {}; + +TEST_P(Http2CodecImplDeferredResetTest, DeferredResetClient) { InSequence s; - // Buffer server data so we can make sure we don't get any window updates. + // Do a basic request/response to flush settings. + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_.encodeHeaders(request_headers, true); + TestHeaderMapImpl response_headers{{":status", "200"}}; + EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); + response_encoder_->encodeHeaders(response_headers, true); + + // Do a 2nd request, but pause server dispatch so we don't send window updates. This will + // result in a deferred reset, followed by a flush which will cause the stream to actually + // be reset. ON_CALL(client_connection_, write(_)) .WillByDefault( Invoke([&](Buffer::Instance& data) -> void { server_wrapper_.buffer_.add(data); })); + StreamEncoder& request_encoder2 = client_.newStream(response_decoder_); + request_encoder2.encodeHeaders(request_headers, false); + Buffer::OwnedImpl body(std::string(1024 * 1024, 'a')); + request_encoder2.encodeData(body, true); + request_encoder2.getStream().resetStream(StreamResetReason::LocalReset); + + // Dispatch server. We expect to see some data. + expectServerStream(); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)) + .WillOnce(InvokeWithoutArgs([&]() -> void { + // Start a response inside the headers callback. This should not result in the client + // seeing any headers as the stream should already be reset on the other side. + TestHeaderMapImpl response_headers{{":status", "200"}}; + response_encoder_->encodeHeaders(response_headers, false); + })); + EXPECT_CALL(request_decoder_, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::RemoteReset)); + + setupDefaultConnectionMocks(); + server_wrapper_.dispatch(Buffer::OwnedImpl(), server_); +} + +TEST_P(Http2CodecImplDeferredResetTest, DeferredResetServer) { + InSequence s; - // This will send a request that is bigger than the initial window size. When we then reset it, - // after attempting to flush we should reset the stream and drop the data we could not send. TestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); request_encoder_.encodeHeaders(request_headers, false); + + // In this case we do the same thing as DeferredResetClient but on the server side. + ON_CALL(server_connection_, write(_)) + .WillByDefault( + Invoke([&](Buffer::Instance& data) -> void { client_wrapper_.buffer_.add(data); })); + TestHeaderMapImpl response_headers{{":status", "200"}}; + response_encoder_->encodeHeaders(response_headers, false); Buffer::OwnedImpl body(std::string(1024 * 1024, 'a')); - request_encoder_.encodeData(body, true); - request_encoder_.getStream().resetStream(StreamResetReason::LocalReset); + response_encoder_->encodeData(body, true); + EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalReset)); + response_encoder_->getStream().resetStream(StreamResetReason::LocalReset); - EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)); - EXPECT_CALL(request_decoder_, decodeData(_, false)).Times(AtLeast(1)); - EXPECT_CALL(callbacks_, onResetStream(StreamResetReason::RemoteReset)); - server_wrapper_.dispatch(Buffer::OwnedImpl(), server_); + MockStreamCallbacks client_stream_callbacks; + request_encoder_.getStream().addCallbacks(client_stream_callbacks); + EXPECT_CALL(response_decoder_, decodeHeaders_(_, false)); + EXPECT_CALL(response_decoder_, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(client_stream_callbacks, onResetStream(StreamResetReason::RemoteReset)); + setupDefaultConnectionMocks(); + client_wrapper_.dispatch(Buffer::OwnedImpl(), client_); } -// we seperate default/edge cases here to avoid combinatorial explosion +// Deferred reset tests use only small windows so that we can test certain conditions. +#define HTTP2SETTINGS_DEFERRED_RESET_COMBINE \ + ::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \ + ::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \ + ::testing::Values(Http2Settings::MIN_INITIAL_STREAM_WINDOW_SIZE), \ + ::testing::Values(Http2Settings::MIN_INITIAL_CONNECTION_WINDOW_SIZE)) -#define HTTP2SETTINGS_DEFAULT_COMBIME \ +INSTANTIATE_TEST_CASE_P(Http2CodecImplDeferredResetTest, Http2CodecImplDeferredResetTest, + ::testing::Combine(HTTP2SETTINGS_DEFERRED_RESET_COMBINE, + HTTP2SETTINGS_DEFERRED_RESET_COMBINE)); + +// we seperate default/edge cases here to avoid combinatorial explosion +#define HTTP2SETTINGS_DEFAULT_COMBINE \ ::testing::Combine(::testing::Values(Http2Settings::DEFAULT_HPACK_TABLE_SIZE), \ ::testing::Values(Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS), \ ::testing::Values(Http2Settings::DEFAULT_INITIAL_STREAM_WINDOW_SIZE), \ ::testing::Values(Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE)) INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest, - ::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBIME, - HTTP2SETTINGS_DEFAULT_COMBIME)); + ::testing::Combine(HTTP2SETTINGS_DEFAULT_COMBINE, + HTTP2SETTINGS_DEFAULT_COMBINE)); -#define HTTP2SETTINGS_EDGE_COMBIME \ +#define HTTP2SETTINGS_EDGE_COMBINE \ ::testing::Combine( \ ::testing::Values(Http2Settings::MIN_HPACK_TABLE_SIZE, Http2Settings::MAX_HPACK_TABLE_SIZE), \ ::testing::Values(Http2Settings::MIN_MAX_CONCURRENT_STREAMS, \ @@ -278,7 +338,7 @@ INSTANTIATE_TEST_CASE_P(Http2CodecImplTestDefaultSettings, Http2CodecImplTest, Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE)) INSTANTIATE_TEST_CASE_P(Http2CodecImplTestEdgeSettings, Http2CodecImplTest, - ::testing::Combine(HTTP2SETTINGS_EDGE_COMBIME, HTTP2SETTINGS_EDGE_COMBIME)); + ::testing::Combine(HTTP2SETTINGS_EDGE_COMBINE, HTTP2SETTINGS_EDGE_COMBINE)); TEST(Http2CodecUtility, reconstituteCrumbledCookies) { { From 9236e69eca8c905945dfd21bb6b70219e7d06c69 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Tue, 20 Jun 2017 09:40:30 -0700 Subject: [PATCH 2/2] comments --- test/common/http/http2/codec_impl_test.cc | 34 ++++++++++------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index bf0e585b5a4b..78de060fd33a 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -65,10 +65,7 @@ class Http2CodecImplTest : public testing::TestWithParam server_(server_connection_, server_callbacks_, stats_store_, server_http2settings_), request_encoder_(client_.newStream(response_decoder_)) { setupDefaultConnectionMocks(); - expectServerStream(); - } - void expectServerStream() { EXPECT_CALL(server_callbacks_, newStream(_)) .WillOnce(Invoke([&](StreamEncoder& encoder) -> StreamDecoder& { response_encoder_ = &encoder; @@ -240,33 +237,30 @@ class Http2CodecImplDeferredResetTest : public Http2CodecImplTest {}; TEST_P(Http2CodecImplDeferredResetTest, DeferredResetClient) { InSequence s; - // Do a basic request/response to flush settings. - TestHeaderMapImpl request_headers; - HttpTestUtility::addDefaultHeaders(request_headers); - EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); - request_encoder_.encodeHeaders(request_headers, true); - TestHeaderMapImpl response_headers{{":status", "200"}}; - EXPECT_CALL(response_decoder_, decodeHeaders_(_, true)); - response_encoder_->encodeHeaders(response_headers, true); + MockStreamCallbacks client_stream_callbacks; + request_encoder_.getStream().addCallbacks(client_stream_callbacks); - // Do a 2nd request, but pause server dispatch so we don't send window updates. This will - // result in a deferred reset, followed by a flush which will cause the stream to actually - // be reset. + // Do a request, but pause server dispatch so we don't send window updates. This will result in a + // deferred reset, followed by a pending frames flush which will cause the stream to actually + // be reset immediately since we are outside of dispatch context. ON_CALL(client_connection_, write(_)) .WillByDefault( Invoke([&](Buffer::Instance& data) -> void { server_wrapper_.buffer_.add(data); })); - StreamEncoder& request_encoder2 = client_.newStream(response_decoder_); - request_encoder2.encodeHeaders(request_headers, false); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + request_encoder_.encodeHeaders(request_headers, false); Buffer::OwnedImpl body(std::string(1024 * 1024, 'a')); - request_encoder2.encodeData(body, true); - request_encoder2.getStream().resetStream(StreamResetReason::LocalReset); + request_encoder_.encodeData(body, true); + EXPECT_CALL(client_stream_callbacks, onResetStream(StreamResetReason::LocalReset)); + request_encoder_.getStream().resetStream(StreamResetReason::LocalReset); // Dispatch server. We expect to see some data. - expectServerStream(); + EXPECT_CALL(response_decoder_, decodeHeaders_(_, _)).Times(0); EXPECT_CALL(request_decoder_, decodeHeaders_(_, false)) .WillOnce(InvokeWithoutArgs([&]() -> void { // Start a response inside the headers callback. This should not result in the client - // seeing any headers as the stream should already be reset on the other side. + // seeing any headers as the stream should already be reset on the other side, even though + // we don't know about it yet. TestHeaderMapImpl response_headers{{":status", "200"}}; response_encoder_->encodeHeaders(response_headers, false); }));