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);