Removes the `trailers_require_end_data` option from oghttp2. This option is not set, and makes the code more confusing. My hope is that the simplification helps us debug https://github.com/envoyproxy/envoy/issues/32371. PiperOrigin-RevId: 607833789
diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc index ddaffae..9cc9f57 100644 --- a/quiche/http2/adapter/oghttp2_adapter_test.cc +++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -5064,10 +5064,9 @@ // trailers are queued. TEST(OgHttp2AdapterTest, ServerSubmitsTrailersWhileDataDeferred) { DataSavingVisitor visitor; - for (const bool queue_trailers : {true, false}) { - OgHttp2Adapter::Options options; - options.perspective = Perspective::kServer; - options.trailers_require_end_data = queue_trailers; + OgHttp2Adapter::Options options; + options.perspective = Perspective::kServer; + for (const bool add_more_body_data : {true, false}) { auto adapter = OgHttp2Adapter::Create(visitor, options); const std::string frames = TestFrameSequence() @@ -5137,43 +5136,25 @@ visitor.Clear(); EXPECT_FALSE(adapter->want_write()); + if (add_more_body_data) { + body1_ptr->AppendPayload(" More body! This is ignored."); + } 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()); + // Even though the data source has not finished sending data, the library + // will write the trailers anyway. + EXPECT_TRUE(adapter->want_write()); - body1_ptr->EndData(); - adapter->ResumeStream(1); - EXPECT_TRUE(adapter->want_write()); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, + END_STREAM_FLAG | END_HEADERS_FLAG)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, + END_STREAM_FLAG | END_HEADERS_FLAG, 0)); - EXPECT_CALL( - visitor, - OnBeforeFrameSent(HEADERS, 1, _, END_STREAM_FLAG | END_HEADERS_FLAG)); - EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, - END_STREAM_FLAG | END_HEADERS_FLAG, 0)); - - send_result = adapter->Send(); - EXPECT_EQ(0, send_result); - EXPECT_FALSE(adapter->want_write()); - } 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, _, END_STREAM_FLAG | END_HEADERS_FLAG)); - EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, - END_STREAM_FLAG | END_HEADERS_FLAG, 0)); - - send_result = adapter->Send(); - EXPECT_EQ(0, send_result); - EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::HEADERS})); - EXPECT_FALSE(adapter->want_write()); - } + send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::HEADERS})); + EXPECT_FALSE(adapter->want_write()); } }
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc index 6527c1c..8f36e85 100644 --- a/quiche/http2/adapter/oghttp2_session.cc +++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -789,8 +789,7 @@ } SendResult connection_can_write = SendResult::SEND_OK; - if (state.outbound_body == nullptr || - (!options_.trailers_require_end_data && state.data_deferred)) { + if (state.outbound_body == nullptr || state.data_deferred) { // No data to send, but there might be trailers. if (state.trailers != nullptr) { // Trailers will include END_STREAM, so the data source can be discarded. @@ -820,10 +819,10 @@ << " end_data: " << info.end_data << " trailers: " << state.trailers.get(); if (info.payload_length == 0 && !info.end_data && - (options_.trailers_require_end_data || state.trailers == nullptr)) { + 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. + // entering the "deferred" state only if no trailers are available to + // send. state.data_deferred = true; break; } else if (info.payload_length == DataFrameSource::kError) { @@ -870,11 +869,9 @@ } } if (info.end_data || - (info.payload_length == 0 && state.trailers != nullptr && - !options_.trailers_require_end_data)) { + (info.payload_length == 0 && state.trailers != nullptr)) { // If SelectPayloadLength() returned {0, false}, and there are trailers to - // send, and the safety feature is disabled, it's okay to send the - // trailers. + // send, it's okay to send the trailers. if (state.trailers != nullptr) { auto block_ptr = std::move(state.trailers); if (info.send_fin) { @@ -998,9 +995,7 @@ // Save trailers so they can be written once data is done. state.trailers = std::make_unique<spdy::Http2HeaderBlock>(ToHeaderBlock(trailers)); - if (!options_.trailers_require_end_data || !iter->second.data_deferred) { - trailers_ready_.insert(stream_id); - } + trailers_ready_.insert(stream_id); } return 0; }
diff --git a/quiche/http2/adapter/oghttp2_session.h b/quiche/http2/adapter/oghttp2_session.h index 8213ed6..da88c68 100644 --- a/quiche/http2/adapter/oghttp2_session.h +++ b/quiche/http2/adapter/oghttp2_session.h
@@ -60,10 +60,6 @@ // 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; // Whether to mark all input data as consumed upon encountering a connection // error while processing bytes. If true, subsequent processing will also // mark all input data as consumed.