Allow visitor_.OnFrameSent() to propagate a fatal send error, and do not retry sending on fatal send errors. This CL makes OgHttp2Session::AfterFrameSent() return a bool to reflect an error value from visitor_.OnFrameSent(), which is treated as a fatal connection error and propagated to OgHttp2Session::Send(). This propagation is helpful for compatibility with nghttp2 and Envoy. For reference, the only (practically considered) error code nghttp2 returns for failed sends is a fatal error: https://nghttp2.org/documentation/nghttp2_session_send.html This CL also makes a change to not let OgHttp2Session::Send() send data when it has previously returned an error value. This is not consistent with nghttp2, but it feels right and does not negatively affect current codec_impl_tests. With this CL, codec_impl_tests Sponge (ProtocolErrorForTest now passes): http://sponge2/279619fd-ba65-4ba9-ad96-367b4606cfc8 PiperOrigin-RevId: 416424214
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index 6410b97..fc5dbd2 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -1992,6 +1992,118 @@ EXPECT_EQ(result, NGHTTP2_ERR_CALLBACK_FAILURE); } +TEST(NgHttp2AdapterTest, ConnectionErrorOnControlFrameSent) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + + const std::string frames = + TestFrameSequence().ClientPreface().Ping(42).Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // PING + EXPECT_CALL(visitor, OnFrameHeader(0, _, PING, 0)); + EXPECT_CALL(visitor, OnPing(42, false)); + + const int64_t read_result = adapter->ProcessBytes(frames); + EXPECT_EQ(static_cast<size_t>(read_result), frames.size()); + + EXPECT_TRUE(adapter->want_write()); + + // SETTINGS ack + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)) + .WillOnce(testing::Return(-902)); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kSendError)); + + int send_result = adapter->Send(); + EXPECT_LT(send_result, 0); + + // Apparently nghttp2 retries sending the frames that had failed before. + EXPECT_TRUE(adapter->want_write()); + + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(PING, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(PING, 0, _, 0x1, 0)); + send_result = adapter->Send(); + EXPECT_EQ(send_result, 0); + + EXPECT_FALSE(adapter->want_write()); +} + +TEST(NgHttp2AdapterTest, ConnectionErrorOnDataFrameSent) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + + const std::string frames = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}, + /*fin=*/true) + .Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); + + const int64_t read_result = adapter->ProcessBytes(frames); + EXPECT_EQ(static_cast<size_t>(read_result), frames.size()); + + auto body = absl::make_unique<TestDataFrameSource>(visitor, true); + body->AppendPayload("Here is some data, which will lead to a fatal error"); + TestDataFrameSource* body_ptr = body.get(); + int submit_result = adapter->SubmitResponse( + 1, ToHeaders({{":status", "200"}}), std::move(body)); + ASSERT_EQ(0, submit_result); + + EXPECT_TRUE(adapter->want_write()); + + // SETTINGS ack + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + // Stream 1, with doomed DATA + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0)); + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0)) + .WillOnce(testing::Return(-902)); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kSendError)); + + int send_result = adapter->Send(); + EXPECT_LT(send_result, 0); + + // The test data source got a signal that the first chunk of data was sent + // successfully, so discarded that data internally. However, due to the send + // error, the next Send() from nghttp2 will try to send that exact same data + // again. Without this line appending the exact same data back to the data + // source, the test crashes. It is not clear how the data source would know to + // not discard the data, unless told by the session? This is not intuitive. + body_ptr->AppendPayload( + "Here is some data, which will lead to a fatal error"); + + // Apparently nghttp2 retries sending the frames that had failed before. + EXPECT_TRUE(adapter->want_write()); + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0)); + + send_result = adapter->Send(); + EXPECT_EQ(send_result, 0); + + EXPECT_FALSE(adapter->want_write()); +} + TEST(NgHttp2AdapterTest, ServerConstruction) { testing::StrictMock<MockHttp2Visitor> visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 50dce2c..8d62da9 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -2164,6 +2164,101 @@ SpdyFrameType::PING})); } +TEST_F(OgHttp2AdapterTest, ConnectionErrorOnControlFrameSent) { + const std::string frames = + TestFrameSequence().ClientPreface().Ping(42).Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(http2_visitor_, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(http2_visitor_, OnSettingsStart()); + EXPECT_CALL(http2_visitor_, OnSettingsEnd()); + // PING + EXPECT_CALL(http2_visitor_, OnFrameHeader(0, _, PING, 0)); + EXPECT_CALL(http2_visitor_, OnPing(42, false)); + + const int64_t read_result = adapter_->ProcessBytes(frames); + EXPECT_EQ(static_cast<size_t>(read_result), frames.size()); + + EXPECT_TRUE(adapter_->want_write()); + + // Server preface (SETTINGS) + EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(http2_visitor_, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + // SETTINGS ack + EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(http2_visitor_, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)) + .WillOnce(testing::Return(-902)); + EXPECT_CALL(http2_visitor_, OnConnectionError(ConnectionError::kSendError)); + + int send_result = adapter_->Send(); + EXPECT_LT(send_result, 0); + + EXPECT_FALSE(adapter_->want_write()); + + send_result = adapter_->Send(); + EXPECT_LT(send_result, 0); +} + +TEST_F(OgHttp2AdapterTest, ConnectionErrorOnDataFrameSent) { + const std::string frames = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}, + /*fin=*/true) + .Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(http2_visitor_, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(http2_visitor_, OnSettingsStart()); + EXPECT_CALL(http2_visitor_, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(http2_visitor_, OnFrameHeader(1, _, HEADERS, 0x5)); + EXPECT_CALL(http2_visitor_, OnBeginHeadersForStream(1)); + EXPECT_CALL(http2_visitor_, OnHeaderForStream(1, _, _)).Times(4); + EXPECT_CALL(http2_visitor_, OnEndHeadersForStream(1)); + EXPECT_CALL(http2_visitor_, OnEndStream(1)); + + const int64_t read_result = adapter_->ProcessBytes(frames); + EXPECT_EQ(static_cast<size_t>(read_result), frames.size()); + + auto body = absl::make_unique<TestDataFrameSource>(http2_visitor_, true); + body->AppendPayload("Here is some data, which will lead to a fatal error"); + TestDataFrameSource* body_ptr = body.get(); + int submit_result = adapter_->SubmitResponse( + 1, ToHeaders({{":status", "200"}}), std::move(body)); + ASSERT_EQ(0, submit_result); + + EXPECT_TRUE(adapter_->want_write()); + + // Server preface (SETTINGS) + EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(http2_visitor_, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + // SETTINGS ack + EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(http2_visitor_, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + // Stream 1, with doomed DATA + EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(HEADERS, 1, _, 0x4)); + EXPECT_CALL(http2_visitor_, OnFrameSent(HEADERS, 1, _, 0x4, 0)); + EXPECT_CALL(http2_visitor_, OnFrameSent(DATA, 1, _, 0x0, 0)) + .WillOnce(testing::Return(-902)); + EXPECT_CALL(http2_visitor_, OnConnectionError(ConnectionError::kSendError)); + + int send_result = adapter_->Send(); + EXPECT_LT(send_result, 0); + + body_ptr->AppendPayload("After the fatal error, data will be sent no more"); + + EXPECT_FALSE(adapter_->want_write()); + + send_result = adapter_->Send(); + EXPECT_LT(send_result, 0); +} + TEST(OgHttp2AdapterServerTest, ClientSendsContinuation) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 346035a..0519cee 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -477,6 +477,10 @@ sending_ = true; RunOnExit r{[this]() { sending_ = false; }}; + if (fatal_send_error_) { + return kSendError; + } + MaybeSetupPreface(); SendResult continue_writing = SendQueuedFrames(); @@ -495,7 +499,12 @@ if (continue_writing == SendResult::SEND_OK) { continue_writing = SendQueuedFrames(); } - return continue_writing == SendResult::SEND_ERROR ? kSendError : 0; + if (continue_writing == SendResult::SEND_ERROR) { + fatal_send_error_ = true; + return kSendError; + } else { + return 0; + } } bool OgHttp2Session::HasReadyStream() const { @@ -564,10 +573,16 @@ // Write blocked. return SendResult::SEND_BLOCKED; } else { - AfterFrameSent(c.frame_type(), c.stream_id(), frame_payload_length, - c.flags(), c.error_code()); - frames_.pop_front(); + + const bool ok = + AfterFrameSent(c.frame_type(), c.stream_id(), frame_payload_length, + c.flags(), c.error_code()); + if (!ok) { + LatchErrorAndNotify(Http2ErrorCode::INTERNAL_ERROR, + ConnectionError::kSendError); + return SendResult::SEND_ERROR; + } if (static_cast<size_t>(result) < frame.size()) { // The frame was partially written, so the rest must be buffered. buffered_data_.append(frame.data() + result, frame.size() - result); @@ -578,11 +593,14 @@ return SendResult::SEND_OK; } -void OgHttp2Session::AfterFrameSent(uint8_t frame_type, uint32_t stream_id, +bool OgHttp2Session::AfterFrameSent(uint8_t frame_type, uint32_t stream_id, size_t payload_length, uint8_t flags, uint32_t error_code) { - visitor_.OnFrameSent(frame_type, stream_id, payload_length, flags, - error_code); + int result = visitor_.OnFrameSent(frame_type, stream_id, payload_length, + flags, error_code); + if (result < 0) { + return false; + } if (stream_id == 0) { const bool is_settings_ack = static_cast<FrameType>(frame_type) == FrameType::SETTINGS && @@ -592,7 +610,7 @@ encoder_header_table_capacity_when_acking_.value()); encoder_header_table_capacity_when_acking_ = absl::nullopt; } - return; + return true; } auto iter = queued_frames_.find(stream_id); if (frame_type != 0) { @@ -602,6 +620,7 @@ // TODO(birenroy): Consider passing through `error_code` here. CloseStreamIfReady(frame_type, stream_id); } + return true; } OgHttp2Session::SendResult OgHttp2Session::WriteForStream( @@ -683,7 +702,13 @@ state.half_closed_local = true; MaybeFinWithRstStream(it); } - AfterFrameSent(/* DATA */ 0, stream_id, length, fin ? 0x1 : 0x0, 0); + const bool ok = + AfterFrameSent(/* DATA */ 0, stream_id, length, fin ? 0x1 : 0x0, 0); + if (!ok) { + LatchErrorAndNotify(Http2ErrorCode::INTERNAL_ERROR, + ConnectionError::kSendError); + return SendResult::SEND_ERROR; + } if (!stream_map_.contains(stream_id)) { // Note: the stream may have been closed if `fin` is true. break;
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h index 74dedac..aea6749 100644 --- a/http2/adapter/oghttp2_session.h +++ b/http2/adapter/oghttp2_session.h
@@ -124,8 +124,9 @@ return !received_goaway_ && !decoder_.HasError(); } bool want_write() const override { - return !frames_.empty() || !buffered_data_.empty() || - !connection_metadata_.empty() || HasReadyStream(); + return !fatal_send_error_ && + (!frames_.empty() || !buffered_data_.empty() || + !connection_metadata_.empty() || HasReadyStream()); } int GetRemoteWindowSize() const override { return connection_send_window_; } @@ -299,7 +300,8 @@ // Serializes and sends queued frames. SendResult SendQueuedFrames(); - void AfterFrameSent(uint8_t frame_type, uint32_t stream_id, + // Returns false if a fatal connection error occurred. + bool AfterFrameSent(uint8_t frame_type, uint32_t stream_id, size_t payload_length, uint8_t flags, uint32_t error_code); @@ -452,6 +454,9 @@ // Replace this with a stream ID, for multiple GOAWAY support. bool queued_goaway_ = false; bool latched_error_ = false; + + // True if a fatal sending error has occurred. + bool fatal_send_error_ = false; }; } // namespace adapter