Introduces new `Http2VisitorInterface` methods for DATA frame generation. An outline of changes: * Adds `Http2VisitorInterface::OnReadyToSendDataForStream()`, which the codec calls when it is ready to send a `DATA` frame; the visitor implementation indicates how many bytes can be sent in this frame. * Adds `Http2VisitorInterface::SendDataFrame()`, which the codec calls with a frame header; the visitor implementation should send the frame header and then the indicated number of payload bytes. * Replaces the existing "data provider" implementation with one that calls back through `NgHttp2Adapter`. This allows the adapter to forward the read callback to a `DataFrameSource`, if present for the stream, or to the visitor implementation otherwise. This change brings the QUICHE `oghttp2` API more in line with its predecessor. Library users can continue to use the old behavior for now by passing a `DataFrameSource` to `SubmitRequest()`/`SubmitResponse()`. At some point in the future, the `DataFrameSource` parameter will be removed, and all library users will have to use the new behavior. PiperOrigin-RevId: 634047735
diff --git a/quiche/http2/adapter/callback_visitor.cc b/quiche/http2/adapter/callback_visitor.cc index 7f2f4a4..f03ee40 100644 --- a/quiche/http2/adapter/callback_visitor.cc +++ b/quiche/http2/adapter/callback_visitor.cc
@@ -107,6 +107,22 @@ } } +Http2VisitorInterface::DataFrameHeaderInfo +CallbackVisitor::OnReadyToSendDataForStream(Http2StreamId /*stream_id*/, + size_t /*max_length*/) { + QUICHE_LOG(FATAL) + << "Not implemented; should not be used with nghttp2 callbacks."; + return {}; +} + +bool CallbackVisitor::SendDataFrame(Http2StreamId /*stream_id*/, + absl::string_view /*frame_header*/, + size_t /*payload_bytes*/) { + QUICHE_LOG(FATAL) + << "Not implemented; should not be used with nghttp2 callbacks."; + return false; +} + void CallbackVisitor::OnConnectionError(ConnectionError /*error*/) { QUICHE_VLOG(1) << "OnConnectionError not implemented"; }
diff --git a/quiche/http2/adapter/callback_visitor.h b/quiche/http2/adapter/callback_visitor.h index 2f2336b..f1d7a3a 100644 --- a/quiche/http2/adapter/callback_visitor.h +++ b/quiche/http2/adapter/callback_visitor.h
@@ -29,6 +29,10 @@ void* user_data); int64_t OnReadyToSend(absl::string_view serialized) override; + DataFrameHeaderInfo OnReadyToSendDataForStream(Http2StreamId stream_id, + size_t max_length) override; + bool SendDataFrame(Http2StreamId stream_id, absl::string_view frame_header, + size_t payload_bytes) override; void OnConnectionError(ConnectionError error) override; bool OnFrameHeader(Http2StreamId stream_id, size_t length, uint8_t type, uint8_t flags) override;
diff --git a/quiche/http2/adapter/http2_visitor_interface.h b/quiche/http2/adapter/http2_visitor_interface.h index fa8491d..531c0af 100644 --- a/quiche/http2/adapter/http2_visitor_interface.h +++ b/quiche/http2/adapter/http2_visitor_interface.h
@@ -7,6 +7,7 @@ #include "absl/strings/string_view.h" #include "quiche/http2/adapter/http2_protocol.h" #include "quiche/common/platform/api/quiche_export.h" +#include "quiche/common/platform/api/quiche_logging.h" namespace http2 { namespace adapter { @@ -60,6 +61,34 @@ // bytes were actually sent. May return kSendBlocked or kSendError. virtual int64_t OnReadyToSend(absl::string_view serialized) = 0; + struct DataFrameHeaderInfo { + int64_t payload_length; + bool end_data; + bool end_stream; // If true, also implies end_data. + }; + // Called when the codec is ready to construct a DATA frame header. The + // implementation should return the number of bytes ready to send, and whether + // it's the end of the message body data. If the implementation returns 0 + // bytes and also `end_data` is false, then the stream is deferred until + // resumed by the application. `payload_length` must be at most `max_length`. + // A `payload_length` of -1 indicates that this stream has encountered an + // unrecoverable error. + virtual DataFrameHeaderInfo OnReadyToSendDataForStream( + Http2StreamId /*stream_id*/, size_t /*max_length*/) { + QUICHE_LOG(FATAL) << "Not implemented"; + return DataFrameHeaderInfo{}; + } + + // Called when the codec is ready to send a DATA frame. The implementation + // should send the `frame_header` and specified number of payload bytes. + // Returning false indicates an unrecoverable error. + virtual bool SendDataFrame(Http2StreamId /*stream_id*/, + absl::string_view /*frame_header*/, + size_t /*payload_bytes*/) { + QUICHE_LOG(FATAL) << "Not implemented."; + return false; + } + // Called when a connection-level error has occurred. enum class ConnectionError { // The peer sent an invalid connection preface.
diff --git a/quiche/http2/adapter/mock_http2_visitor.h b/quiche/http2/adapter/mock_http2_visitor.h index 86345d3..40d3c80 100644 --- a/quiche/http2/adapter/mock_http2_visitor.h +++ b/quiche/http2/adapter/mock_http2_visitor.h
@@ -33,6 +33,12 @@ MOCK_METHOD(int64_t, OnReadyToSend, (absl::string_view serialized), (override)); + MOCK_METHOD(DataFrameHeaderInfo, OnReadyToSendDataForStream, + (Http2StreamId stream_id, size_t max_length), (override)); + MOCK_METHOD(bool, SendDataFrame, + (Http2StreamId stream_id, absl::string_view frame_header, + size_t payload_bytes), + (override)); MOCK_METHOD(void, OnConnectionError, (ConnectionError error), (override)); MOCK_METHOD(bool, OnFrameHeader, (Http2StreamId stream_id, size_t length, uint8_t type,
diff --git a/quiche/http2/adapter/nghttp2_adapter.cc b/quiche/http2/adapter/nghttp2_adapter.cc index cdea62b..23593ce 100644 --- a/quiche/http2/adapter/nghttp2_adapter.cc +++ b/quiche/http2/adapter/nghttp2_adapter.cc
@@ -19,6 +19,25 @@ using ConnectionError = Http2VisitorInterface::ConnectionError; +const size_t kFrameHeaderSize = 9; + +// A nghttp2-style `nghttp2_data_source_read_callback`. +ssize_t DataFrameReadCallback(nghttp2_session* /* session */, int32_t stream_id, + uint8_t* /* buf */, size_t length, + uint32_t* data_flags, nghttp2_data_source* source, + void* /* user_data */) { + NgHttp2Adapter* adapter = reinterpret_cast<NgHttp2Adapter*>(source->ptr); + return adapter->DelegateReadCallback(stream_id, length, data_flags); +} + +// A nghttp2-style `nghttp2_send_data_callback`. +int DataFrameSendCallback(nghttp2_session* /* session */, nghttp2_frame* frame, + const uint8_t* framehd, size_t length, + nghttp2_data_source* source, void* /* user_data */) { + NgHttp2Adapter* adapter = reinterpret_cast<NgHttp2Adapter*>(source->ptr); + return adapter->DelegateSendCallback(frame->hd.stream_id, framehd, length); +} + } // anonymous namespace // A metadata source that notifies the owning NgHttp2Adapter upon completion or @@ -225,10 +244,14 @@ absl::Span<const Header> headers, std::unique_ptr<DataFrameSource> data_source, bool end_stream, void* stream_user_data) { - QUICHE_DCHECK_EQ(end_stream, data_source == nullptr); auto nvs = GetNghttp2Nvs(headers); - std::unique_ptr<nghttp2_data_provider> provider = - MakeDataProvider(data_source.get()); + std::unique_ptr<nghttp2_data_provider> provider; + + if (data_source != nullptr || !end_stream) { + provider = std::make_unique<nghttp2_data_provider>(); + provider->source.ptr = this; + provider->read_callback = &DataFrameReadCallback; + } int32_t stream_id = nghttp2_submit_request(session_->raw_ptr(), nullptr, nvs.data(), @@ -246,11 +269,13 @@ absl::Span<const Header> headers, std::unique_ptr<DataFrameSource> data_source, bool end_stream) { - QUICHE_DCHECK_EQ(end_stream, data_source == nullptr); auto nvs = GetNghttp2Nvs(headers); - std::unique_ptr<nghttp2_data_provider> provider = - MakeDataProvider(data_source.get()); - + std::unique_ptr<nghttp2_data_provider> provider; + if (data_source != nullptr || !end_stream) { + provider = std::make_unique<nghttp2_data_provider>(); + provider->source.ptr = this; + provider->read_callback = &DataFrameReadCallback; + } if (data_source != nullptr) { sources_.emplace(stream_id, std::move(data_source)); } @@ -296,6 +321,38 @@ sources_.erase(stream_id); } +ssize_t NgHttp2Adapter::DelegateReadCallback(int32_t stream_id, + size_t max_length, + uint32_t* data_flags) { + auto it = sources_.find(stream_id); + if (it == sources_.end()) { + // A DataFrameSource is not available for this stream; forward to the + // visitor. + return callbacks::VisitorReadCallback(visitor_, stream_id, max_length, + data_flags); + } else { + // A DataFrameSource is available for this stream. + return callbacks::DataFrameSourceReadCallback(*it->second, max_length, + data_flags); + } +} + +int NgHttp2Adapter::DelegateSendCallback(int32_t stream_id, + const uint8_t* framehd, + size_t length) { + auto it = sources_.find(stream_id); + if (it == sources_.end()) { + // A DataFrameSource is not available for this stream; forward to the + // visitor. + visitor_.SendDataFrame(stream_id, ToStringView(framehd, kFrameHeaderSize), + length); + } else { + // A DataFrameSource is available for this stream. + it->second->Send(ToStringView(framehd, kFrameHeaderSize), length); + } + return 0; +} + NgHttp2Adapter::NgHttp2Adapter(Http2VisitorInterface& visitor, Perspective perspective, const nghttp2_option* options) @@ -320,9 +377,9 @@ options_ = owned_options; } - session_ = - std::make_unique<NgHttp2Session>(perspective_, callbacks::Create(), - options_, static_cast<void*>(&visitor_)); + session_ = std::make_unique<NgHttp2Session>( + perspective_, callbacks::Create(&DataFrameSendCallback), options_, + static_cast<void*>(&visitor_)); if (owned_options != nullptr) { nghttp2_option_del(owned_options); }
diff --git a/quiche/http2/adapter/nghttp2_adapter.h b/quiche/http2/adapter/nghttp2_adapter.h index 6f4b405..8fa54e4 100644 --- a/quiche/http2/adapter/nghttp2_adapter.h +++ b/quiche/http2/adapter/nghttp2_adapter.h
@@ -105,6 +105,16 @@ return 0; } + // Delegates a DATA frame read callback to either the visitor or a registered + // DataFrameSource. + ssize_t DelegateReadCallback(int32_t stream_id, size_t max_length, + uint32_t* data_flags); + + // Delegates a DATA frame send callback to either the visitor or a registered + // DataFrameSource. + int DelegateSendCallback(int32_t stream_id, const uint8_t* framehd, + size_t length); + private: class NotifyingMetadataSource;
diff --git a/quiche/http2/adapter/nghttp2_adapter_test.cc b/quiche/http2/adapter/nghttp2_adapter_test.cc index c621176..c03fc1b 100644 --- a/quiche/http2/adapter/nghttp2_adapter_test.cc +++ b/quiche/http2/adapter/nghttp2_adapter_test.cc
@@ -876,9 +876,12 @@ EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS})); } -using NgHttp2AdapterDataTest = quiche::test::QuicheTest; +class NgHttp2AdapterDataTest : public quiche::test::QuicheTestWithParam<bool> { +}; -TEST_F(NgHttp2AdapterDataTest, ClientSendsTrailers) { +INSTANTIATE_TEST_SUITE_P(BothValues, NgHttp2AdapterDataTest, testing::Bool()); + +TEST_P(NgHttp2AdapterDataTest, ClientSendsTrailers) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); @@ -898,11 +901,11 @@ // nghttp2 does not require that the data source indicate the end of data // before trailers are enqueued. - const int32_t stream_id1 = - adapter->SubmitRequest(headers1, std::move(body1), false, nullptr); + const int32_t stream_id1 = adapter->SubmitRequest( + headers1, GetParam() ? nullptr : std::move(body1), false, nullptr); ASSERT_GT(stream_id1, 0); EXPECT_EQ(stream_id1, kStreamId); - EXPECT_EQ(adapter->sources_size(), 1); + EXPECT_EQ(adapter->sources_size(), GetParam() ? 0 : 1); EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x4)); EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x4, 0)); @@ -2046,7 +2049,7 @@ SpdyFrameType::RST_STREAM})); } -TEST_F(NgHttp2AdapterDataTest, ClientSubmitRequest) { +TEST_P(NgHttp2AdapterDataTest, ClientSubmitRequest) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); int result = adapter->Send(); @@ -2085,12 +2088,13 @@ visitor.AppendPayloadForStream(1, kBody); visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - int stream_id = adapter->SubmitRequest( - ToHeaders({{":method", "POST"}, - {":scheme", "http"}, - {":authority", "example.com"}, - {":path", "/this/is/request/one"}}), - std::move(body1), false, const_cast<char*>(kSentinel)); + int stream_id = + adapter->SubmitRequest(ToHeaders({{":method", "POST"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + GetParam() ? nullptr : std::move(body1), false, + const_cast<char*>(kSentinel)); ASSERT_EQ(1, stream_id); EXPECT_TRUE(adapter->want_write()); @@ -2896,7 +2900,7 @@ })); } -TEST_F(NgHttp2AdapterDataTest, ClientObeysMaxConcurrentStreams) { +TEST_P(NgHttp2AdapterDataTest, ClientObeysMaxConcurrentStreams) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); int result = adapter->Send(); @@ -2936,12 +2940,12 @@ visitor.AppendPayloadForStream(1, kBody); visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - const int stream_id = - adapter->SubmitRequest(ToHeaders({{":method", "POST"}, - {":scheme", "http"}, - {":authority", "example.com"}, - {":path", "/this/is/request/one"}}), - std::move(body1), false, nullptr); + const int stream_id = adapter->SubmitRequest( + ToHeaders({{":method", "POST"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + GetParam() ? nullptr : std::move(body1), false, nullptr); EXPECT_GT(stream_id, 0); EXPECT_TRUE(adapter->want_write()); @@ -3013,7 +3017,7 @@ EXPECT_FALSE(adapter->want_write()); } -TEST_F(NgHttp2AdapterDataTest, ClientReceivesInitialWindowSetting) { +TEST_P(NgHttp2AdapterDataTest, ClientReceivesInitialWindowSetting) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); @@ -3052,12 +3056,12 @@ visitor.AppendPayloadForStream(1, kLongBody); visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, true); - const int stream_id = - adapter->SubmitRequest(ToHeaders({{":method", "POST"}, - {":scheme", "http"}, - {":authority", "example.com"}, - {":path", "/this/is/request/one"}}), - std::move(body1), false, nullptr); + const int stream_id = adapter->SubmitRequest( + ToHeaders({{":method", "POST"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + GetParam() ? nullptr : std::move(body1), false, nullptr); EXPECT_GT(stream_id, 0); EXPECT_TRUE(adapter->want_write()); @@ -3075,7 +3079,7 @@ SpdyFrameType::DATA, SpdyFrameType::DATA})); } -TEST_F(NgHttp2AdapterDataTest, +TEST_P(NgHttp2AdapterDataTest, ClientReceivesInitialWindowSettingAfterStreamStart) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); @@ -3106,12 +3110,12 @@ visitor.AppendPayloadForStream(1, kLongBody); visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - const int stream_id = - adapter->SubmitRequest(ToHeaders({{":method", "POST"}, - {":scheme", "http"}, - {":authority", "example.com"}, - {":path", "/this/is/request/one"}}), - std::move(body1), false, nullptr); + const int stream_id = adapter->SubmitRequest( + ToHeaders({{":method", "POST"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + GetParam() ? nullptr : std::move(body1), false, nullptr); EXPECT_GT(stream_id, 0); EXPECT_TRUE(adapter->want_write()); @@ -3681,7 +3685,7 @@ EXPECT_FALSE(adapter->want_write()); } -TEST_F(NgHttp2AdapterDataTest, ConnectionErrorOnDataFrameSent) { +TEST_P(NgHttp2AdapterDataTest, ConnectionErrorOnDataFrameSent) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -3713,8 +3717,9 @@ auto body = std::make_unique<VisitorDataSource>(visitor, 1); visitor.AppendPayloadForStream( 1, "Here is some data, which will lead to a fatal error"); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); EXPECT_TRUE(adapter->want_write()); @@ -4083,7 +4088,7 @@ // Tests the case where the response body is in the progress of being sent while // trailers are queued. -TEST_F(NgHttp2AdapterDataTest, ServerSubmitsTrailersWhileDataDeferred) { +TEST_P(NgHttp2AdapterDataTest, ServerSubmitsTrailersWhileDataDeferred) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -4138,7 +4143,7 @@ auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); int submit_result = adapter->SubmitResponse( 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -4182,7 +4187,7 @@ EXPECT_FALSE(adapter->want_write()); } -TEST_F(NgHttp2AdapterDataTest, ServerSubmitsTrailersWithDataEndStream) { +TEST_P(NgHttp2AdapterDataTest, ServerSubmitsTrailersWithDataEndStream) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -4223,8 +4228,9 @@ visitor.SetEndData(1, true); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(submit_result, 0); const std::vector<Header> trailers = @@ -4250,7 +4256,7 @@ SpdyFrameType::HEADERS})); } -TEST_F(NgHttp2AdapterDataTest, +TEST_P(NgHttp2AdapterDataTest, ServerSubmitsTrailersWithDataEndStreamAndDeferral) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -4292,8 +4298,9 @@ visitor.AppendPayloadForStream(1, kBody); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(submit_result, 0); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, ACK_FLAG)); @@ -5221,7 +5228,7 @@ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::HEADERS})); } -TEST_F(NgHttp2AdapterDataTest, ServerSubmitResponse) { +TEST_P(NgHttp2AdapterDataTest, ServerSubmitResponse) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); EXPECT_FALSE(adapter->want_write()); @@ -5285,7 +5292,7 @@ 1, ToHeaders({{":status", "404"}, {"x-comment", "I have no idea what you're talking about."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -5316,7 +5323,7 @@ EXPECT_GT(adapter->GetHpackEncoderDynamicTableSize(), 0); } -TEST_F(NgHttp2AdapterDataTest, ServerSubmitResponseWithResetFromClient) { +TEST_P(NgHttp2AdapterDataTest, ServerSubmitResponseWithResetFromClient) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); EXPECT_FALSE(adapter->want_write()); @@ -5367,10 +5374,10 @@ 1, ToHeaders({{":status", "404"}, {"x-comment", "I have no idea what you're talking about."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); - EXPECT_EQ(adapter->sources_size(), 1); + EXPECT_EQ(adapter->sources_size(), GetParam() ? 0 : 1); // Client resets the stream before the server can send the response. const std::string reset = @@ -5448,7 +5455,7 @@ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::GOAWAY})); } -TEST_F(NgHttp2AdapterDataTest, ServerSendsTrailers) { +TEST_P(NgHttp2AdapterDataTest, ServerSendsTrailers) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); EXPECT_FALSE(adapter->want_write()); @@ -5502,7 +5509,7 @@ auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); int submit_result = adapter->SubmitResponse( 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -5631,7 +5638,7 @@ absl::StrJoin(visitor.GetMetadata(1), "")); } -TEST_F(NgHttp2AdapterDataTest, RepeatedHeaderNames) { +TEST_P(NgHttp2AdapterDataTest, RepeatedHeaderNames) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); EXPECT_FALSE(adapter->want_write()); @@ -5675,8 +5682,8 @@ visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = - adapter->SubmitResponse(1, headers1, std::move(body1), false); + int submit_result = adapter->SubmitResponse( + 1, headers1, GetParam() ? nullptr : std::move(body1), false); ASSERT_EQ(0, submit_result); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); @@ -5693,7 +5700,7 @@ SpdyFrameType::DATA})); } -TEST_F(NgHttp2AdapterDataTest, ServerRespondsToRequestWithTrailers) { +TEST_P(NgHttp2AdapterDataTest, ServerRespondsToRequestWithTrailers) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); EXPECT_FALSE(adapter->want_write()); @@ -5732,8 +5739,8 @@ const std::vector<Header> headers1 = ToHeaders({{":status", "200"}}); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = - adapter->SubmitResponse(1, headers1, std::move(body1), false); + int submit_result = adapter->SubmitResponse( + 1, headers1, GetParam() ? nullptr : std::move(body1), false); ASSERT_EQ(0, submit_result); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); @@ -5774,7 +5781,7 @@ EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::DATA})); } -TEST_F(NgHttp2AdapterDataTest, ServerSubmitsResponseWithDataSourceError) { +TEST_P(NgHttp2AdapterDataTest, ServerSubmitsResponseWithDataSourceError) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); EXPECT_FALSE(adapter->want_write()); @@ -5812,7 +5819,7 @@ int submit_result = adapter->SubmitResponse( 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -6005,7 +6012,7 @@ EXPECT_EQ(frames.size(), static_cast<size_t>(result)); } -TEST_F(NgHttp2AdapterDataTest, ServerSendsInvalidTrailers) { +TEST_P(NgHttp2AdapterDataTest, ServerSendsInvalidTrailers) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); EXPECT_FALSE(adapter->want_write()); @@ -6047,7 +6054,7 @@ auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); int submit_result = adapter->SubmitResponse( 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -6860,7 +6867,7 @@ EXPECT_FALSE(adapter->want_write()); } -TEST_F(NgHttp2AdapterDataTest, SkipsSendingFramesForRejectedStream) { +TEST_P(NgHttp2AdapterDataTest, SkipsSendingFramesForRejectedStream) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -6894,8 +6901,9 @@ 1, "Here is some data, which will be completely ignored!"); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); auto source = std::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders( @@ -6929,7 +6937,7 @@ SpdyFrameType::RST_STREAM})); } -TEST_F(NgHttp2AdapterDataTest, ServerQueuesMetadataWithStreamReset) { +TEST_P(NgHttp2AdapterDataTest, ServerQueuesMetadataWithStreamReset) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -6962,8 +6970,9 @@ 1, "Here is some data, which will be completely ignored!"); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); auto source = std::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders( @@ -7076,7 +7085,7 @@ EXPECT_EQ(static_cast<size_t>(next_result), next_frame.size()); } -TEST_F(NgHttp2AdapterDataTest, ServerDoesNotSendFramesAfterImmediateGoAway) { +TEST_P(NgHttp2AdapterDataTest, ServerDoesNotSendFramesAfterImmediateGoAway) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -7111,8 +7120,9 @@ // Submit a response for the stream. visitor.AppendPayloadForStream(1, "This data is doomed to never be written."); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); // Submit a WINDOW_UPDATE frame. @@ -7758,7 +7768,7 @@ SpdyFrameType::WINDOW_UPDATE})); } -TEST_F(NgHttp2AdapterDataTest, NegativeFlowControlStreamResumption) { +TEST_P(NgHttp2AdapterDataTest, NegativeFlowControlStreamResumption) { TestVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -7799,8 +7809,9 @@ // Submit a response for the stream. visitor.AppendPayloadForStream(1, std::string(70000, 'a')); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
diff --git a/quiche/http2/adapter/nghttp2_callbacks.cc b/quiche/http2/adapter/nghttp2_callbacks.cc index 200a709..6a93e43 100644 --- a/quiche/http2/adapter/nghttp2_callbacks.cc +++ b/quiche/http2/adapter/nghttp2_callbacks.cc
@@ -348,7 +348,8 @@ return 0; } -nghttp2_session_callbacks_unique_ptr Create() { +nghttp2_session_callbacks_unique_ptr Create( + nghttp2_send_data_callback send_data_callback) { nghttp2_session_callbacks* callbacks; nghttp2_session_callbacks_new(&callbacks); @@ -372,8 +373,8 @@ nghttp2_session_callbacks_set_on_invalid_frame_recv_callback( callbacks, &OnInvalidFrameReceived); nghttp2_session_callbacks_set_error_callback2(callbacks, &OnError); - nghttp2_session_callbacks_set_send_data_callback( - callbacks, &DataFrameSourceSendCallback); + nghttp2_session_callbacks_set_send_data_callback(callbacks, + send_data_callback); nghttp2_session_callbacks_set_pack_extension_callback( callbacks, &OnPackExtensionCallback); nghttp2_session_callbacks_set_unpack_extension_callback(
diff --git a/quiche/http2/adapter/nghttp2_callbacks.h b/quiche/http2/adapter/nghttp2_callbacks.h index dfdff06..367db37 100644 --- a/quiche/http2/adapter/nghttp2_callbacks.h +++ b/quiche/http2/adapter/nghttp2_callbacks.h
@@ -81,7 +81,8 @@ int OnError(nghttp2_session* session, int lib_error_code, const char* msg, size_t len, void* user_data); -nghttp2_session_callbacks_unique_ptr Create(); +nghttp2_session_callbacks_unique_ptr Create( + nghttp2_send_data_callback send_data_callback); } // namespace callbacks } // namespace adapter
diff --git a/quiche/http2/adapter/nghttp2_data_provider.cc b/quiche/http2/adapter/nghttp2_data_provider.cc index c0d76d1..3475210 100644 --- a/quiche/http2/adapter/nghttp2_data_provider.cc +++ b/quiche/http2/adapter/nghttp2_data_provider.cc
@@ -9,18 +9,29 @@ namespace adapter { namespace callbacks { -namespace { -const size_t kFrameHeaderSize = 9; +ssize_t VisitorReadCallback(Http2VisitorInterface& visitor, int32_t stream_id, + size_t max_length, uint32_t* data_flags) { + *data_flags |= NGHTTP2_DATA_FLAG_NO_COPY; + auto [payload_length, end_data, end_stream] = + visitor.OnReadyToSendDataForStream(stream_id, max_length); + if (payload_length == 0 && !end_data) { + return NGHTTP2_ERR_DEFERRED; + } else if (payload_length == DataFrameSource::kError) { + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; + } + if (end_data) { + *data_flags |= NGHTTP2_DATA_FLAG_EOF; + } + if (!end_stream) { + *data_flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; + } + return payload_length; } -ssize_t DataFrameSourceReadCallback(nghttp2_session* /* session */, - int32_t /* stream_id */, uint8_t* /* buf */, - size_t length, uint32_t* data_flags, - nghttp2_data_source* source, - void* /* user_data */) { +ssize_t DataFrameSourceReadCallback(DataFrameSource& source, size_t length, + uint32_t* data_flags) { *data_flags |= NGHTTP2_DATA_FLAG_NO_COPY; - auto* frame_source = static_cast<DataFrameSource*>(source->ptr); - auto [result_length, done] = frame_source->SelectPayloadLength(length); + auto [result_length, done] = source.SelectPayloadLength(length); if (result_length == 0 && !done) { return NGHTTP2_ERR_DEFERRED; } else if (result_length == DataFrameSource::kError) { @@ -29,34 +40,12 @@ if (done) { *data_flags |= NGHTTP2_DATA_FLAG_EOF; } - if (!frame_source->send_fin()) { + if (!source.send_fin()) { *data_flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; } return result_length; } -int DataFrameSourceSendCallback(nghttp2_session* /* session */, - nghttp2_frame* /* frame */, - const uint8_t* framehd, size_t length, - nghttp2_data_source* source, - void* /* user_data */) { - auto* frame_source = static_cast<DataFrameSource*>(source->ptr); - frame_source->Send(ToStringView(framehd, kFrameHeaderSize), length); - return 0; -} - } // namespace callbacks - -std::unique_ptr<nghttp2_data_provider> MakeDataProvider( - DataFrameSource* source) { - if (source == nullptr) { - return nullptr; - } - auto provider = std::make_unique<nghttp2_data_provider>(); - provider->source.ptr = source; - provider->read_callback = &callbacks::DataFrameSourceReadCallback; - return provider; -} - } // namespace adapter } // namespace http2
diff --git a/quiche/http2/adapter/nghttp2_data_provider.h b/quiche/http2/adapter/nghttp2_data_provider.h index a3f0957..a0c9cd9 100644 --- a/quiche/http2/adapter/nghttp2_data_provider.h +++ b/quiche/http2/adapter/nghttp2_data_provider.h
@@ -5,32 +5,24 @@ #include <memory> #include "quiche/http2/adapter/data_source.h" +#include "quiche/http2/adapter/http2_visitor_interface.h" #include "quiche/http2/adapter/nghttp2.h" namespace http2 { namespace adapter { namespace callbacks { -// Assumes |source| is a DataFrameSource. -ssize_t DataFrameSourceReadCallback(nghttp2_session* /*session */, - int32_t /* stream_id */, uint8_t* /* buf */, - size_t length, uint32_t* data_flags, - nghttp2_data_source* source, - void* /* user_data */); +// A callback that returns DATA frame payload size and associated flags, given a +// Http2VisitorInterface. +ssize_t VisitorReadCallback(Http2VisitorInterface& visitor, int32_t stream_id, + size_t max_length, uint32_t* data_flags); -int DataFrameSourceSendCallback(nghttp2_session* /* session */, - nghttp2_frame* /* frame */, - const uint8_t* framehd, size_t length, - nghttp2_data_source* source, - void* /* user_data */); +// A callback that returns DATA frame payload size and associated flags, given a +// DataFrameSource. +ssize_t DataFrameSourceReadCallback(DataFrameSource& source, size_t length, + uint32_t* data_flags); } // namespace callbacks - -// Transforms a DataFrameSource into a nghttp2_data_provider. Does not take -// ownership of |source|. Returns nullptr if |source| is nullptr. -std::unique_ptr<nghttp2_data_provider> MakeDataProvider( - DataFrameSource* source); - } // namespace adapter } // namespace http2
diff --git a/quiche/http2/adapter/nghttp2_data_provider_test.cc b/quiche/http2/adapter/nghttp2_data_provider_test.cc index caaaf51..6ebc8b9 100644 --- a/quiche/http2/adapter/nghttp2_data_provider_test.cc +++ b/quiche/http2/adapter/nghttp2_data_provider_test.cc
@@ -1,5 +1,6 @@ #include "quiche/http2/adapter/nghttp2_data_provider.h" +#include "quiche/http2/adapter/nghttp2_util.h" #include "quiche/http2/adapter/test_utils.h" #include "quiche/common/platform/api/quiche_test.h" @@ -9,105 +10,175 @@ const size_t kFrameHeaderSize = 9; -// Verifies that a nghttp2_data_provider derived from a DataFrameSource works -// correctly with nghttp2-style callbacks when the amount of data read is less -// than what the source provides. -TEST(DataProviderTest, ReadLessThanSourceProvides) { +// Verifies that the DataFrameSource read callback works correctly when the +// amount of data read is less than what the source provides. +TEST(DataFrameSourceTest, ReadLessThanSourceProvides) { const int32_t kStreamId = 1; TestVisitor visitor; visitor.AppendPayloadForStream(kStreamId, "Example payload"); visitor.SetEndData(kStreamId, true); VisitorDataSource source(visitor, kStreamId); - auto provider = MakeDataProvider(&source); uint32_t data_flags = 0; const size_t kReadLength = 10; // Read callback selects a payload length given an upper bound. ssize_t result = - provider->read_callback(nullptr, kStreamId, nullptr, kReadLength, - &data_flags, &provider->source, nullptr); + callbacks::DataFrameSourceReadCallback(source, kReadLength, &data_flags); ASSERT_EQ(kReadLength, result); EXPECT_EQ(NGHTTP2_DATA_FLAG_NO_COPY | NGHTTP2_DATA_FLAG_NO_END_STREAM, data_flags); const uint8_t framehd[kFrameHeaderSize] = {1, 2, 3, 4, 5, 6, 7, 8, 9}; // Sends the frame header and some payload bytes. - int send_result = callbacks::DataFrameSourceSendCallback( - nullptr, nullptr, framehd, result, &provider->source, nullptr); - EXPECT_EQ(0, send_result); + source.Send(ToStringView(framehd, kFrameHeaderSize), result); // Data accepted by the visitor includes a frame header and kReadLength bytes // of payload. EXPECT_EQ(visitor.data().size(), kFrameHeaderSize + kReadLength); } -// Verifies that a nghttp2_data_provider derived from a DataFrameSource works -// correctly with nghttp2-style callbacks when the amount of data read is more -// than what the source provides. -TEST(DataProviderTest, ReadMoreThanSourceProvides) { +// Verifies that the Visitor read callback works correctly when the amount of +// data read is less than what the source provides. +TEST(VisitorTest, ReadLessThanSourceProvides) { + const int32_t kStreamId = 1; + TestVisitor visitor; + visitor.AppendPayloadForStream(kStreamId, "Example payload"); + visitor.SetEndData(kStreamId, true); + uint32_t data_flags = 0; + const size_t kReadLength = 10; + // Read callback selects a payload length given an upper bound. + ssize_t result = callbacks::VisitorReadCallback(visitor, kStreamId, + kReadLength, &data_flags); + ASSERT_EQ(kReadLength, result); + EXPECT_EQ(NGHTTP2_DATA_FLAG_NO_COPY | NGHTTP2_DATA_FLAG_NO_END_STREAM, + data_flags); + + const uint8_t framehd[kFrameHeaderSize] = {1, 2, 3, 4, 5, 6, 7, 8, 9}; + // Sends the frame header and some payload bytes. + visitor.SendDataFrame(kStreamId, ToStringView(framehd, kFrameHeaderSize), + result); + // Data accepted by the visitor includes a frame header and kReadLength bytes + // of payload. + EXPECT_EQ(visitor.data().size(), kFrameHeaderSize + kReadLength); +} + +// Verifies that the DataFrameSource read callback works correctly when the +// amount of data read is more than what the source provides. +TEST(DataFrameSourceTest, ReadMoreThanSourceProvides) { const int32_t kStreamId = 1; const absl::string_view kPayload = "Example payload"; TestVisitor visitor; visitor.AppendPayloadForStream(kStreamId, kPayload); visitor.SetEndData(kStreamId, true); VisitorDataSource source(visitor, kStreamId); - auto provider = MakeDataProvider(&source); uint32_t data_flags = 0; const size_t kReadLength = 30; // Read callback selects a payload length given an upper bound. ssize_t result = - provider->read_callback(nullptr, kStreamId, nullptr, kReadLength, - &data_flags, &provider->source, nullptr); + callbacks::DataFrameSourceReadCallback(source, kReadLength, &data_flags); ASSERT_EQ(kPayload.size(), result); EXPECT_EQ(NGHTTP2_DATA_FLAG_NO_COPY | NGHTTP2_DATA_FLAG_EOF, data_flags); const uint8_t framehd[kFrameHeaderSize] = {1, 2, 3, 4, 5, 6, 7, 8, 9}; // Sends the frame header and some payload bytes. - int send_result = callbacks::DataFrameSourceSendCallback( - nullptr, nullptr, framehd, result, &provider->source, nullptr); - EXPECT_EQ(0, send_result); + source.Send(ToStringView(framehd, kFrameHeaderSize), result); // Data accepted by the visitor includes a frame header and the entire // payload. EXPECT_EQ(visitor.data().size(), kFrameHeaderSize + kPayload.size()); } -// Verifies that a nghttp2_data_provider derived from a DataFrameSource works -// correctly with nghttp2-style callbacks when the source is blocked. -TEST(DataProviderTest, ReadFromBlockedSource) { +// Verifies that the Visitor read callback works correctly when the amount of +// data read is more than what the source provides. +TEST(VisitorTest, ReadMoreThanSourceProvides) { + const int32_t kStreamId = 1; + const absl::string_view kPayload = "Example payload"; + TestVisitor visitor; + visitor.AppendPayloadForStream(kStreamId, kPayload); + visitor.SetEndData(kStreamId, true); + VisitorDataSource source(visitor, kStreamId); + uint32_t data_flags = 0; + const size_t kReadLength = 30; + // Read callback selects a payload length given an upper bound. + ssize_t result = callbacks::VisitorReadCallback(visitor, kStreamId, + kReadLength, &data_flags); + ASSERT_EQ(kPayload.size(), result); + EXPECT_EQ(NGHTTP2_DATA_FLAG_NO_COPY | NGHTTP2_DATA_FLAG_EOF, data_flags); + + const uint8_t framehd[kFrameHeaderSize] = {1, 2, 3, 4, 5, 6, 7, 8, 9}; + // Sends the frame header and some payload bytes. + visitor.SendDataFrame(kStreamId, ToStringView(framehd, kFrameHeaderSize), + result); + // Data accepted by the visitor includes a frame header and the entire + // payload. + EXPECT_EQ(visitor.data().size(), kFrameHeaderSize + kPayload.size()); +} + +// Verifies that the DataFrameSource read callback works correctly when the +// source is blocked. +TEST(DataFrameSourceTest, ReadFromBlockedSource) { const int32_t kStreamId = 1; TestVisitor visitor; // Source has no payload, but also no fin, so it's blocked. VisitorDataSource source(visitor, kStreamId); - auto provider = MakeDataProvider(&source); uint32_t data_flags = 0; const size_t kReadLength = 10; ssize_t result = - provider->read_callback(nullptr, kStreamId, nullptr, kReadLength, - &data_flags, &provider->source, nullptr); + callbacks::DataFrameSourceReadCallback(source, kReadLength, &data_flags); // Read operation is deferred, since the source is blocked. EXPECT_EQ(NGHTTP2_ERR_DEFERRED, result); } -// Verifies that a nghttp2_data_provider derived from a DataFrameSource works -// correctly with nghttp2-style callbacks when the source provides only fin and -// no data. -TEST(DataProviderTest, ReadFromZeroLengthSource) { +// Verifies that the Visitor read callback works correctly when the source is +// blocked. +TEST(VisitorTest, ReadFromBlockedSource) { + const int32_t kStreamId = 1; + TestVisitor visitor; + // Stream has no payload, but also no fin, so it's blocked. + uint32_t data_flags = 0; + const size_t kReadLength = 10; + ssize_t result = callbacks::VisitorReadCallback(visitor, kStreamId, + kReadLength, &data_flags); + // Read operation is deferred, since the source is blocked. + EXPECT_EQ(NGHTTP2_ERR_DEFERRED, result); +} + +// Verifies that the DataFrameSource read callback works correctly when the +// source provides only fin and no data. +TEST(DataFrameSourceTest, ReadFromZeroLengthSource) { const int32_t kStreamId = 1; TestVisitor visitor; visitor.SetEndData(kStreamId, true); // Empty payload and fin=true indicates the source is done. VisitorDataSource source(visitor, kStreamId); - auto provider = MakeDataProvider(&source); uint32_t data_flags = 0; const size_t kReadLength = 10; ssize_t result = - provider->read_callback(nullptr, kStreamId, nullptr, kReadLength, - &data_flags, &provider->source, nullptr); + callbacks::DataFrameSourceReadCallback(source, kReadLength, &data_flags); ASSERT_EQ(0, result); EXPECT_EQ(NGHTTP2_DATA_FLAG_NO_COPY | NGHTTP2_DATA_FLAG_EOF, data_flags); const uint8_t framehd[kFrameHeaderSize] = {1, 2, 3, 4, 5, 6, 7, 8, 9}; - int send_result = callbacks::DataFrameSourceSendCallback( - nullptr, nullptr, framehd, result, &provider->source, nullptr); - EXPECT_EQ(0, send_result); + source.Send(ToStringView(framehd, kFrameHeaderSize), result); + // Data accepted by the visitor includes a frame header with fin and zero + // bytes of payload. + EXPECT_EQ(visitor.data().size(), kFrameHeaderSize); +} + +// Verifies that the Visitor read callback works correctly when the source +// provides only fin and no data. +TEST(VisitorTest, ReadFromZeroLengthSource) { + const int32_t kStreamId = 1; + TestVisitor visitor; + // Empty payload and fin=true indicates the source is done. + visitor.SetEndData(kStreamId, true); + uint32_t data_flags = 0; + const size_t kReadLength = 10; + ssize_t result = callbacks::VisitorReadCallback(visitor, kStreamId, + kReadLength, &data_flags); + ASSERT_EQ(0, result); + EXPECT_EQ(NGHTTP2_DATA_FLAG_NO_COPY | NGHTTP2_DATA_FLAG_EOF, data_flags); + + const uint8_t framehd[kFrameHeaderSize] = {1, 2, 3, 4, 5, 6, 7, 8, 9}; + visitor.SendDataFrame(kStreamId, ToStringView(framehd, kFrameHeaderSize), + result); // Data accepted by the visitor includes a frame header with fin and zero // bytes of payload. EXPECT_EQ(visitor.data().size(), kFrameHeaderSize);
diff --git a/quiche/http2/adapter/nghttp2_session_test.cc b/quiche/http2/adapter/nghttp2_session_test.cc index ab3f337..164971b 100644 --- a/quiche/http2/adapter/nghttp2_session_test.cc +++ b/quiche/http2/adapter/nghttp2_session_test.cc
@@ -37,7 +37,7 @@ void TearDown() override { nghttp2_option_del(options_); } nghttp2_session_callbacks_unique_ptr CreateCallbacks() { - nghttp2_session_callbacks_unique_ptr callbacks = callbacks::Create(); + nghttp2_session_callbacks_unique_ptr callbacks = callbacks::Create(nullptr); return callbacks; }
diff --git a/quiche/http2/adapter/oghttp2_adapter.cc b/quiche/http2/adapter/oghttp2_adapter.cc index e232307..0a827bd 100644 --- a/quiche/http2/adapter/oghttp2_adapter.cc +++ b/quiche/http2/adapter/oghttp2_adapter.cc
@@ -134,7 +134,6 @@ absl::Span<const Header> headers, std::unique_ptr<DataFrameSource> data_source, bool end_stream, void* user_data) { - QUICHE_DCHECK_EQ(end_stream, data_source == nullptr); return session_->SubmitRequest(headers, std::move(data_source), end_stream, user_data); } @@ -143,7 +142,6 @@ absl::Span<const Header> headers, std::unique_ptr<DataFrameSource> data_source, bool end_stream) { - QUICHE_DCHECK_EQ(end_stream, data_source == nullptr); return session_->SubmitResponse(stream_id, headers, std::move(data_source), end_stream); }
diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc index 41010cb..14fba31 100644 --- a/quiche/http2/adapter/oghttp2_adapter_test.cc +++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -1402,9 +1402,12 @@ EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS})); } -using OgHttp2AdapterDataTest = quiche::test::QuicheTest; +class OgHttp2AdapterDataTest : public quiche::test::QuicheTestWithParam<bool> { +}; -TEST_F(OgHttp2AdapterDataTest, ClientSendsTrailers) { +INSTANTIATE_TEST_SUITE_P(BothValues, OgHttp2AdapterDataTest, testing::Bool()); + +TEST_P(OgHttp2AdapterDataTest, ClientSendsTrailers) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kClient; @@ -1423,8 +1426,8 @@ visitor.SetEndData(1, false); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - const int32_t stream_id1 = - adapter->SubmitRequest(headers1, std::move(body1), false, nullptr); + const int32_t stream_id1 = adapter->SubmitRequest( + headers1, GetParam() ? nullptr : std::move(body1), false, nullptr); ASSERT_EQ(stream_id1, 1); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); @@ -2628,7 +2631,7 @@ SpdyFrameType::RST_STREAM})); } -TEST_F(OgHttp2AdapterDataTest, ClientObeysMaxConcurrentStreams) { +TEST_P(OgHttp2AdapterDataTest, ClientObeysMaxConcurrentStreams) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kClient; @@ -2681,12 +2684,12 @@ visitor.AppendPayloadForStream(1, kBody); visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - const int stream_id = - adapter->SubmitRequest(ToHeaders({{":method", "POST"}, - {":scheme", "http"}, - {":authority", "example.com"}, - {":path", "/this/is/request/one"}}), - std::move(body1), false, nullptr); + const int stream_id = adapter->SubmitRequest( + ToHeaders({{":method", "POST"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + GetParam() ? nullptr : std::move(body1), false, nullptr); ASSERT_EQ(stream_id, 1); EXPECT_TRUE(adapter->want_write()); @@ -2760,7 +2763,7 @@ EXPECT_FALSE(adapter->want_write()); } -TEST_F(OgHttp2AdapterDataTest, ClientReceivesInitialWindowSetting) { +TEST_P(OgHttp2AdapterDataTest, ClientReceivesInitialWindowSetting) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kClient; @@ -2804,12 +2807,12 @@ visitor.AppendPayloadForStream(1, kLongBody); visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - const int stream_id = - adapter->SubmitRequest(ToHeaders({{":method", "POST"}, - {":scheme", "http"}, - {":authority", "example.com"}, - {":path", "/this/is/request/one"}}), - std::move(body1), false, nullptr); + const int stream_id = adapter->SubmitRequest( + ToHeaders({{":method", "POST"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + GetParam() ? nullptr : std::move(body1), false, nullptr); EXPECT_GT(stream_id, 0); EXPECT_TRUE(adapter->want_write()); @@ -2827,7 +2830,7 @@ SpdyFrameType::DATA, SpdyFrameType::DATA})); } -TEST_F(OgHttp2AdapterDataTest, +TEST_P(OgHttp2AdapterDataTest, ClientReceivesInitialWindowSettingAfterStreamStart) { TestVisitor visitor; OgHttp2Adapter::Options options; @@ -2862,12 +2865,12 @@ visitor.AppendPayloadForStream(1, kLongBody); visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - const int stream_id = - adapter->SubmitRequest(ToHeaders({{":method", "POST"}, - {":scheme", "http"}, - {":authority", "example.com"}, - {":path", "/this/is/request/one"}}), - std::move(body1), false, nullptr); + const int stream_id = adapter->SubmitRequest( + ToHeaders({{":method", "POST"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}), + GetParam() ? nullptr : std::move(body1), false, nullptr); EXPECT_GT(stream_id, 0); EXPECT_TRUE(adapter->want_write()); @@ -3451,7 +3454,7 @@ EXPECT_FALSE(adapter->want_write()); } -TEST_F(OgHttp2AdapterDataTest, ClientEncountersFlowControlBlock) { +TEST_P(OgHttp2AdapterDataTest, ClientEncountersFlowControlBlock) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kClient; @@ -3470,8 +3473,8 @@ visitor.SetEndData(1, false); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - const int32_t stream_id1 = - adapter->SubmitRequest(headers1, std::move(body1), false, nullptr); + const int32_t stream_id1 = adapter->SubmitRequest( + headers1, GetParam() ? nullptr : std::move(body1), false, nullptr); ASSERT_GT(stream_id1, 0); const std::vector<Header> headers2 = @@ -3484,8 +3487,8 @@ visitor.SetEndData(3, false); auto body2 = std::make_unique<VisitorDataSource>(visitor, 3); - const int32_t stream_id2 = - adapter->SubmitRequest(headers2, std::move(body2), false, nullptr); + const int32_t stream_id2 = adapter->SubmitRequest( + headers2, GetParam() ? nullptr : std::move(body2), false, nullptr); ASSERT_EQ(stream_id2, 3); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); @@ -3534,7 +3537,7 @@ EXPECT_EQ(0, result); } -TEST_F(OgHttp2AdapterDataTest, ClientSendsTrailersAfterFlowControlBlock) { +TEST_P(OgHttp2AdapterDataTest, ClientSendsTrailersAfterFlowControlBlock) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kClient; @@ -3552,8 +3555,8 @@ visitor.SetEndData(1, false); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - const int32_t stream_id1 = - adapter->SubmitRequest(headers1, std::move(body1), false, nullptr); + const int32_t stream_id1 = adapter->SubmitRequest( + headers1, GetParam() ? nullptr : std::move(body1), false, nullptr); ASSERT_GT(stream_id1, 0); const std::vector<Header> headers2 = @@ -3567,8 +3570,8 @@ visitor.SetEndData(3, false); auto body2 = std::make_unique<VisitorDataSource>(visitor, 3); - const int32_t stream_id2 = - adapter->SubmitRequest(headers2, std::move(body2), false, nullptr); + const int32_t stream_id2 = adapter->SubmitRequest( + headers2, GetParam() ? nullptr : std::move(body2), false, nullptr); ASSERT_GT(stream_id2, 0); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); @@ -4195,7 +4198,7 @@ EXPECT_LT(send_result, 0); } -TEST_F(OgHttp2AdapterDataTest, ConnectionErrorOnDataFrameSent) { +TEST_P(OgHttp2AdapterDataTest, ConnectionErrorOnDataFrameSent) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -4230,8 +4233,9 @@ visitor.AppendPayloadForStream( 1, "Here is some data, which will lead to a fatal error"); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); EXPECT_TRUE(adapter->want_write()); @@ -4299,7 +4303,7 @@ EXPECT_EQ(frames.size(), static_cast<size_t>(result)); } -TEST_F(OgHttp2AdapterDataTest, RepeatedHeaderNames) { +TEST_P(OgHttp2AdapterDataTest, RepeatedHeaderNames) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -4345,8 +4349,8 @@ visitor.SetEndData(1, true); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = - adapter->SubmitResponse(1, headers1, std::move(body1), false); + int submit_result = adapter->SubmitResponse( + 1, headers1, GetParam() ? nullptr : std::move(body1), false); ASSERT_EQ(0, submit_result); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); @@ -4365,7 +4369,7 @@ SpdyFrameType::HEADERS, SpdyFrameType::DATA})); } -TEST_F(OgHttp2AdapterDataTest, ServerRespondsToRequestWithTrailers) { +TEST_P(OgHttp2AdapterDataTest, ServerRespondsToRequestWithTrailers) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -4406,8 +4410,8 @@ const std::vector<Header> headers1 = ToHeaders({{":status", "200"}}); auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = - adapter->SubmitResponse(1, headers1, std::move(body1), false); + int submit_result = adapter->SubmitResponse( + 1, headers1, GetParam() ? nullptr : std::move(body1), false); ASSERT_EQ(0, submit_result); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); @@ -4568,7 +4572,7 @@ SpdyFrameType::RST_STREAM})); } -TEST_F(OgHttp2AdapterDataTest, ServerSubmitsResponseWithDataSourceError) { +TEST_P(OgHttp2AdapterDataTest, ServerSubmitsResponseWithDataSourceError) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -4607,7 +4611,7 @@ auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); int submit_result = adapter->SubmitResponse( 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -4865,7 +4869,7 @@ EXPECT_EQ(frames.size(), static_cast<size_t>(result)); } -TEST_F(OgHttp2AdapterDataTest, ServerSendsInvalidTrailers) { +TEST_P(OgHttp2AdapterDataTest, ServerSendsInvalidTrailers) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -4909,7 +4913,7 @@ auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); int submit_result = adapter->SubmitResponse( 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -5160,7 +5164,7 @@ // Tests the case where the response body is in the progress of being sent while // trailers are queued. -TEST_F(OgHttp2AdapterDataTest, ServerSubmitsTrailersWhileDataDeferred) { +TEST_P(OgHttp2AdapterDataTest, ServerSubmitsTrailersWhileDataDeferred) { OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; for (const bool add_more_body_data : {true, false}) { @@ -5220,7 +5224,7 @@ auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); int submit_result = adapter->SubmitResponse( 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -5258,7 +5262,7 @@ // Tests the case where the response body and trailers become blocked by flow // control while the stream is writing. Regression test for // https://github.com/envoyproxy/envoy/issues/31710 -TEST_F(OgHttp2AdapterDataTest, ServerSubmitsTrailersWithFlowControlBlockage) { +TEST_P(OgHttp2AdapterDataTest, ServerSubmitsTrailersWithFlowControlBlockage) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -5312,7 +5316,7 @@ auto body1 = std::make_unique<VisitorDataSource>(visitor, 1); int submit_result = adapter->SubmitResponse( 1, ToHeaders({{":status", "200"}, {"x-comment", "Sure, sounds good."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -5374,7 +5378,7 @@ EXPECT_FALSE(adapter->want_write()); } -TEST_F(OgHttp2AdapterDataTest, ServerSubmitsTrailersWithDataEndStream) { +TEST_P(OgHttp2AdapterDataTest, ServerSubmitsTrailersWithDataEndStream) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -5417,8 +5421,9 @@ visitor.SetEndData(1, true); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(submit_result, 0); const std::vector<Header> trailers = @@ -5444,7 +5449,7 @@ SpdyFrameType::HEADERS, SpdyFrameType::DATA})); } -TEST_F(OgHttp2AdapterDataTest, +TEST_P(OgHttp2AdapterDataTest, ServerSubmitsTrailersWithDataEndStreamAndDeferral) { TestVisitor visitor; OgHttp2Adapter::Options options; @@ -5488,8 +5493,9 @@ visitor.AppendPayloadForStream(1, kBody); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(submit_result, 0); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); @@ -6407,7 +6413,7 @@ SpdyFrameType::HEADERS})); } -TEST_F(OgHttp2AdapterDataTest, ServerSubmitResponse) { +TEST_P(OgHttp2AdapterDataTest, ServerSubmitResponse) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -6476,7 +6482,7 @@ 1, ToHeaders({{":status", "404"}, {"x-comment", "I have no idea what you're talking about."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -6507,7 +6513,7 @@ EXPECT_GT(adapter->GetHpackEncoderDynamicTableSize(), 0); } -TEST_F(OgHttp2AdapterDataTest, ServerSubmitResponseWithResetFromClient) { +TEST_P(OgHttp2AdapterDataTest, ServerSubmitResponseWithResetFromClient) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -6563,7 +6569,7 @@ 1, ToHeaders({{":status", "404"}, {"x-comment", "I have no idea what you're talking about."}}), - std::move(body1), false); + GetParam() ? nullptr : std::move(body1), false); EXPECT_EQ(submit_result, 0); EXPECT_TRUE(adapter->want_write()); @@ -6649,9 +6655,12 @@ } using OgHttp2AdapterInteractionDataTest = OgHttp2AdapterDataTest; + +INSTANTIATE_TEST_SUITE_P(BothValues, OgHttp2AdapterInteractionDataTest, + testing::Bool()); // Exercises a naive mutually recursive test client and server. This test fails // without recursion guards in OgHttp2Session. -TEST_F(OgHttp2AdapterInteractionDataTest, ClientServerInteractionTest) { +TEST_P(OgHttp2AdapterInteractionDataTest, ClientServerInteractionTest) { TestVisitor client_visitor; OgHttp2Adapter::Options client_options; client_options.perspective = Perspective::kClient; @@ -6699,7 +6708,7 @@ {":authority", "example.com"}, {":path", absl::StrCat("/this/is/request/", new_stream_id)}}), - std::move(body), false, nullptr); + GetParam() ? nullptr : std::move(body), false, nullptr); EXPECT_EQ(new_stream_id, created_stream_id); client_adapter->Send(); } @@ -7359,7 +7368,7 @@ EXPECT_FALSE(adapter->want_write()); } -TEST_F(OgHttp2AdapterDataTest, SkipsSendingFramesForRejectedStream) { +TEST_P(OgHttp2AdapterDataTest, SkipsSendingFramesForRejectedStream) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -7396,8 +7405,9 @@ 1, "Here is some data, which will be completely ignored!"); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); auto source = std::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders( @@ -7542,7 +7552,7 @@ EXPECT_LT(next_result, 0); } -TEST_F(OgHttp2AdapterDataTest, ServerDoesNotSendFramesAfterImmediateGoAway) { +TEST_P(OgHttp2AdapterDataTest, ServerDoesNotSendFramesAfterImmediateGoAway) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -7580,8 +7590,9 @@ // Submit a response for the stream. visitor.AppendPayloadForStream(1, "This data is doomed to never be written."); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); // Submit a WINDOW_UPDATE frame. @@ -8413,7 +8424,7 @@ EXPECT_EQ(frames.size(), static_cast<size_t>(result)); } -TEST_F(OgHttp2AdapterDataTest, NegativeFlowControlStreamResumption) { +TEST_P(OgHttp2AdapterDataTest, NegativeFlowControlStreamResumption) { TestVisitor visitor; OgHttp2Adapter::Options options; options.perspective = Perspective::kServer; @@ -8457,8 +8468,9 @@ // Submit a response for the stream. visitor.AppendPayloadForStream(1, std::string(70000, 'a')); auto body = std::make_unique<VisitorDataSource>(visitor, 1); - int submit_result = adapter->SubmitResponse( - 1, ToHeaders({{":status", "200"}}), std::move(body), false); + int submit_result = + adapter->SubmitResponse(1, ToHeaders({{":status", "200"}}), + GetParam() ? nullptr : std::move(body), false); ASSERT_EQ(0, submit_result); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc index f47385c..cbc13d3 100644 --- a/quiche/http2/adapter/oghttp2_session.cc +++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -24,6 +24,7 @@ namespace { using ConnectionError = Http2VisitorInterface::ConnectionError; +using DataFrameHeaderInfo = Http2VisitorInterface::DataFrameHeaderInfo; using SpdyFramerError = Http2DecoderAdapter::SpdyFramerError; using ::spdy::SpdySettingsIR; @@ -683,11 +684,13 @@ QUICHE_LOG(ERROR) << "Unable to find stream " << stream_id; return -501; // NGHTTP2_ERR_INVALID_ARGUMENT } - QUICHE_DCHECK_EQ(end_stream, data_source == nullptr); - if (!end_stream) { + if (data_source != nullptr) { // Add data source to stream state iter->second.outbound_body = std::move(data_source); write_scheduler_.MarkStreamReady(stream_id, false); + } else if (!end_stream) { + iter->second.check_visitor_for_body = true; + write_scheduler_.MarkStreamReady(stream_id, false); } SendHeaders(stream_id, ToHeaderBlock(headers), end_stream); return 0; @@ -1023,8 +1026,6 @@ // Enqueue trailers immediately. SendTrailers(stream_id, ToHeaderBlock(trailers)); } else { - QUICHE_LOG_IF(ERROR, state.outbound_body->send_fin()) - << "DataFrameSource will send fin, preventing trailers!"; // Save trailers so they can be written once data is done. state.trailers = std::make_unique<spdy::Http2HeaderBlock>(ToHeaderBlock(trailers)); @@ -1825,10 +1826,12 @@ } auto iter = CreateStream(stream_id); - QUICHE_DCHECK_EQ(end_stream, data_source == nullptr); - if (!end_stream) { + if (data_source != nullptr) { iter->second.outbound_body = std::move(data_source); write_scheduler_.MarkStreamReady(stream_id, false); + } else if (!end_stream) { + iter->second.check_visitor_for_body = true; + write_scheduler_.MarkStreamReady(stream_id, false); } iter->second.user_data = user_data; for (const auto& [name, value] : headers) { @@ -2060,33 +2063,51 @@ } bool OgHttp2Session::HasMoreData(const StreamState& stream_state) const { - return stream_state.outbound_body != nullptr; + return stream_state.outbound_body != nullptr || + stream_state.check_visitor_for_body; } bool OgHttp2Session::IsReadyToWriteData(const StreamState& stream_state) const { - return stream_state.outbound_body != nullptr && !stream_state.data_deferred; + return HasMoreData(stream_state) && !stream_state.data_deferred; } void OgHttp2Session::AbandonData(StreamState& stream_state) { stream_state.outbound_body = nullptr; + stream_state.check_visitor_for_body = false; } OgHttp2Session::DataFrameInfo OgHttp2Session::GetDataFrameInfo( - Http2StreamId /*stream_id*/, size_t flow_control_available, + Http2StreamId stream_id, size_t flow_control_available, StreamState& stream_state) { - DataFrameInfo info; - std::tie(info.payload_length, info.end_data) = - stream_state.outbound_body->SelectPayloadLength(flow_control_available); - info.send_fin = - info.end_data ? stream_state.outbound_body->send_fin() : false; + DataFrameInfo info{.payload_length = 0, .end_data = true, .send_fin = true}; + if (stream_state.outbound_body != nullptr) { + std::tie(info.payload_length, info.end_data) = + stream_state.outbound_body->SelectPayloadLength(flow_control_available); + info.send_fin = + info.end_data ? stream_state.outbound_body->send_fin() : false; + } else if (stream_state.check_visitor_for_body) { + DataFrameHeaderInfo visitor_info = + visitor_.OnReadyToSendDataForStream(stream_id, flow_control_available); + info.payload_length = visitor_info.payload_length; + info.end_data = visitor_info.end_data || visitor_info.end_stream; + info.send_fin = visitor_info.end_stream; + } else { + QUICHE_LOG(DFATAL) << "GetDataFrameInfo for stream " << stream_id + << " but no body available!"; + } return info; } -bool OgHttp2Session::SendDataFrame(Http2StreamId /*stream_id*/, +bool OgHttp2Session::SendDataFrame(Http2StreamId stream_id, absl::string_view frame_header, size_t payload_length, StreamState& stream_state) { - return stream_state.outbound_body->Send(frame_header, payload_length); + if (stream_state.outbound_body != nullptr) { + return stream_state.outbound_body->Send(frame_header, payload_length); + } else { + QUICHE_DCHECK(stream_state.check_visitor_for_body); + return visitor_.SendDataFrame(stream_id, frame_header, payload_length); + } } } // namespace adapter
diff --git a/quiche/http2/adapter/oghttp2_session.h b/quiche/http2/adapter/oghttp2_session.h index 3f37019..8d6756f 100644 --- a/quiche/http2/adapter/oghttp2_session.h +++ b/quiche/http2/adapter/oghttp2_session.h
@@ -249,6 +249,7 @@ int32_t send_window; std::optional<HeaderType> received_header_type; std::optional<size_t> remaining_content_length; + bool check_visitor_for_body = false; bool half_closed_local = false; bool half_closed_remote = false; // Indicates that `outbound_body` temporarily cannot produce data.
diff --git a/quiche/http2/adapter/recording_http2_visitor.cc b/quiche/http2/adapter/recording_http2_visitor.cc index d55045f..0b989f0 100644 --- a/quiche/http2/adapter/recording_http2_visitor.cc +++ b/quiche/http2/adapter/recording_http2_visitor.cc
@@ -13,6 +13,22 @@ return serialized.size(); } +Http2VisitorInterface::DataFrameHeaderInfo +RecordingHttp2Visitor::OnReadyToSendDataForStream(Http2StreamId stream_id, + size_t max_length) { + events_.push_back(absl::StrFormat("OnReadyToSendDataForStream %d %d", + stream_id, max_length)); + return {70000, true, true}; +} + +bool RecordingHttp2Visitor::SendDataFrame(Http2StreamId stream_id, + absl::string_view /*frame_header*/, + size_t payload_bytes) { + events_.push_back( + absl::StrFormat("SendDataFrame %d %d", stream_id, payload_bytes)); + return true; +} + void RecordingHttp2Visitor::OnConnectionError(ConnectionError error) { events_.push_back( absl::StrFormat("OnConnectionError %s", ConnectionErrorToString(error)));
diff --git a/quiche/http2/adapter/recording_http2_visitor.h b/quiche/http2/adapter/recording_http2_visitor.h index d796fe7..b7835ee 100644 --- a/quiche/http2/adapter/recording_http2_visitor.h +++ b/quiche/http2/adapter/recording_http2_visitor.h
@@ -21,6 +21,10 @@ // From Http2VisitorInterface int64_t OnReadyToSend(absl::string_view serialized) override; + DataFrameHeaderInfo OnReadyToSendDataForStream(Http2StreamId stream_id, + size_t max_length) override; + bool SendDataFrame(Http2StreamId stream_id, absl::string_view frame_header, + size_t payload_bytes) override; void OnConnectionError(ConnectionError error) override; bool OnFrameHeader(Http2StreamId stream_id, size_t length, uint8_t type, uint8_t flags) override;
diff --git a/quiche/http2/adapter/test_utils.cc b/quiche/http2/adapter/test_utils.cc index a72bc1d..1e6765c 100644 --- a/quiche/http2/adapter/test_utils.cc +++ b/quiche/http2/adapter/test_utils.cc
@@ -94,7 +94,7 @@ payload.return_error = true; } -VisitorDataSource::VisitorDataSource(TestVisitor& visitor, +VisitorDataSource::VisitorDataSource(Http2VisitorInterface& visitor, Http2StreamId stream_id) : visitor_(visitor), stream_id_(stream_id) {}
diff --git a/quiche/http2/adapter/test_utils.h b/quiche/http2/adapter/test_utils.h index 047da91..5c7a687 100644 --- a/quiche/http2/adapter/test_utils.h +++ b/quiche/http2/adapter/test_utils.h
@@ -59,16 +59,10 @@ } } - struct DataFrameHeaderInfo { - int64_t payload_length; - bool end_data; - bool end_stream; - }; - // These methods will be moved to Http2VisitorInterface in a future CL. DataFrameHeaderInfo OnReadyToSendDataForStream(Http2StreamId stream_id, - size_t max_length); + size_t max_length) override; bool SendDataFrame(Http2StreamId stream_id, absl::string_view frame_header, - size_t payload_bytes); + size_t payload_bytes) override; // Test methods to manipulate the data frame payload to send for a stream. void AppendPayloadForStream(Http2StreamId stream_id, @@ -104,15 +98,14 @@ // A DataFrameSource that invokes visitor methods. class QUICHE_NO_EXPORT VisitorDataSource : public DataFrameSource { public: - // TODO(birenroy): revert visitor type to the interface type. - VisitorDataSource(TestVisitor& visitor, Http2StreamId stream_id); + VisitorDataSource(Http2VisitorInterface& visitor, Http2StreamId stream_id); std::pair<int64_t, bool> SelectPayloadLength(size_t max_length) override; bool Send(absl::string_view frame_header, size_t payload_length) override; bool send_fin() const override; private: - TestVisitor& visitor_; + Http2VisitorInterface& visitor_; const Http2StreamId stream_id_; // Whether the stream should end with the final frame of data. bool has_fin_ = false;