Call visitor_.OnConnectionError() when failing to send the connection preface.
This CL brings the connection preface write failure handling in line with
queued frame write failure handling
(http://google3/third_party/http2/adapter/oghttp2_session.cc?l=332-334&rcl=399578585).
This CL also changes a static const member to a static constexpr member. For
some reason, this change was needed to be able to refer to the member outside
of the class (e.g., in a test).
PiperOrigin-RevId: 400037133
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h
index 308f0d8..8297334 100644
--- a/http2/adapter/http2_visitor_interface.h
+++ b/http2/adapter/http2_visitor_interface.h
@@ -52,8 +52,8 @@
Http2VisitorInterface& operator=(const Http2VisitorInterface&) = delete;
virtual ~Http2VisitorInterface() = default;
- static const int64_t kSendBlocked = 0;
- static const int64_t kSendError = -1;
+ static constexpr int64_t kSendBlocked = 0;
+ static constexpr int64_t kSendError = -1;
// Called when there are serialized frames to send. Should return how many
// bytes were actually sent. May return kSendBlocked or kSendError.
virtual int64_t OnReadyToSend(absl::string_view serialized) = 0;
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index a61c55c..ccfb1ac 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -1386,6 +1386,17 @@
EXPECT_FALSE(adapter->want_write());
}
+TEST(NgHttp2AdapterTest, FailureSendingConnectionPreface) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
+
+ visitor.set_has_write_error();
+ EXPECT_CALL(visitor, OnConnectionError);
+
+ int result = adapter->Send();
+ EXPECT_EQ(result, NGHTTP2_ERR_CALLBACK_FAILURE);
+}
+
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 0efb679..f663fb7 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -4,6 +4,7 @@
#include "absl/strings/str_join.h"
#include "http2/adapter/http2_protocol.h"
+#include "http2/adapter/http2_visitor_interface.h"
#include "http2/adapter/mock_http2_visitor.h"
#include "http2/adapter/oghttp2_util.h"
#include "http2/adapter/test_frame_sequence.h"
@@ -822,6 +823,18 @@
EXPECT_FALSE(adapter->want_write());
}
+TEST(OgHttp2AdapterClientTest, FailureSendingConnectionPreface) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kClient};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ visitor.set_has_write_error();
+ EXPECT_CALL(visitor, OnConnectionError);
+
+ int result = adapter->Send();
+ EXPECT_EQ(result, Http2VisitorInterface::kSendError);
+}
+
TEST_F(OgHttp2AdapterTest, SubmitMetadata) {
auto source = absl::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders(
{{"query-cost", "is too darn high"}, {"secret-sauce", "hollandaise"}})));
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index 4359bbf..149a026 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -299,9 +299,13 @@
serialized_prefix_.erase(0, result);
}
}
- if (!serialized_prefix_.empty()) {
- return result < 0 ? result : 0;
+ if (result < 0) {
+ visitor_.OnConnectionError();
+ return result;
+ } else if (!serialized_prefix_.empty()) {
+ return 0;
}
+
bool continue_writing = SendQueuedFrames();
while (continue_writing && !connection_metadata_.empty()) {
continue_writing = SendMetadata(0, connection_metadata_);
diff --git a/http2/adapter/test_utils.h b/http2/adapter/test_utils.h
index 0493788..ec7e95c 100644
--- a/http2/adapter/test_utils.h
+++ b/http2/adapter/test_utils.h
@@ -23,6 +23,9 @@
: public testing::StrictMock<MockHttp2Visitor> {
public:
int64_t OnReadyToSend(absl::string_view data) override {
+ if (has_write_error_) {
+ return kSendError;
+ }
if (is_write_blocked_) {
return kSendBlocked;
}
@@ -63,11 +66,14 @@
bool is_write_blocked() const { return is_write_blocked_; }
void set_is_write_blocked(bool value) { is_write_blocked_ = value; }
+ void set_has_write_error() { has_write_error_ = true; }
+
private:
std::string data_;
absl::flat_hash_map<Http2StreamId, std::vector<std::string>> metadata_map_;
size_t send_limit_ = std::numeric_limits<size_t>::max();
bool is_write_blocked_ = false;
+ bool has_write_error_ = false;
};
// A test DataFrameSource. Starts out in the empty, blocked state.
