Convey more information in Http2VisitorInterface::OnConnectionError(). This CL introduces a ConnectionError enum class to Http2VisitorInterface and adds a ConnectionError argument to Http2VisitorInterface::OnConnectionError(). The visitor then has more detail about the scenario that triggers the connection error. For now, values include sending and parsing errors, an invalid connection preface, and a header error. PiperOrigin-RevId: 402636055
diff --git a/http2/adapter/callback_visitor.cc b/http2/adapter/callback_visitor.cc index becf5bf..99c1223 100644 --- a/http2/adapter/callback_visitor.cc +++ b/http2/adapter/callback_visitor.cc
@@ -68,7 +68,7 @@ } } -void CallbackVisitor::OnConnectionError() { +void CallbackVisitor::OnConnectionError(ConnectionError /*error*/) { QUICHE_LOG(ERROR) << "OnConnectionError not implemented"; }
diff --git a/http2/adapter/callback_visitor.h b/http2/adapter/callback_visitor.h index f252989..4abfe94 100644 --- a/http2/adapter/callback_visitor.h +++ b/http2/adapter/callback_visitor.h
@@ -23,7 +23,7 @@ void* user_data); int64_t OnReadyToSend(absl::string_view serialized) override; - void OnConnectionError() override; + void OnConnectionError(ConnectionError error) override; bool OnFrameHeader(Http2StreamId stream_id, size_t length, uint8_t type, uint8_t flags) override; void OnSettingsStart() override;
diff --git a/http2/adapter/http2_util.cc b/http2/adapter/http2_util.cc index e054a82..17c3ce8 100644 --- a/http2/adapter/http2_util.cc +++ b/http2/adapter/http2_util.cc
@@ -2,6 +2,11 @@ namespace http2 { namespace adapter { +namespace { + +using ConnectionError = Http2VisitorInterface::ConnectionError; + +} // anonymous namespace spdy::SpdyErrorCode TranslateErrorCode(Http2ErrorCode code) { switch (code) { @@ -69,5 +74,18 @@ } } +absl::string_view ConnectionErrorToString(ConnectionError error) { + switch (error) { + case ConnectionError::kInvalidConnectionPreface: + return "InvalidConnectionPreface"; + case ConnectionError::kSendError: + return "SendError"; + case ConnectionError::kParseError: + return "ParseError"; + case ConnectionError::kHeaderError: + return "HeaderError"; + } +} + } // namespace adapter } // namespace http2
diff --git a/http2/adapter/http2_util.h b/http2/adapter/http2_util.h index 88e9a49..be6b877 100644 --- a/http2/adapter/http2_util.h +++ b/http2/adapter/http2_util.h
@@ -2,6 +2,7 @@ #define QUICHE_HTTP2_ADAPTER_HTTP2_UTIL_H_ #include "http2/adapter/http2_protocol.h" +#include "http2/adapter/http2_visitor_interface.h" #include "common/platform/api/quiche_export.h" #include "spdy/core/spdy_protocol.h" @@ -13,6 +14,9 @@ QUICHE_EXPORT_PRIVATE Http2ErrorCode TranslateErrorCode(spdy::SpdyErrorCode code); +QUICHE_EXPORT_PRIVATE absl::string_view ConnectionErrorToString( + Http2VisitorInterface::ConnectionError error); + } // namespace adapter } // namespace http2
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h index 4d12e42..f955bfb 100644 --- a/http2/adapter/http2_visitor_interface.h +++ b/http2/adapter/http2_visitor_interface.h
@@ -60,8 +60,18 @@ // bytes were actually sent. May return kSendBlocked or kSendError. virtual int64_t OnReadyToSend(absl::string_view serialized) = 0; - // Called when a connection-level processing error has been encountered. - virtual void OnConnectionError() = 0; + // Called when a connection-level error has occurred. + enum class ConnectionError { + // The peer sent an invalid connection preface. + kInvalidConnectionPreface, + // The visitor encountered an error sending bytes to the peer. + kSendError, + // There was an error reading and framing bytes from the peer. + kParseError, + // The visitor considered a received header to be a connection error. + kHeaderError, + }; + virtual void OnConnectionError(ConnectionError error) = 0; // Called when the header for a frame is received. virtual bool OnFrameHeader(Http2StreamId /*stream_id*/, size_t /*length*/,
diff --git a/http2/adapter/mock_http2_visitor.h b/http2/adapter/mock_http2_visitor.h index 1faf443..a66fe79 100644 --- a/http2/adapter/mock_http2_visitor.h +++ b/http2/adapter/mock_http2_visitor.h
@@ -30,7 +30,7 @@ MOCK_METHOD(int64_t, OnReadyToSend, (absl::string_view serialized), (override)); - MOCK_METHOD(void, OnConnectionError, (), (override)); + MOCK_METHOD(void, OnConnectionError, (ConnectionError error), (override)); MOCK_METHOD(bool, OnFrameHeader, (Http2StreamId stream_id, size_t length, uint8_t type, uint8_t flags),
diff --git a/http2/adapter/nghttp2_adapter.cc b/http2/adapter/nghttp2_adapter.cc index 5da518f..e780562 100644 --- a/http2/adapter/nghttp2_adapter.cc +++ b/http2/adapter/nghttp2_adapter.cc
@@ -3,6 +3,7 @@ #include "absl/algorithm/container.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "http2/adapter/http2_visitor_interface.h" #include "http2/adapter/nghttp2_callbacks.h" #include "http2/adapter/nghttp2_data_provider.h" #include "third_party/nghttp2/src/lib/includes/nghttp2/nghttp2.h" @@ -14,6 +15,8 @@ namespace { +using ConnectionError = Http2VisitorInterface::ConnectionError; + // A metadata source that deletes itself upon completion. class SelfDeletingMetadataSource : public MetadataSource { public: @@ -63,7 +66,7 @@ int64_t NgHttp2Adapter::ProcessBytes(absl::string_view bytes) { const int64_t processed_bytes = session_->ProcessBytes(bytes); if (processed_bytes < 0) { - visitor_.OnConnectionError(); + visitor_.OnConnectionError(ConnectionError::kParseError); } return processed_bytes; } @@ -142,7 +145,7 @@ const int result = nghttp2_session_send(session_->raw_ptr()); if (result != 0) { QUICHE_VLOG(1) << "nghttp2_session_send returned " << result; - visitor_.OnConnectionError(); + visitor_.OnConnectionError(ConnectionError::kSendError); } return result; }
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index b78827a..8fc772d 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -13,6 +13,8 @@ namespace test { namespace { +using ConnectionError = Http2VisitorInterface::ConnectionError; + using testing::_; enum FrameType { @@ -474,7 +476,7 @@ EXPECT_CALL(visitor, OnMetadataForStream(1, _)) .WillOnce(testing::Return(false)); // Remaining frames are not processed due to the error. - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); // The false return from OnMetadataForStream() results in a connection error. @@ -702,7 +704,8 @@ OnHeaderForStream(1, "date", "Tue, 6 Apr 2021 12:54:01 GMT")) .WillOnce( testing::Return(Http2VisitorInterface::HEADER_CONNECTION_ERROR)); - EXPECT_CALL(visitor, OnConnectionError()); + // Translation to nghttp2 treats this error as a general parsing error. + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(-902 /* NGHTTP2_ERR_CALLBACK_FAILURE */, stream_result); @@ -765,7 +768,7 @@ EXPECT_CALL(visitor, OnBeginHeadersForStream(1)) .WillOnce(testing::Return(false)); // Rejecting headers leads to a connection error. - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(NGHTTP2_ERR_CALLBACK_FAILURE, stream_result); @@ -836,7 +839,7 @@ EXPECT_CALL(visitor, OnGoAway(1, Http2ErrorCode::INTERNAL_ERROR, "indigestion")) .WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(NGHTTP2_ERR_CALLBACK_FAILURE, stream_result); @@ -1391,7 +1394,7 @@ auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); visitor.set_has_write_error(); - EXPECT_CALL(visitor, OnConnectionError); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kSendError)); int result = adapter->Send(); EXPECT_EQ(result, NGHTTP2_ERR_CALLBACK_FAILURE); @@ -1721,7 +1724,7 @@ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one")); EXPECT_CALL(visitor, OnEndHeadersForStream(1)) .WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_EQ(-902, result); @@ -1766,7 +1769,7 @@ EXPECT_CALL(visitor, OnFrameHeader(0, 8, PING, 0)) .WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_EQ(-902, result); @@ -1822,7 +1825,7 @@ EXPECT_CALL(visitor, OnFrameHeader(1, 25, DATA, 0)); EXPECT_CALL(visitor, OnBeginDataForStream(1, 25)) .WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_EQ(NGHTTP2_ERR_CALLBACK_FAILURE, result); @@ -1878,7 +1881,7 @@ EXPECT_CALL(visitor, OnFrameHeader(1, 25, DATA, 0)); EXPECT_CALL(visitor, OnBeginDataForStream(1, 25)); EXPECT_CALL(visitor, OnDataForStream(1, _)).WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_EQ(NGHTTP2_ERR_CALLBACK_FAILURE, result);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 38ab9fb..544ae23 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -17,6 +17,8 @@ namespace test { namespace { +using ConnectionError = Http2VisitorInterface::ConnectionError; + using testing::_; enum FrameType { @@ -286,7 +288,7 @@ EXPECT_CALL(visitor, OnMetadataForStream(1, _)) .WillOnce(testing::Return(false)); // Remaining frames are not processed due to the error. - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); // Negative integer returned to indicate an error. @@ -441,7 +443,7 @@ OnHeaderForStream(1, "date", "Tue, 6 Apr 2021 12:54:01 GMT")) .WillOnce( testing::Return(Http2VisitorInterface::HEADER_CONNECTION_ERROR)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kHeaderError)); // Note: OgHttp2Adapter continues processing bytes until the input is // complete. EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0)); @@ -513,7 +515,7 @@ EXPECT_CALL(visitor, OnBeginHeadersForStream(1)) .WillOnce(testing::Return(false)); // Rejecting headers leads to a connection error. - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kHeaderError)); // Note: OgHttp2Adapter continues processing bytes until the input is // complete. EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0)); @@ -677,7 +679,7 @@ // TODO(birenroy): Pass the GOAWAY opaque data through the oghttp2 stack. EXPECT_CALL(visitor, OnGoAway(1, Http2ErrorCode::INTERNAL_ERROR, "")) .WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_LT(stream_result, 0); @@ -827,7 +829,7 @@ auto adapter = OgHttp2Adapter::Create(visitor, options); visitor.set_has_write_error(); - EXPECT_CALL(visitor, OnConnectionError); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kSendError)); int result = adapter->Send(); EXPECT_EQ(result, Http2VisitorInterface::kSendError); @@ -1435,7 +1437,7 @@ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one")); EXPECT_CALL(visitor, OnEndHeadersForStream(1)) .WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_LT(result, 0); @@ -1484,7 +1486,7 @@ EXPECT_CALL(visitor, OnFrameHeader(0, 8, PING, 0)) .WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_LT(result, 0); @@ -1544,7 +1546,7 @@ EXPECT_CALL(visitor, OnFrameHeader(1, 25, DATA, 0)); EXPECT_CALL(visitor, OnBeginDataForStream(1, 25)) .WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_LT(result, 0); @@ -1604,7 +1606,7 @@ EXPECT_CALL(visitor, OnFrameHeader(1, 25, DATA, 0)); EXPECT_CALL(visitor, OnBeginDataForStream(1, 25)); EXPECT_CALL(visitor, OnDataForStream(1, _)).WillOnce(testing::Return(false)); - EXPECT_CALL(visitor, OnConnectionError()); + EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kParseError)); const int64_t result = adapter->ProcessBytes(frames); EXPECT_LT(result, 0);
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 09a3ce0..adcc7b5 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -6,6 +6,7 @@ #include "absl/memory/memory.h" #include "absl/strings/escaping.h" #include "http2/adapter/http2_protocol.h" +#include "http2/adapter/http2_visitor_interface.h" #include "http2/adapter/oghttp2_util.h" #include "spdy/core/spdy_protocol.h" @@ -14,6 +15,8 @@ namespace { +using ConnectionError = Http2VisitorInterface::ConnectionError; + // #define OGHTTP2_DEBUG_TRACE 1 #ifdef OGHTTP2_DEBUG_TRACE @@ -274,7 +277,7 @@ QUICHE_DLOG(INFO) << "Preface doesn't match! Expected: [" << absl::CEscape(remaining_preface_) << "], actual: [" << absl::CEscape(bytes) << "]"; - LatchErrorAndNotify(); + LatchErrorAndNotify(ConnectionError::kInvalidConnectionPreface); return -1; } remaining_preface_.remove_prefix(min_size); @@ -345,7 +348,7 @@ } } if (result < 0) { - LatchErrorAndNotify(); + LatchErrorAndNotify(ConnectionError::kSendError); return result; } else if (!serialized_prefix_.empty()) { return 0; @@ -381,7 +384,7 @@ spdy::SpdySerializedFrame frame = framer_.SerializeFrame(*frame_ptr); const int64_t result = visitor_.OnReadyToSend(absl::string_view(frame)); if (result < 0) { - LatchErrorAndNotify(); + LatchErrorAndNotify(ConnectionError::kSendError); return false; } else if (result == 0) { // Write blocked. @@ -623,7 +626,7 @@ QUICHE_VLOG(1) << "Error: " << http2::Http2DecoderAdapter::SpdyFramerErrorToString(error) << " details: " << detailed_error; - LatchErrorAndNotify(); + LatchErrorAndNotify(ConnectionError::kParseError); } void OgHttp2Session::OnCommonHeader(spdy::SpdyStreamId stream_id, @@ -832,7 +835,7 @@ stream_id, spdy::ERROR_CODE_INTERNAL_ERROR)); } } else if (result == Http2VisitorInterface::HEADER_CONNECTION_ERROR) { - LatchErrorAndNotify(); + LatchErrorAndNotify(ConnectionError::kHeaderError); } } @@ -1004,9 +1007,9 @@ } } -void OgHttp2Session::LatchErrorAndNotify() { +void OgHttp2Session::LatchErrorAndNotify(ConnectionError error) { latched_error_ = true; - visitor_.OnConnectionError(); + visitor_.OnConnectionError(error); } } // namespace adapter
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h index 814b843..7f60302 100644 --- a/http2/adapter/oghttp2_session.h +++ b/http2/adapter/oghttp2_session.h
@@ -279,7 +279,7 @@ // Returns true if the session can create a new stream. bool CanCreateStream() const; - void LatchErrorAndNotify(); + void LatchErrorAndNotify(Http2VisitorInterface::ConnectionError error); // Receives events when inbound frames are parsed. Http2VisitorInterface& visitor_;
diff --git a/http2/adapter/recording_http2_visitor.cc b/http2/adapter/recording_http2_visitor.cc index 4dbe4e2..7e12e3d 100644 --- a/http2/adapter/recording_http2_visitor.cc +++ b/http2/adapter/recording_http2_visitor.cc
@@ -2,6 +2,7 @@ #include "absl/strings/str_format.h" #include "http2/adapter/http2_protocol.h" +#include "http2/adapter/http2_util.h" namespace http2 { namespace adapter { @@ -12,8 +13,9 @@ return serialized.size(); } -void RecordingHttp2Visitor::OnConnectionError() { - events_.push_back("OnConnectionError"); +void RecordingHttp2Visitor::OnConnectionError(ConnectionError error) { + events_.push_back( + absl::StrFormat("OnConnectionError %s", ConnectionErrorToString(error))); } bool RecordingHttp2Visitor::OnFrameHeader(Http2StreamId stream_id,
diff --git a/http2/adapter/recording_http2_visitor.h b/http2/adapter/recording_http2_visitor.h index 8946ad3..4cbc15b 100644 --- a/http2/adapter/recording_http2_visitor.h +++ b/http2/adapter/recording_http2_visitor.h
@@ -21,7 +21,7 @@ // From Http2VisitorInterface int64_t OnReadyToSend(absl::string_view serialized) override; - void OnConnectionError() override; + void OnConnectionError(ConnectionError error) override; bool OnFrameHeader(Http2StreamId stream_id, size_t length, uint8_t type, uint8_t flags) override; void OnSettingsStart() override;
diff --git a/http2/adapter/recording_http2_visitor_test.cc b/http2/adapter/recording_http2_visitor_test.cc index 6763244..d8d3ea7 100644 --- a/http2/adapter/recording_http2_visitor_test.cc +++ b/http2/adapter/recording_http2_visitor_test.cc
@@ -3,6 +3,7 @@ #include <list> #include "http2/adapter/http2_protocol.h" +#include "http2/adapter/http2_visitor_interface.h" #include "http2/test_tools/http2_random.h" #include "common/platform/api/quiche_test.h" @@ -64,7 +65,8 @@ std::list<RecordingHttp2Visitor*> visitors = {&chocolate_visitor, &vanilla_visitor}; for (RecordingHttp2Visitor* visitor : visitors) { - visitor->OnConnectionError(); + visitor->OnConnectionError( + Http2VisitorInterface::ConnectionError::kSendError); visitor->OnFrameHeader(stream_id, length, type, flags); visitor->OnSettingsStart(); visitor->OnSetting(setting);
diff --git a/http2/adapter/test_utils.cc b/http2/adapter/test_utils.cc index 4223b49..210cb66 100644 --- a/http2/adapter/test_utils.cc +++ b/http2/adapter/test_utils.cc
@@ -3,6 +3,7 @@ #include <ostream> #include "absl/strings/str_format.h" +#include "http2/adapter/http2_visitor_interface.h" #include "common/quiche_endian.h" #include "spdy/core/hpack/hpack_encoder.h" #include "spdy/core/spdy_frame_reader.h" @@ -10,6 +11,11 @@ namespace http2 { namespace adapter { namespace test { +namespace { + +using ConnectionError = Http2VisitorInterface::ConnectionError; + +} // anonymous namespace TestDataFrameSource::TestDataFrameSource(Http2VisitorInterface& visitor, bool has_fin) @@ -49,7 +55,7 @@ const int64_t result = visitor_.OnReadyToSend(concatenated); if (result < 0) { // Write encountered error. - visitor_.OnConnectionError(); + visitor_.OnConnectionError(ConnectionError::kSendError); current_fragment_ = {}; payload_fragments_.clear(); return false; @@ -60,7 +66,7 @@ // Probably need to handle this better within this test class. QUICHE_LOG(DFATAL) << "DATA frame not fully flushed. Connection will be corrupt!"; - visitor_.OnConnectionError(); + visitor_.OnConnectionError(ConnectionError::kSendError); current_fragment_ = {}; payload_fragments_.clear(); return false;