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;
