Makes a safety mechanism related to trailers configurable, and disabled by default.
Originally, trailers are queued until a data source explicitly indicates the end of data. With this change, invoking `SubmitTrailers()` is taken as a signal that all data has been sent for the stream.
PiperOrigin-RevId: 411608319
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index c49f1f1..d406994 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -1915,10 +1915,7 @@
EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4));
EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
- EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "POST"));
- EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
- EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
- EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
EXPECT_CALL(visitor, OnEndHeadersForStream(1));
EXPECT_CALL(visitor, OnFrameHeader(1, 4, WINDOW_UPDATE, 0));
EXPECT_CALL(visitor, OnWindowUpdate(1, 2000));
@@ -1937,10 +1934,7 @@
EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
int send_result = adapter->Send();
- // Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS ack
- EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS}));
visitor.Clear();
const absl::string_view kBody = "This is an example response body.";
@@ -1963,9 +1957,6 @@
send_result = adapter->Send();
// Some bytes should have been serialized.
EXPECT_EQ(0, send_result);
- // SETTINGS ack
- EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::HEADERS,
- spdy::SpdyFrameType::DATA}));
visitor.Clear();
EXPECT_FALSE(adapter->want_write());
diff --git a/http2/adapter/nghttp2_util.cc b/http2/adapter/nghttp2_util.cc
index 3dc87ab..f3923a7 100644
--- a/http2/adapter/nghttp2_util.cc
+++ b/http2/adapter/nghttp2_util.cc
@@ -182,10 +182,10 @@
QUICHE_LOG(ERROR) << "Source did not use the zero-copy API!";
return {kError, false};
} else {
- if (data_flags & NGHTTP2_DATA_FLAG_NO_END_STREAM) {
- send_fin_ = false;
- }
const bool eof = data_flags & NGHTTP2_DATA_FLAG_EOF;
+ if (eof && (data_flags & NGHTTP2_DATA_FLAG_NO_END_STREAM) == 0) {
+ send_fin_ = true;
+ }
return {result, eof};
}
}
@@ -211,7 +211,7 @@
nghttp2_data_provider provider_;
nghttp2_send_data_callback send_data_;
void* user_data_;
- bool send_fin_ = true;
+ bool send_fin_ = false;
};
std::unique_ptr<DataFrameSource> MakeZeroCopyDataFrameSource(
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index de7d61f..7d0ca8a 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -1975,103 +1975,109 @@
// trailers are queued.
TEST(OgHttp2AdapterServerTest, ServerSubmitsTrailersWhileDataDeferred) {
DataSavingVisitor visitor;
- OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
- auto adapter = OgHttp2Adapter::Create(visitor, options);
+ for (const bool queue_trailers : {true, false}) {
+ OgHttp2Adapter::Options options{
+ .perspective = Perspective::kServer,
+ .trailers_require_end_data = queue_trailers};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
- const std::string frames = TestFrameSequence()
- .ClientPreface()
- .Headers(1,
- {{":method", "POST"},
- {":scheme", "https"},
- {":authority", "example.com"},
- {":path", "/this/is/request/one"}},
- /*fin=*/false)
- .WindowUpdate(1, 2000)
- .Data(1, "This is the request body.")
- .WindowUpdate(0, 2000)
- .Serialize();
- testing::InSequence s;
+ const std::string frames = TestFrameSequence()
+ .ClientPreface()
+ .Headers(1,
+ {{":method", "POST"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}},
+ /*fin=*/false)
+ .WindowUpdate(1, 2000)
+ .Data(1, "This is the request body.")
+ .WindowUpdate(0, 2000)
+ .Serialize();
+ testing::InSequence s;
- // Client preface (empty SETTINGS)
- EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
- EXPECT_CALL(visitor, OnSettingsStart());
- EXPECT_CALL(visitor, OnSettingsEnd());
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
- EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4));
- EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
- EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "POST"));
- EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https"));
- EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com"));
- EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one"));
- EXPECT_CALL(visitor, OnEndHeadersForStream(1));
- EXPECT_CALL(visitor, OnFrameHeader(1, 4, WINDOW_UPDATE, 0));
- EXPECT_CALL(visitor, OnWindowUpdate(1, 2000));
- EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
- EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
- EXPECT_CALL(visitor, OnDataForStream(1, "This is the request body."));
- EXPECT_CALL(visitor, OnFrameHeader(0, 4, WINDOW_UPDATE, 0));
- EXPECT_CALL(visitor, OnWindowUpdate(0, 2000));
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(1, 4, WINDOW_UPDATE, 0));
+ EXPECT_CALL(visitor, OnWindowUpdate(1, 2000));
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, _));
+ EXPECT_CALL(visitor, OnDataForStream(1, "This is the request body."));
+ EXPECT_CALL(visitor, OnFrameHeader(0, 4, WINDOW_UPDATE, 0));
+ EXPECT_CALL(visitor, OnWindowUpdate(0, 2000));
- const int64_t result = adapter->ProcessBytes(frames);
- EXPECT_EQ(frames.size(), static_cast<size_t>(result));
+ const int64_t result = adapter->ProcessBytes(frames);
+ EXPECT_EQ(frames.size(), static_cast<size_t>(result));
- EXPECT_TRUE(adapter->want_write());
+ EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
- EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
- EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
- int send_result = adapter->Send();
- // Some bytes should have been serialized.
- EXPECT_EQ(0, send_result);
- // SETTINGS ack
- EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
- spdy::SpdyFrameType::SETTINGS}));
- visitor.Clear();
+ int send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ visitor.Clear();
- const absl::string_view kBody = "This is an example response body.";
+ const absl::string_view kBody = "This is an example response body.";
- // The body source must indicate that the end of the body is not the end of
- // the stream.
- auto body1 = absl::make_unique<TestDataFrameSource>(visitor, false);
- body1->AppendPayload(kBody);
- auto* body1_ptr = body1.get();
- int submit_result = adapter->SubmitResponse(
- 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}),
- std::move(body1));
- EXPECT_EQ(submit_result, 0);
- EXPECT_TRUE(adapter->want_write());
+ // The body source must indicate that the end of the body is not the end of
+ // the stream.
+ auto body1 = absl::make_unique<TestDataFrameSource>(visitor, false);
+ body1->AppendPayload(kBody);
+ auto* body1_ptr = body1.get();
+ int submit_result = adapter->SubmitResponse(
+ 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}),
+ std::move(body1));
+ EXPECT_EQ(submit_result, 0);
+ EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x4));
- EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0));
- EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x4));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0));
+ EXPECT_CALL(visitor, OnFrameSent(DATA, 1, _, 0x0, 0));
- send_result = adapter->Send();
- // Some bytes should have been serialized.
- EXPECT_EQ(0, send_result);
- // SETTINGS ack
- EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::HEADERS,
- spdy::SpdyFrameType::DATA}));
- visitor.Clear();
- EXPECT_FALSE(adapter->want_write());
+ send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ visitor.Clear();
+ EXPECT_FALSE(adapter->want_write());
- int trailer_result =
- adapter->SubmitTrailer(1, ToHeaders({{"final-status", "a-ok"}}));
- ASSERT_EQ(trailer_result, 0);
- // Even though there are new trailers to write, the data source has not
- // finished writing data and is blocked.
- EXPECT_FALSE(adapter->want_write());
+ int trailer_result =
+ adapter->SubmitTrailer(1, ToHeaders({{"final-status", "a-ok"}}));
+ ASSERT_EQ(trailer_result, 0);
+ if (queue_trailers) {
+ // Even though there are new trailers to write, the data source has not
+ // finished writing data and is blocked.
+ EXPECT_FALSE(adapter->want_write());
- body1_ptr->EndData();
- adapter->ResumeStream(1);
- EXPECT_TRUE(adapter->want_write());
+ body1_ptr->EndData();
+ adapter->ResumeStream(1);
+ EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x5));
- EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x5, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x5));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x5, 0));
- send_result = adapter->Send();
- EXPECT_EQ(0, send_result);
+ send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ } else {
+ // Even though the data source has not finished sending data, the library
+ // will write the trailers anyway.
+ EXPECT_TRUE(adapter->want_write());
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x5));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x5, 0));
+
+ send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::HEADERS}));
+ }
+ }
}
TEST(OgHttp2AdapterServerTest, ServerErrorWhileHandlingHeaders) {
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index 399c90c..168a9a0 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -451,6 +451,7 @@
const Http2StreamId stream_id = write_scheduler_.PopNextReadyStream();
// TODO(birenroy): Add a return value to indicate write blockage, so streams
// aren't woken unnecessarily.
+ QUICHE_VLOG(1) << "Waking stream " << stream_id << " for writes.";
continue_writing = WriteForStream(stream_id);
}
if (continue_writing == SendResult::SEND_OK) {
@@ -570,7 +571,14 @@
bool end_data;
std::tie(length, end_data) =
state.outbound_body->SelectPayloadLength(available_window);
- if (length == 0 && !end_data) {
+ QUICHE_VLOG(2) << "WriteForStream | length: " << length
+ << " end_data: " << end_data
+ << " trailers: " << state.trailers.get();
+ if (length == 0 && !end_data &&
+ (options_.trailers_require_end_data || state.trailers == nullptr)) {
+ // An unproductive call to SelectPayloadLength() results in this stream
+ // entering the "deferred" state only if either no trailers are available
+ // to send, or trailers require an explicit end_data before being sent.
state.data_deferred = true;
break;
} else if (length == DataFrameSource::kError) {
@@ -608,7 +616,11 @@
break;
}
}
- if (end_data) {
+ if (end_data || (length == 0 && state.trailers != nullptr &&
+ !options_.trailers_require_end_data)) {
+ // If SelectPayloadLength() returned {0, false}, and there are trailers to
+ // send, and the safety feature is disabled, it's okay to send the
+ // trailers.
if (state.trailers != nullptr) {
auto block_ptr = std::move(state.trailers);
if (fin) {
@@ -727,6 +739,9 @@
// Save trailers so they can be written once data is done.
state.trailers =
absl::make_unique<spdy::SpdyHeaderBlock>(ToHeaderBlock(trailers));
+ if (!options_.trailers_require_end_data) {
+ iter->second.data_deferred = false;
+ }
if (!iter->second.data_deferred) {
write_scheduler_.MarkStreamReady(stream_id, false);
}
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h
index ad007d4..00d5f3e 100644
--- a/http2/adapter/oghttp2_session.h
+++ b/http2/adapter/oghttp2_session.h
@@ -39,6 +39,10 @@
// Whether (as server) to send a RST_STREAM NO_ERROR when sending a fin on
// an incomplete stream.
bool rst_stream_no_error_when_incomplete = false;
+ // Whether (as server) to queue trailers until after a stream's data source
+ // has indicated the end of data. If false, the server will assume that
+ // submitting trailers indicates the end of data.
+ bool trailers_require_end_data = false;
};
OgHttp2Session(Http2VisitorInterface& visitor, Options options);
