Skip to content
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

test: Remove exceptions from the H/2 codec test #18720

Merged
merged 2 commits into from
Oct 27, 2021
Merged

test: Remove exceptions from the H/2 codec test #18720

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b105db8
Remove exceptions from the H/2 codec test
yanavlasov Oct 22, 2021
3ad4f2b
Address comments
yanavlasov Oct 26, 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
102 changes: 59 additions & 43 deletions test/common/http/http2/codec_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,6 @@ class Http2CodecImplTestFixture {
static bool slowContainsStreamId(int id, ConnectionImpl& connection) {
return connection.slowContainsStreamId(id);
}
// The Http::Connection::dispatch method does not throw (any more). However unit tests in this
// file use codecs for sending test data through mock network connections to the codec under test.
// It is infeasible to plumb error codes returned by the dispatch() method of the codecs under
// test, through mock connections and sending codec. As a result error returned by the dispatch
// method of the codec under test invoked by the ConnectionWrapper is thrown as an exception. Note
// that exception goes only through the mock network connection and sending codec, i.e. it is
// thrown only through the test harness code. Specific exception types are to distinguish error
// codes returned when processing requests or responses.
// TODO(yanavlasov): modify the code to verify test expectations at the point of calling codec
// under test through the ON_CALL expectations in the
// setupDefaultConnectionMocks() method. This will make the exceptions below
// unnecessary.
struct ClientCodecError : public std::runtime_error {
ClientCodecError(Http::Status&& status)
: std::runtime_error(std::string(status.message())), status_(std::move(status)) {}
Expand All @@ -84,19 +72,19 @@ class Http2CodecImplTestFixture {
struct ConnectionWrapper {
Http::Status dispatch(const Buffer::Instance& data, ConnectionImpl& connection) {
connection_ = &connection;
Http::Status status = Http::okStatus();
buffer_.add(data);
return dispatchBufferedData();
}

Http::Status dispatchBufferedData() {
Http::Status status = Http::okStatus();
if (!dispatching_) {
if (!dispatching_ && status_.ok()) {
while (buffer_.length() > 0) {
dispatching_ = true;
status = connection_->dispatch(buffer_);
if (!status.ok()) {
// Exit early if we hit an error status.
// Exit early if we hit an error status and record it for verification in the test.
status_.Update(status);
return status;
}
dispatching_ = false;
Expand All @@ -108,6 +96,7 @@ class Http2CodecImplTestFixture {
bool dispatching_{};
Buffer::OwnedImpl buffer_;
ConnectionImpl* connection_{};
Http::Status status_;
};

enum SettingsTupleIndex {
Expand Down Expand Up @@ -168,17 +157,11 @@ class Http2CodecImplTestFixture {
if (corrupt_metadata_frame_) {
corruptMetadataFramePayload(data);
}
auto status = server_wrapper_.dispatch(data, *server_);
if (!status.ok()) {
throw ServerCodecError(std::move(status));
}
server_wrapper_.dispatch(data, *server_).IgnoreError();
}));
ON_CALL(server_connection_, write(_, _))
.WillByDefault(Invoke([&](Buffer::Instance& data, bool) -> void {
auto status = client_wrapper_.dispatch(data, *client_);
if (!status.ok()) {
throw ClientCodecError(std::move(status));
}
client_wrapper_.dispatch(data, *client_).IgnoreError();
}));
}

Expand Down Expand Up @@ -421,7 +404,9 @@ TEST_P(Http2CodecImplTest, TrailerStatus) {
response_encoder_->encodeHeaders(response_headers, false);

// nghttp2 doesn't allow :status in trailers
EXPECT_THROW(response_encoder_->encode100ContinueHeaders(continue_headers), ClientCodecError);
response_encoder_->encode100ContinueHeaders(continue_headers);
EXPECT_FALSE(client_wrapper_.status_.ok());
EXPECT_TRUE(isCodecProtocolError(client_wrapper_.status_));
EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value());
};

Expand Down Expand Up @@ -471,7 +456,9 @@ TEST_P(Http2CodecImplTest, Invalid101SwitchingProtocols) {

TestResponseHeaderMapImpl upgrade_headers{{":status", "101"}};
EXPECT_CALL(response_decoder_, decodeHeaders_(_, _)).Times(0);
EXPECT_THROW(response_encoder_->encodeHeaders(upgrade_headers, false), ClientCodecError);
response_encoder_->encodeHeaders(upgrade_headers, false);
EXPECT_FALSE(client_wrapper_.status_.ok());
EXPECT_TRUE(isCodecProtocolError(client_wrapper_.status_));
EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value());
}

Expand All @@ -484,7 +471,9 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFin) {
EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok());

TestResponseHeaderMapImpl continue_headers{{":status", "100"}};
EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), ClientCodecError);
response_encoder_->encodeHeaders(continue_headers, true);
EXPECT_FALSE(client_wrapper_.status_.ok());
EXPECT_TRUE(isCodecProtocolError(client_wrapper_.status_));
EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value());
}

Expand Down Expand Up @@ -553,7 +542,9 @@ TEST_P(Http2CodecImplTest, InvalidRepeatContinue) {
EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_));
response_encoder_->encode100ContinueHeaders(continue_headers);

EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), ClientCodecError);
response_encoder_->encodeHeaders(continue_headers, true);
EXPECT_FALSE(client_wrapper_.status_.ok());
EXPECT_TRUE(isCodecProtocolError(client_wrapper_.status_));
EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value());
};

Expand Down Expand Up @@ -611,7 +602,9 @@ TEST_P(Http2CodecImplTest, Invalid204WithContentLength) {
"debug",
"Invalid HTTP header field was received: frame type: 1, stream: 1, name: [content-length], "
"value: [3]",
EXPECT_THROW(response_encoder_->encodeHeaders(response_headers, false), ClientCodecError));
response_encoder_->encodeHeaders(response_headers, false));
EXPECT_FALSE(client_wrapper_.status_.ok());
EXPECT_TRUE(isCodecProtocolError(client_wrapper_.status_));
EXPECT_EQ(1, client_stats_store_.counter("http2.rx_messaging_error").value());
};

Expand Down Expand Up @@ -841,8 +834,11 @@ TEST_P(Http2CodecImplTest, BadMetadataVecReceivedTest) {
metadata_map_vector.push_back(std::move(metadata_map_ptr));

corrupt_metadata_frame_ = true;
EXPECT_THROW_WITH_MESSAGE(request_encoder_->encodeMetadata(metadata_map_vector), ServerCodecError,
"The user callback function failed");
request_encoder_->encodeMetadata(metadata_map_vector);
// The error is detected by the server codec.
EXPECT_FALSE(server_wrapper_.status_.ok());
EXPECT_TRUE(isCodecProtocolError(server_wrapper_.status_));
EXPECT_EQ(server_wrapper_.status_.message(), "The user callback function failed");
}

// Encode response metadata while dispatching request data from the client, so
Expand Down Expand Up @@ -1899,6 +1895,8 @@ TEST_P(Http2CodecImplStreamLimitTest, LazyDecreaseMaxConcurrentStreamsConsumeErr
EXPECT_EQ(1, server_stats_store_.counter("http2.tx_reset").value());
EXPECT_EQ(1, TestUtility::findGauge(client_stats_store_, "http2.streams_active")->value());
EXPECT_EQ(1, TestUtility::findGauge(server_stats_store_, "http2.streams_active")->value());
// The server codec should not fail since the error is "consumed".
EXPECT_TRUE(server_wrapper_.status_.ok());
}

TEST_P(Http2CodecImplStreamLimitTest, LazyDecreaseMaxConcurrentStreamsIgnoreError) {
Expand Down Expand Up @@ -1937,14 +1935,17 @@ TEST_P(Http2CodecImplStreamLimitTest, LazyDecreaseMaxConcurrentStreamsIgnoreErro

request_encoder_ = &client_->newStream(response_decoder_);
setupDefaultConnectionMocks();
EXPECT_THROW_WITH_MESSAGE(request_encoder_->encodeHeaders(request_headers, true).IgnoreError(),
ServerCodecError, "The user callback function failed");
EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok());
// The server codec should fail since there are no available streams.
EXPECT_FALSE(server_wrapper_.status_.ok());
EXPECT_TRUE(isCodecProtocolError(server_wrapper_.status_));
EXPECT_EQ(server_wrapper_.status_.message(), "The user callback function failed");

EXPECT_EQ(0, server_stats_store_.counter("http2.stream_refused_errors").value());
EXPECT_EQ(0, server_stats_store_.counter("http2.tx_reset").value());
asraa marked this conversation as resolved.
Show resolved Hide resolved

// Not verifying the http2.streams_active server/client gauges here as the
// EXPECT_THROW_WITH_MESSAGE above doesn't let us fully capture the behavior of the real system.
// test dispatch function doesn't let us fully capture the behavior of the real system.
// In the real world, the status returned from dispatch would trigger a connection close which
// would result in the active stream gauges to go down to 0.
}
Expand Down Expand Up @@ -2437,8 +2438,11 @@ TEST_P(Http2CodecImplTest, PingFlood) {
buffer.move(frame);
}));

EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError,
"Too many control frames in the outbound queue.");
client_->sendPendingFrames().IgnoreError();
// The PING flood is detected by the server codec.
EXPECT_FALSE(server_wrapper_.status_.ok());
EXPECT_TRUE(isBufferFloodError(server_wrapper_.status_));
EXPECT_EQ(server_wrapper_.status_.message(), "Too many control frames in the outbound queue.");
EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_control_flood").value());
}

Expand Down Expand Up @@ -2507,8 +2511,11 @@ TEST_P(Http2CodecImplTest, PingFloodCounterReset) {

// 1 more ping frame should overflow the outbound frame limit.
EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr));
EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError,
"Too many control frames in the outbound queue.");
client_->sendPendingFrames().IgnoreError();
// The server codec should fail when it gets 1 PING too many.
EXPECT_FALSE(server_wrapper_.status_.ok());
EXPECT_TRUE(isBufferFloodError(server_wrapper_.status_));
EXPECT_EQ(server_wrapper_.status_.message(), "Too many control frames in the outbound queue.");
}

// Verify that codec detects flood of outbound HEADER frames
Expand Down Expand Up @@ -2678,8 +2685,11 @@ TEST_P(Http2CodecImplTest, PingStacksWithDataFlood) {
}
// Send one PING frame above the outbound queue size limit
EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr));
EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError,
"Too many frames in the outbound queue.");
client_->sendPendingFrames().IgnoreError();
// The server codec should fail when it gets 1 frame too many.
EXPECT_FALSE(server_wrapper_.status_.ok());
EXPECT_TRUE(isBufferFloodError(server_wrapper_.status_));
EXPECT_EQ(server_wrapper_.status_.message(), "Too many frames in the outbound queue.");

EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_flood").value());
}
Expand Down Expand Up @@ -2774,8 +2784,11 @@ TEST_P(Http2CodecImplTest, MetadataFlood) {

TEST_P(Http2CodecImplTest, PriorityFlood) {
priorityFlood();
EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError,
"Too many PRIORITY frames");
client_->sendPendingFrames().IgnoreError();
// The PRIORITY flood is detected by the server codec.
EXPECT_FALSE(server_wrapper_.status_.ok());
EXPECT_TRUE(isBufferFloodError(server_wrapper_.status_));
EXPECT_EQ(server_wrapper_.status_.message(), "Too many PRIORITY frames");
}

TEST_P(Http2CodecImplTest, PriorityFloodOverride) {
Expand All @@ -2787,8 +2800,11 @@ TEST_P(Http2CodecImplTest, PriorityFloodOverride) {

TEST_P(Http2CodecImplTest, WindowUpdateFlood) {
windowUpdateFlood();
EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError,
"Too many WINDOW_UPDATE frames");
client_->sendPendingFrames().IgnoreError();
// The server codec should fail when it gets 1 WINDOW_UPDATE frame too many.
EXPECT_FALSE(server_wrapper_.status_.ok());
EXPECT_TRUE(isBufferFloodError(server_wrapper_.status_));
EXPECT_EQ(server_wrapper_.status_.message(), "Too many WINDOW_UPDATE frames");
}

TEST_P(Http2CodecImplTest, WindowUpdateFloodOverride) {
Expand Down