From 422e0b2568a15f19a2ead649a1c775a4dc6b84b4 Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Wed, 27 Oct 2021 14:06:01 +0000 Subject: [PATCH] test: Remove exceptions from the H/2 codec test (#18720) Signed-off-by: Yan Avlasov --- test/common/http/http2/codec_impl_test.cc | 102 +++++++++++++--------- 1 file changed, 59 insertions(+), 43 deletions(-) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 50d9b50802b3..a23b056b9a77 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -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)) {} @@ -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; @@ -108,6 +96,7 @@ class Http2CodecImplTestFixture { bool dispatching_{}; Buffer::OwnedImpl buffer_; ConnectionImpl* connection_{}; + Http::Status status_; }; enum SettingsTupleIndex { @@ -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(); })); } @@ -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()); }; @@ -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()); } @@ -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()); } @@ -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()); }; @@ -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()); }; @@ -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 @@ -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) { @@ -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()); // 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. } @@ -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()); } @@ -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 @@ -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()); } @@ -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) { @@ -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) {