Skip to content

Commit

Permalink
quiche: handle stream blockage during decodeHeaders and decodeTrailers (
Browse files Browse the repository at this point in the history
envoyproxy#16128)

Commit Message: use posted callback to block/unblock quic stream in EnvoyQuicStream::readDisable() with weak_ptr to stream to handle stream life time issue.
Additional Description: currently if readDisabled() is called in decodeHeaders|Trailers(), the stream will be blocked right away which breaks the assumption QUICHE has that a stream shouldn't be blocked during OnInitialHeadersComplete() and OnTrailingHeadersComplete().
This change makes the stream state change completely outside of QUICHE call stack. (The unblocking is already outside of QUICHE call stack in existing implementation.) Also simplify the blockage state change logic.

Risk Level: low
Testing: added more unit tests and enabled Http2UpstreamIntegrationTest::ManyLargeSimultaneousRequestWithBufferLimits which was flaky.

Part of envoyproxy#2557 envoyproxy#14829

Signed-off-by: Dan Zhang <danzh@google.com>
Co-authored-by: Dan Zhang <danzh@google.com>
Signed-off-by: Gokul Nair <gnair@twitter.com>
  • Loading branch information
2 people authored and Gokul Nair committed May 6, 2021
1 parent f68f942 commit cda66d3
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 11 deletions.
10 changes: 0 additions & 10 deletions test/common/quic/envoy_quic_client_stream_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -577,16 +577,6 @@ TEST_P(EnvoyQuicClientStreamTest, CloseConnectionDuringDecodingTrailer) {
}
}

TEST_P(EnvoyQuicClientStreamTest, MetadataNotSupported) {
Http::MetadataMap metadata_map = {{"key", "value"}};
Http::MetadataMapPtr metadata_map_ptr = std::make_unique<Http::MetadataMap>(metadata_map);
Http::MetadataMapVector metadata_map_vector;
metadata_map_vector.push_back(std::move(metadata_map_ptr));
quic_stream_->encodeMetadata(metadata_map_vector);
EXPECT_EQ(1, TestUtility::findCounter(scope_, "http3.metadata_not_supported_error")->value());
EXPECT_CALL(stream_callbacks_, onResetStream(_, _));
}

// Tests that posted stream block callback won't cause use-after-free crash.
TEST_P(EnvoyQuicClientStreamTest, ReadDisabledBeforeClose) {
const auto result = quic_stream_->encodeHeaders(request_headers_, /*end_stream=*/true);
Expand Down
2 changes: 1 addition & 1 deletion test/integration/quic_http_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ TEST_P(QuicHttpIntegrationTest, UpstreamReadDisabledOnGiantResponseBody) {
}

TEST_P(QuicHttpIntegrationTest, DownstreamReadDisabledOnGiantPost) {
config_helper_.addConfigModifier(ConfigHelper::adjustUpstreamTimeoutForTsan);
config_helper_.addConfigModifier(setUpstreamTimeout);
config_helper_.setBufferLimits(/*upstream_buffer_limit=*/1024, /*downstream_buffer_limit=*/1024);
testRouterRequestAndResponseWithBody(/*request_size=*/10 * 1024 * 1024, /*response_size=*/1024,
false);
Expand Down

0 comments on commit cda66d3

Please sign in to comment.