Skip to content

test: more fast-timeouts. #15959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alyssawilk merged 2 commits into from
Apr 14, 2021
Merged

test: more fast-timeouts. #15959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9d8f48
test: more fast-timeouts.
alyssawilk Apr 1, 2021
38f4da1
comments
alyssawilk Apr 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,7 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyOnResponseBody) {

// The stream should have been reset here before the complete
// response was received.
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
}

// Send a request, but wait longer than the "message timeout" before sending a response
Expand Down
6 changes: 3 additions & 3 deletions test/integration/http2_flood_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1351,7 +1351,7 @@ TEST_P(Http2FloodMitigationTest, UpstreamEmptyData) {
}

ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
EXPECT_EQ("200", response->headers().getStatusValue());
EXPECT_EQ(1,
test_server_->counter("cluster.cluster_0.http2.inbound_empty_frames_flood")->value());
Expand Down Expand Up @@ -1417,7 +1417,7 @@ TEST_P(Http2FloodMitigationTest, UpstreamPriorityOneOpenStream) {
ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
// Downstream client should get stream reset since upstream sent headers but did not complete the
// stream
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
EXPECT_EQ(
1, test_server_->counter("cluster.cluster_0.http2.inbound_priority_frames_flood")->value());
}
Expand Down Expand Up @@ -1475,7 +1475,7 @@ TEST_P(Http2FloodMitigationTest, UpstreamRstStreamOnStreamIdleTimeout) {
// Verify that when RST_STREAM overflows upstream queue it is handled correctly
// by causing upstream connection to be disconnected.
ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
// EXPECT_EQ("408", response->headers().getStatusValue());
EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.http2.outbound_control_flood")->value());
EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_rq_idle_timeout")->value());
Expand Down
12 changes: 6 additions & 6 deletions test/integration/http_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ void HttpIntegrationTest::testRouterUpstreamDisconnectBeforeResponseComplete(
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
}

Expand Down Expand Up @@ -1144,7 +1144,7 @@ void HttpIntegrationTest::testLargeRequestUrl(uint32_t url_size, uint32_t max_he
EXPECT_TRUE(response->complete());
EXPECT_EQ("431", response->headers().Status()->value().getStringView());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
}
} else {
Expand Down Expand Up @@ -1192,7 +1192,7 @@ void HttpIntegrationTest::testLargeRequestHeaders(uint32_t size, uint32_t count,
EXPECT_TRUE(response->complete());
EXPECT_EQ("431", response->headers().getStatusValue());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
}
} else {
Expand Down Expand Up @@ -1235,7 +1235,7 @@ void HttpIntegrationTest::testLargeRequestTrailers(uint32_t size, uint32_t max_s
} else {
// Expect a stream reset when the size of the trailers is larger than the maximum
// limit.
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
EXPECT_FALSE(response->complete());
}
Expand Down Expand Up @@ -1432,7 +1432,7 @@ void HttpIntegrationTest::testMaxStreamDuration() {
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForEndStream());
codec_client_->close();
}
}
Expand Down Expand Up @@ -1477,7 +1477,7 @@ void HttpIntegrationTest::testMaxStreamDurationWithRetry(bool invoke_retry_upstr
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForEndStream());
codec_client_->close();
}

Expand Down
2 changes: 1 addition & 1 deletion test/integration/http_timeout_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutAfterHeadersBeforeBodyResetsUpst

ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15)));

response->waitForReset();
ASSERT_TRUE(response->waitForReset());

codec_client_->close();

Expand Down
2 changes: 1 addition & 1 deletion test/integration/idle_timeout_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class IdleTimeoutIntegrationTest : public HttpProtocolIntegrationTest {
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response.waitForReset();
ASSERT_TRUE(response.waitForReset());
codec_client_->close();
}
if (!stat_name.empty()) {
Expand Down
10 changes: 9 additions & 1 deletion test/integration/integration_stream_decoder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,19 @@ AssertionResult IntegrationStreamDecoder::waitForEndStream(std::chrono::millisec
return AssertionSuccess();
}

void IntegrationStreamDecoder::waitForReset() {
AssertionResult IntegrationStreamDecoder::waitForReset(std::chrono::milliseconds timeout) {
Copy link
Copy Markdown
Contributor

@snowp snowp Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must use result?

Copy link
Copy Markdown
Contributor

@yanavlasov yanavlasov Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean ABSL_MUST_USE_RESULT?

Copy link
Copy Markdown
Contributor

@snowp snowp Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, seems like we want callers to check this?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, maybe I can on this one - I couldn't on the other one I ported without changing signature, updating envoy filter examples, and then adding MUST_USE but I think Enovy filter examples doens't use this function

if (!saw_reset_) {
// Set a timer to stop the dispatcher if the timeout has been exceeded.
Event::TimerPtr timer(dispatcher_.createTimer([this]() -> void { dispatcher_.exit(); }));
Copy link
Copy Markdown
Contributor

@snowp snowp Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment on how this works (or this is a common pattern?), it took me a minute. If it is a common pattern, can we extract this to some helper?

Also might want to note somewhere that this leaves the dispatcher in a bad state so you really want ASSERT vs EXPECT

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it leaves the dispatcher in a bad state, what do you think the problem is?

Copy link
Copy Markdown
Contributor

@snowp snowp Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably misunderstanding how this works, but seems to me like this stops the dispatcher after the timer expires? It would then mean that it's no longer running after this? But on the other hand we don't disable this so this will fire on its own at some point anyways, so I guess it can't be messing up the dispatcher. So what exactly does calling dispatcher_.exit() do?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the dispatcher below is run in blocking mode, so it will loop for ever unless something tells it to exit.
Previously, it would exit if reset was received (or disconnect), now it also gives up after 5s.

timer->enableTimer(timeout);
waiting_for_reset_ = true;
dispatcher_.run(Event::Dispatcher::RunType::Block);
// If the timer has fired, this timed out before a reset was received.
if (!timer->enabled()) {
return AssertionFailure() << "Timed out waiting for reset.";
}
}
return AssertionSuccess();
}

void IntegrationStreamDecoder::decode100ContinueHeaders(Http::ResponseHeaderMapPtr&& headers) {
Expand Down
3 changes: 2 additions & 1 deletion test/integration/integration_stream_decoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::Stre
void waitForBodyData(uint64_t size);
testing::AssertionResult
waitForEndStream(std::chrono::milliseconds timeout = TestUtility::DefaultTimeout);
void waitForReset();
ABSL_MUST_USE_RESULT testing::AssertionResult
waitForReset(std::chrono::milliseconds timeout = TestUtility::DefaultTimeout);
void clearBody() { body_.clear(); }

// Http::StreamDecoder
Expand Down
14 changes: 7 additions & 7 deletions test/integration/multiplexed_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ TEST_P(Http2IntegrationTest, CodecStreamIdleTimeout) {
upstream_request_->encodeHeaders(default_response_headers_, false);
upstream_request_->encodeData(70000, true);
test_server_->waitForCounterEq("http2.tx_flush_timeout", 1);
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
}

TEST_P(Http2IntegrationTest, Http2DownstreamKeepalive) {
Expand Down Expand Up @@ -171,7 +171,7 @@ TEST_P(Http2IntegrationTest, Http2DownstreamKeepalive) {
test_server_->waitForCounterEq("http2.keepalive_timeout", 1,
std::chrono::milliseconds(timeout_ms * 2));

response->waitForReset();
ASSERT_TRUE(response->waitForReset());
}

static std::string response_metadata_filter = R"EOF(
Expand Down Expand Up @@ -299,7 +299,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) {
upstream_request_->encodeResetStream();

// Verifies stream is reset.
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
ASSERT_FALSE(response->complete());
}

Expand Down Expand Up @@ -502,7 +502,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMultipleMetadataReachSizeLimit) {
upstream_request_->encodeData(12, true);

// Verifies reset is received.
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
ASSERT_FALSE(response->complete());
}

Expand Down Expand Up @@ -882,8 +882,8 @@ TEST_P(Http2IntegrationTest, CodecErrorAfterStreamStart) {
Buffer::OwnedImpl bogus_data("some really bogus data");
codec_client_->rawConnection().write(bogus_data, false);

// Verifies reset is received.
response->waitForReset();
// Verifies error is received.
ASSERT_TRUE(response->waitForEndStream());
}

TEST_P(Http2IntegrationTest, Http2BadMagic) {
Expand Down Expand Up @@ -1690,7 +1690,7 @@ TEST_P(Http2IntegrationTest, OnLocalReply) {
{
default_request_headers_.addCopy("reset", "yes");
auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
ASSERT_FALSE(response->complete());
}
}
Expand Down
4 changes: 2 additions & 2 deletions test/integration/multiplexed_upstream_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ TEST_P(Http2UpstreamIntegrationTest, BidirectionalStreamingReset) {

// Reset the stream.
upstream_request_->encodeResetStream();
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
EXPECT_FALSE(response->complete());
}

Expand Down Expand Up @@ -386,7 +386,7 @@ TEST_P(Http2UpstreamIntegrationTest, UpstreamConnectionCloseWithManyStreams) {
ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
// Ensure the streams are all reset successfully.
for (uint32_t i = 1; i < num_requests; ++i) {
responses[i]->waitForReset();
ASSERT_TRUE(responses[i]->waitForReset());
}
}

Expand Down
23 changes: 11 additions & 12 deletions test/integration/protocol_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,7 @@ TEST_P(DownstreamProtocolIntegrationTest, HeadersWithUnderscoresCauseRequestReje
ASSERT_TRUE(response->complete());
EXPECT_EQ("400", response->headers().getStatusValue());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
ASSERT_TRUE(response->reset());
EXPECT_EQ(Http::StreamResetReason::RemoteReset, response->resetReason());
Expand Down Expand Up @@ -1257,7 +1257,7 @@ TEST_P(ProtocolIntegrationTest, 304WithBody) {
// 304-with-body, is there a protocol error on the active stream.
if (downstream_protocol_ >= Http::CodecClient::Type::HTTP2 &&
upstreamProtocol() >= FakeHttpConnection::Type::HTTP2) {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
}
}

Expand Down Expand Up @@ -1438,7 +1438,7 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLengthAllowed) {
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
}

Expand Down Expand Up @@ -1497,7 +1497,7 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) {
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
}

Expand Down Expand Up @@ -1553,7 +1553,7 @@ name: local-reply-during-encode-data

// Response was aborted after headers were sent to the client.
// The stream was reset. Client does not receive body or trailers.
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
EXPECT_FALSE(response->complete());
EXPECT_EQ("200", response->headers().getStatusValue());
EXPECT_EQ(0, response->body().length());
Expand Down Expand Up @@ -1622,7 +1622,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) {
EXPECT_TRUE(response->complete());
EXPECT_EQ("431", response->headers().getStatusValue());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
}
}
Expand Down Expand Up @@ -2193,8 +2193,7 @@ TEST_P(DownstreamProtocolIntegrationTest, BasicMaxStreamTimeout) {
ASSERT_TRUE(upstream_request_->waitForHeadersComplete());

test_server_->waitForCounterGe("http.config_test.downstream_rq_max_duration_reached", 1);
response->waitForReset();
EXPECT_TRUE(response->complete());
ASSERT_TRUE(response->waitForEndStream());
}

TEST_P(DownstreamProtocolIntegrationTest, BasicMaxStreamTimeoutLegacy) {
Expand All @@ -2214,7 +2213,7 @@ TEST_P(DownstreamProtocolIntegrationTest, BasicMaxStreamTimeoutLegacy) {
ASSERT_TRUE(upstream_request_->waitForHeadersComplete());

test_server_->waitForCounterGe("http.config_test.downstream_rq_max_duration_reached", 1);
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
EXPECT_FALSE(response->complete());
EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("max_duration_timeout"));
}
Expand Down Expand Up @@ -2259,7 +2258,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) {
EXPECT_EQ("404", response->headers().getStatusValue());
EXPECT_TRUE(response->complete());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
ASSERT_TRUE(codec_client_->waitForDisconnect());
}
}
Expand Down Expand Up @@ -2288,7 +2287,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) {
auto response = codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{
{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}});

response->waitForReset();
ASSERT_TRUE(response->waitForReset());
EXPECT_FALSE(codec_client_->disconnected());
}

Expand Down Expand Up @@ -2321,7 +2320,7 @@ TEST_P(DownstreamProtocolIntegrationTest, Test100AndDisconnectLegacy) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
EXPECT_FALSE(response->complete());
} else {
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
EXPECT_FALSE(response->complete());
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/quic_http_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ TEST_P(QuicHttpIntegrationTest, Reset101SwitchProtocolResponse) {
ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));

upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "101"}}, false);
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
codec_client_->close();
EXPECT_FALSE(response->complete());
}
Expand Down
6 changes: 3 additions & 3 deletions test/integration/tcp_tunneling_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ TEST_P(ConnectTerminationIntegrationTest, UpstreamClose) {

// Tear down by closing the upstream connection.
ASSERT_TRUE(fake_raw_upstream_connection_->close());
response_->waitForReset();
ASSERT_TRUE(response_->waitForReset());
}

TEST_P(ConnectTerminationIntegrationTest, TestTimeout) {
Expand All @@ -179,7 +179,7 @@ TEST_P(ConnectTerminationIntegrationTest, TestTimeout) {
setUpConnection();

// Wait for the timeout to close the connection.
response_->waitForReset();
ASSERT_TRUE(response_->waitForReset());
ASSERT_TRUE(fake_raw_upstream_connection_->waitForHalfClose());
}

Expand Down Expand Up @@ -229,7 +229,7 @@ TEST_P(ConnectTerminationIntegrationTest, BasicMaxStreamDuration) {
if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
} else {
response_->waitForReset();
ASSERT_TRUE(response_->waitForReset());
codec_client_->close();
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/vhds_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ TEST_P(VhdsIntegrationTest, VhdsOnDemandUpdateHttpConnectionCloses) {
vhds_stream_->sendGrpcMessage(vhds_update);

codec_client_->sendReset(encoder);
response->waitForReset();
ASSERT_TRUE(response->waitForReset());
EXPECT_TRUE(codec_client_->connected());

cleanupUpstreamAndDownstream();
Expand Down
2 changes: 1 addition & 1 deletion test/integration/websocket_integration_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class WebsocketIntegrationTest : public HttpProtocolIntegrationTest {
void waitForClientDisconnectOrReset(
Http::StreamResetReason reason = Http::StreamResetReason::RemoteReset) {
if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) {
response_->waitForReset();
ASSERT_TRUE(response_->waitForReset());
ASSERT_EQ(reason, response_->resetReason());
} else {
ASSERT_TRUE(codec_client_->waitForDisconnect());
Expand Down