Fix a flow control issue introduced in cl/557313748. See b/308247268#comment3 for details of the issue. Most of this change is updating the original feature flag to the new v2 flag. The actual fix is in `QuicPacketCreator::ConsumeData`, after `delegate_->MaybeBundleOpportunistically()` is called. Protected by FLAGS_quic_restart_flag_quic_opport_bundle_qpack_decoder_data2 (Replaces quic_restart_flag_quic_opport_bundle_qpack_decoder_data). PiperOrigin-RevId: 582131467
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc index 06200ff..7eafa6c 100644 --- a/quiche/quic/core/http/end_to_end_test.cc +++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -2845,7 +2845,7 @@ client_connection->GetStats().packets_sent; if (version_.UsesHttp3()) { - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // QPACK decoder instructions and RESET_STREAM and STOP_SENDING frames are // sent in a single packet. EXPECT_EQ(packets_sent_before + 1, packets_sent_now); @@ -3028,7 +3028,7 @@ return; } override_client_connection_id_length_ = kQuicDefaultConnectionIdLength; - SetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data, false); + SetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2, false); ASSERT_TRUE(Initialize()); SendSynchronousFooRequestAndCheckResponse();
diff --git a/quiche/quic/core/http/quic_spdy_session.cc b/quiche/quic/core/http/quic_spdy_session.cc index b78a6cc..4319cad 100644 --- a/quiche/quic/core/http/quic_spdy_session.cc +++ b/quiche/quic/core/http/quic_spdy_session.cc
@@ -847,7 +847,7 @@ } bool QuicSpdySession::CheckStreamWriteBlocked(QuicStream* stream) const { - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data) && + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2) && qpack_decoder_send_stream_ != nullptr && stream->id() == qpack_decoder_send_stream_->id()) { // Decoder data is always bundled opportunistically.
diff --git a/quiche/quic/core/http/quic_spdy_session_test.cc b/quiche/quic/core/http/quic_spdy_session_test.cc index a8c8c0a..3147048 100644 --- a/quiche/quic/core/http/quic_spdy_session_test.cc +++ b/quiche/quic/core/http/quic_spdy_session_test.cc
@@ -1319,7 +1319,7 @@ // In HTTP/3, Qpack stream will send data on stream reset and cause packet to // be flushed. if (VersionUsesHttp3(transport_version()) && - !GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + !GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _, _)) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0))); } @@ -1556,7 +1556,7 @@ // the STOP_SENDING, so set up the EXPECT there. EXPECT_CALL(*connection_, OnStreamReset(stream->id(), _)); EXPECT_CALL(*connection_, SendControlFrame(_)); - } else if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + } else if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _, _)) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0))); }
diff --git a/quiche/quic/core/http/quic_spdy_stream_test.cc b/quiche/quic/core/http/quic_spdy_stream_test.cc index 3073703..bf2b2c3 100644 --- a/quiche/quic/core/http/quic_spdy_stream_test.cc +++ b/quiche/quic/core/http/quic_spdy_stream_test.cc
@@ -583,7 +583,7 @@ stream_->id(), QuicResetStreamError::FromInternal(QUIC_HEADERS_TOO_LARGE), 0)); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { auto qpack_decoder_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); // Stream type and stream cancellation. @@ -2249,7 +2249,7 @@ std::string headers = HeadersFrame(encoded_headers); EXPECT_CALL(debug_visitor, OnHeadersFrameReceived(stream_->id(), encoded_headers.length())); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // Decoder stream type. EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), /* write_length = */ 1, @@ -2286,7 +2286,7 @@ EXPECT_CALL(debug_visitor, OnHeadersFrameReceived(stream_->id(), encoded_trailers.length())); // Header acknowledgement. - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), _, _, _, _, _)); } @@ -2328,7 +2328,7 @@ auto decoder_send_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // Decoder stream type. EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), /* write_length = */ 1, @@ -2367,7 +2367,7 @@ // Decoding is blocked because dynamic table entry has not been received yet. EXPECT_FALSE(stream_->trailers_decompressed()); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // Header acknowledgement. EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), _, _, _, _, _)); @@ -2462,7 +2462,7 @@ auto decoder_send_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // Decoder stream type. EXPECT_CALL(*session_, WritevData(decoder_send_stream->id(), /* write_length = */ 1, @@ -2533,7 +2533,7 @@ // Decoding is blocked because dynamic table entry has not been received yet. EXPECT_FALSE(stream_->headers_decompressed()); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // Decoder stream type and stream cancellation instruction. auto decoder_send_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); @@ -2583,7 +2583,7 @@ // Decoding is blocked because dynamic table entry has not been received yet. EXPECT_FALSE(stream_->headers_decompressed()); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // Decoder stream type and stream cancellation instruction. auto decoder_send_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); @@ -2976,7 +2976,7 @@ auto qpack_decoder_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // Stream type. EXPECT_CALL(*session_, WritevData(qpack_decoder_stream->id(), /* write_length = */ 1, @@ -3009,7 +3009,7 @@ auto qpack_decoder_stream = QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get()); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // Stream type. EXPECT_CALL(*session_, WritevData(qpack_decoder_stream->id(), /* write_length = */ 1, @@ -3399,7 +3399,7 @@ QuicStreamFrame frame(stream_->id(), /* fin = */ false, 0, data_frame); stream_->OnStreamFrame(frame); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { // As a result of resetting the stream, stream type and stream cancellation // are sent on the QPACK decoder stream. auto qpack_decoder_stream =
diff --git a/quiche/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc b/quiche/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc index 86a8564..b5be87d 100644 --- a/quiche/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc +++ b/quiche/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
@@ -175,7 +175,7 @@ EXPECT_CALL(visitor_, OnHeadersDecoded(_, true)); qpack_decoder_.OnInsertWithoutNameReference("foo", "bar"); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { qpack_decoder_.FlushDecoderStream(); } } @@ -201,7 +201,7 @@ EXPECT_EQ(strlen("foo") + strlen("bar"), header_list.uncompressed_header_bytes()); EXPECT_EQ(encoded_data.size(), header_list.compressed_header_bytes()); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { qpack_decoder_.FlushDecoderStream(); } } @@ -227,7 +227,7 @@ accumulator_.EndHeaderBlock(); EXPECT_THAT(header_list, ElementsAre(Pair("foo", "bar"), Pair("foo", "bar"))); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { qpack_decoder_.FlushDecoderStream(); } }
diff --git a/quiche/quic/core/qpack/qpack_decoder.cc b/quiche/quic/core/qpack/qpack_decoder.cc index 7a798ee..bad9ca3 100644 --- a/quiche/quic/core/qpack/qpack_decoder.cc +++ b/quiche/quic/core/qpack/qpack_decoder.cc
@@ -31,7 +31,7 @@ void QpackDecoder::OnStreamReset(QuicStreamId stream_id) { if (header_table_.maximum_dynamic_table_capacity() > 0) { decoder_stream_sender_.SendStreamCancellation(stream_id); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { decoder_stream_sender_.Flush(); } } @@ -68,7 +68,7 @@ known_received_count_ = header_table_.inserted_entry_count(); } - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { decoder_stream_sender_.Flush(); } }
diff --git a/quiche/quic/core/qpack/qpack_decoder_stream_sender.cc b/quiche/quic/core/qpack/qpack_decoder_stream_sender.cc index 879f21b..a0b1ec4 100644 --- a/quiche/quic/core/qpack/qpack_decoder_stream_sender.cc +++ b/quiche/quic/core/qpack/qpack_decoder_stream_sender.cc
@@ -38,8 +38,8 @@ if (buffer_.empty() || delegate_ == nullptr) { return; } - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { - QUIC_RESTART_FLAG_COUNT_N(quic_opport_bundle_qpack_decoder_data, 3, 3); + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { + QUIC_RESTART_FLAG_COUNT_N(quic_opport_bundle_qpack_decoder_data2, 3, 4); // Swap buffer_ before calling WriteStreamData, which might result in a // reentrant call to `Flush()`. std::string copy;
diff --git a/quiche/quic/core/qpack/qpack_decoder_test.cc b/quiche/quic/core/qpack/qpack_decoder_test.cc index 4415871..451f388 100644 --- a/quiche/quic/core/qpack/qpack_decoder_test.cc +++ b/quiche/quic/core/qpack/qpack_decoder_test.cc
@@ -369,13 +369,13 @@ .InSequence(s); EXPECT_CALL(handler_, OnHeaderDecoded(Eq("foo"), Eq("ZZZ"))).InSequence(s); EXPECT_CALL(handler_, OnHeaderDecoded(Eq(":method"), Eq("ZZ"))).InSequence(s); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))) .InSequence(s); } EXPECT_CALL(handler_, OnDecodingCompleted()).InSequence(s); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))) .InSequence(s); @@ -390,7 +390,7 @@ "80" // Dynamic table entry with relative index 0, absolute index 3. "41025a5a")); // Name of entry 1 (relative index) from dynamic table, // with value "ZZ". - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } @@ -400,13 +400,13 @@ .InSequence(s); EXPECT_CALL(handler_, OnHeaderDecoded(Eq("foo"), Eq("ZZZ"))).InSequence(s); EXPECT_CALL(handler_, OnHeaderDecoded(Eq(":method"), Eq("ZZ"))).InSequence(s); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))) .InSequence(s); } EXPECT_CALL(handler_, OnDecodingCompleted()).InSequence(s); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))) .InSequence(s); @@ -421,7 +421,7 @@ "82" // Dynamic table entry with relative index 2, absolute index 3. "43025a5a")); // Name of entry 3 (relative index) from dynamic table, // with value "ZZ". - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } @@ -431,13 +431,13 @@ .InSequence(s); EXPECT_CALL(handler_, OnHeaderDecoded(Eq("foo"), Eq("ZZZ"))).InSequence(s); EXPECT_CALL(handler_, OnHeaderDecoded(Eq(":method"), Eq("ZZ"))).InSequence(s); - if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (!GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))) .InSequence(s); } EXPECT_CALL(handler_, OnDecodingCompleted()).InSequence(s); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))) .InSequence(s); @@ -452,7 +452,7 @@ "12" // Dynamic table entry with post-base index 2, absolute index 3. "01025a5a")); // Name of entry 1 (post-base index) from dynamic table, // with value "ZZ". - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } } @@ -485,7 +485,7 @@ "0200" // Required Insert Count 1 and Delta Base 0. // Base is 1 + 0 = 1. "80")); // Dynamic table entry with relative index 0, absolute index 0. - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } } @@ -718,7 +718,7 @@ "0a00" // Encoded Required Insert Count 10, Required Insert Count 201, // Delta Base 0, Base 201. "80")); // Emit dynamic table entry with relative index 0. - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } } @@ -880,7 +880,7 @@ DecodeEncoderStreamData(absl::HexStringToBytes("3fe107")); // Add literal entry with name "foo" and value "bar". DecodeEncoderStreamData(absl::HexStringToBytes("6294e703626172")); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } } @@ -920,7 +920,7 @@ EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))); EndDecoding(); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } } @@ -976,7 +976,7 @@ // Add literal entry with name "foo" and value "bar". // Insert Count is now 6, reaching Required Insert Count of the header block. DecodeEncoderStreamData(absl::HexStringToBytes("6294e70362617a")); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } } @@ -1019,7 +1019,7 @@ "0200" // Required Insert Count 1 and Delta Base 0. // Base is 1 + 0 = 1. "80")); // Dynamic table entry with relative index 0, absolute index 0. - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { FlushDecoderStream(); } }
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index 48e5085..1e18807 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -3167,8 +3167,8 @@ } } - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { - QUIC_RESTART_FLAG_COUNT_N(quic_opport_bundle_qpack_decoder_data, 1, 3); + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { + QUIC_RESTART_FLAG_COUNT_N(quic_opport_bundle_qpack_decoder_data2, 1, 4); visitor_->MaybeBundleOpportunistically(); } @@ -5795,8 +5795,8 @@ uber_received_packet_manager_.GetEarliestAckTimeout(); QUIC_BUG_IF(quic_bug_12714_32, !earliest_ack_timeout.IsInitialized()); MaybeBundleCryptoDataWithAcks(); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { - QUIC_RESTART_FLAG_COUNT_N(quic_opport_bundle_qpack_decoder_data, 2, 3); + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { + QUIC_RESTART_FLAG_COUNT_N(quic_opport_bundle_qpack_decoder_data2, 2, 4); visitor_->MaybeBundleOpportunistically(); } earliest_ack_timeout = uber_received_packet_manager_.GetEarliestAckTimeout();
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index 713193b..621f86c 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -263,6 +263,9 @@ // Asks session to bundle data opportunistically with outgoing data. virtual void MaybeBundleOpportunistically() = 0; + + // Get from session the flow control send window for stream |id|. + virtual QuicByteCount GetFlowControlSendWindowSize(QuicStreamId id) = 0; }; // Interface which gets callbacks from the QuicConnection at interesting @@ -728,6 +731,9 @@ bool ShouldGeneratePacket(HasRetransmittableData retransmittable, IsHandshake handshake) override; void MaybeBundleOpportunistically() override; + QuicByteCount GetFlowControlSendWindowSize(QuicStreamId id) override { + return visitor_->GetFlowControlSendWindowSize(id); + } QuicPacketBuffer GetPacketBuffer() override; void OnSerializedPacket(SerializedPacket packet) override; void OnUnrecoverableError(QuicErrorCode error,
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index ecfb82b..984a287 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -700,8 +700,9 @@ EXPECT_CALL(visitor_, OnCongestionWindowChange(_)).Times(AnyNumber()); EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(AnyNumber()); EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)).Times(AnyNumber()); - if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data)) { + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { EXPECT_CALL(visitor_, MaybeBundleOpportunistically()).Times(AnyNumber()); + EXPECT_CALL(visitor_, GetFlowControlSendWindowSize(_)).Times(AnyNumber()); } EXPECT_CALL(visitor_, OnOneRttPacketAcknowledged()) .Times(testing::AtMost(1));
diff --git a/quiche/quic/core/quic_dispatcher.cc b/quiche/quic/core/quic_dispatcher.cc index 3984426..3725433 100644 --- a/quiche/quic/core/quic_dispatcher.cc +++ b/quiche/quic/core/quic_dispatcher.cc
@@ -143,6 +143,11 @@ void MaybeBundleOpportunistically() override { QUICHE_DCHECK(false); } + QuicByteCount GetFlowControlSendWindowSize(QuicStreamId /*id*/) override { + QUICHE_DCHECK(false); + return std::numeric_limits<QuicByteCount>::max(); + } + SerializedPacketFate GetSerializedPacketFate( bool /*is_mtu_discovery*/, EncryptionLevel /*encryption_level*/) override {
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h index 6e33e2e..6e5ed09 100644 --- a/quiche/quic/core/quic_flags_list.h +++ b/quiche/quic/core/quic_flags_list.h
@@ -46,7 +46,7 @@ // If true, allow client to enable BBRv2 on server via connection option \'B2ON\'. QUIC_FLAG(quic_reloadable_flag_quic_allow_client_enabled_bbr_v2, true) // If true, always bundle qpack decoder data with other frames opportunistically. -QUIC_FLAG(quic_restart_flag_quic_opport_bundle_qpack_decoder_data, false) +QUIC_FLAG(quic_restart_flag_quic_opport_bundle_qpack_decoder_data2, false) // If true, an endpoint does not detect path degrading or blackholing until handshake gets confirmed. QUIC_FLAG(quic_reloadable_flag_quic_no_path_degrading_before_handshake_confirmed, true) // If true, default-enable 5RTO blachole detection.
diff --git a/quiche/quic/core/quic_packet_creator.cc b/quiche/quic/core/quic_packet_creator.cc index eacc0c2..793f417 100644 --- a/quiche/quic/core/quic_packet_creator.cc +++ b/quiche/quic/core/quic_packet_creator.cc
@@ -1313,6 +1313,26 @@ "generator tries to write stream data."; bool has_handshake = QuicUtils::IsCryptoStreamId(transport_version(), id); delegate_->MaybeBundleOpportunistically(); + // If the data being consumed is subject to flow control, check the flow + // control send window to see if |write_length| exceeds the send window after + // bundling opportunistic data, if so, reduce |write_length| to the send + // window size. + // The data being consumed is subject to flow control iff + // - It is not a retransmission. We check next_transmission_type_ for that. + // - And it's not handshake data. This is always true for ConsumeData because + // the function is not called for handshake data. + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2) && + next_transmission_type_ == NOT_RETRANSMISSION) { + if (QuicByteCount send_window = delegate_->GetFlowControlSendWindowSize(id); + write_length > send_window) { + QUIC_RESTART_FLAG_COUNT_N(quic_opport_bundle_qpack_decoder_data2, 4, 4); + QUIC_DLOG(INFO) << ENDPOINT + << "After bundled data, reducing (old) write_length:" + << write_length << "to (new) send_window:" << send_window; + write_length = send_window; + state = NO_FIN; + } + } bool fin = state != NO_FIN; QUIC_BUG_IF(quic_bug_12398_17, has_handshake && fin) << ENDPOINT << "Handshake packets should never send a fin";
diff --git a/quiche/quic/core/quic_packet_creator.h b/quiche/quic/core/quic_packet_creator.h index 4cfd566..bb2303e 100644 --- a/quiche/quic/core/quic_packet_creator.h +++ b/quiche/quic/core/quic_packet_creator.h
@@ -64,6 +64,14 @@ // anything with to-be-sent data. virtual void MaybeBundleOpportunistically() = 0; + // When sending flow controlled data, this will be called after + // MaybeBundleOpportunistically(). If the returned flow control send window + // is smaller than data's write_length, write_length will be adjusted + // acccordingly. + // If the delegate has no notion of flow control, it should return + // std::numeric_limit<QuicByteCount>::max(). + virtual QuicByteCount GetFlowControlSendWindowSize(QuicStreamId id) = 0; + // Returns the packet fate for serialized packets which will be handed over // to delegate via OnSerializedPacket(). Called when a packet is about to be // serialized.
diff --git a/quiche/quic/core/quic_packet_creator_test.cc b/quiche/quic/core/quic_packet_creator_test.cc index 844011a..e1510f5 100644 --- a/quiche/quic/core/quic_packet_creator_test.cc +++ b/quiche/quic/core/quic_packet_creator_test.cc
@@ -2477,6 +2477,8 @@ (HasRetransmittableData retransmittable, IsHandshake handshake), (override)); MOCK_METHOD(void, MaybeBundleOpportunistically, (), (override)); + MOCK_METHOD(QuicByteCount, GetFlowControlSendWindowSize, (QuicStreamId), + (override)); MOCK_METHOD(QuicPacketBuffer, GetPacketBuffer, (), (override)); MOCK_METHOD(void, OnSerializedPacket, (SerializedPacket), (override)); MOCK_METHOD(void, OnUnrecoverableError, (QuicErrorCode, const std::string&), @@ -2616,6 +2618,8 @@ .WillRepeatedly(Return(QuicPacketBuffer())); EXPECT_CALL(delegate_, GetSerializedPacketFate(_, _)) .WillRepeatedly(Return(SEND_TO_WRITER)); + EXPECT_CALL(delegate_, GetFlowControlSendWindowSize(_)) + .WillRepeatedly(Return(std::numeric_limits<QuicByteCount>::max())); creator_.SetEncrypter( ENCRYPTION_FORWARD_SECURE, std::make_unique<TaggingEncrypter>(ENCRYPTION_FORWARD_SECURE)); @@ -2850,6 +2854,61 @@ EXPECT_FALSE(creator_.HasPendingRetransmittableFrames()); } +// Tests the case that after bundling data, send window reduced to be shorter +// than data. +TEST_F(QuicPacketCreatorMultiplePacketsTest, + ConsumeDataAdjustWriteLengthAfterBundledData) { + creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE); + creator_.SetTransmissionType(NOT_RETRANSMISSION); + delegate_.SetCanWriteAnything(); + + const std::string data(1000, 'D'); + QuicStreamId stream_id = QuicUtils::GetFirstBidirectionalStreamId( + framer_.transport_version(), Perspective::IS_CLIENT); + + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { + EXPECT_CALL(delegate_, GetFlowControlSendWindowSize(stream_id)) + .WillOnce(Return(data.length() - 1)); + } else { + EXPECT_CALL(delegate_, GetFlowControlSendWindowSize(_)).Times(0); + } + + QuicConsumedData consumed = creator_.ConsumeData(stream_id, data, 0u, FIN); + + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { + EXPECT_EQ(consumed.bytes_consumed, data.length() - 1); + EXPECT_FALSE(consumed.fin_consumed); + } else { + EXPECT_EQ(consumed.bytes_consumed, data.length()); + EXPECT_TRUE(consumed.fin_consumed); + } +} + +// Tests the case that after bundling data, send window is exactly as big as +// data length. +TEST_F(QuicPacketCreatorMultiplePacketsTest, + ConsumeDataDoesNotAdjustWriteLengthAfterBundledData) { + creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE); + creator_.SetTransmissionType(NOT_RETRANSMISSION); + delegate_.SetCanWriteAnything(); + + const std::string data(1000, 'D'); + QuicStreamId stream_id = QuicUtils::GetFirstBidirectionalStreamId( + framer_.transport_version(), Perspective::IS_CLIENT); + + if (GetQuicRestartFlag(quic_opport_bundle_qpack_decoder_data2)) { + EXPECT_CALL(delegate_, GetFlowControlSendWindowSize(stream_id)) + .WillOnce(Return(data.length())); + } else { + EXPECT_CALL(delegate_, GetFlowControlSendWindowSize(_)).Times(0); + } + + QuicConsumedData consumed = creator_.ConsumeData(stream_id, data, 0u, FIN); + + EXPECT_EQ(consumed.bytes_consumed, data.length()); + EXPECT_TRUE(consumed.fin_consumed); +} + TEST_F(QuicPacketCreatorMultiplePacketsTest, ConsumeData_NotWritable) { delegate_.SetCanNotWrite();
diff --git a/quiche/quic/core/quic_session.cc b/quiche/quic/core/quic_session.cc index ddb0914..d4de0f2 100644 --- a/quiche/quic/core/quic_session.cc +++ b/quiche/quic/core/quic_session.cc
@@ -2428,6 +2428,16 @@ return GetCryptoStream()->GetHandshakeState(); } +QuicByteCount QuicSession::GetFlowControlSendWindowSize(QuicStreamId id) { + QuicStream* stream = GetActiveStream(id); + if (stream == nullptr) { + // No flow control for invalid or inactive stream ids. Returning uint64max + // allows QuicPacketCreator to write as much data as possible. + return std::numeric_limits<QuicByteCount>::max(); + } + return stream->CalculateSendWindowSize(); +} + WriteStreamDataResult QuicSession::WriteStreamData(QuicStreamId id, QuicStreamOffset offset, QuicByteCount data_length,
diff --git a/quiche/quic/core/quic_session.h b/quiche/quic/core/quic_session.h index 1a20486..dd757b7 100644 --- a/quiche/quic/core/quic_session.h +++ b/quiche/quic/core/quic_session.h
@@ -189,6 +189,7 @@ void OnServerPreferredAddressAvailable( const QuicSocketAddress& /*server_preferred_address*/) override; void MaybeBundleOpportunistically() override {} + QuicByteCount GetFlowControlSendWindowSize(QuicStreamId id) override; // QuicStreamFrameDataProducer WriteStreamDataResult WriteStreamData(QuicStreamId id,
diff --git a/quiche/quic/core/quic_stream.h b/quiche/quic/core/quic_stream.h index f5c7d8a..3633c3b 100644 --- a/quiche/quic/core/quic_stream.h +++ b/quiche/quic/core/quic_stream.h
@@ -390,6 +390,10 @@ stream_contributes_to_connection_flow_control_ = false; } + // Returns the min of stream level flow control window size and connection + // level flow control window size. + QuicByteCount CalculateSendWindowSize() const; + protected: // Called when data of [offset, offset + data_length] is buffered in send // buffer. @@ -474,7 +478,7 @@ // RFC 9000. virtual void OnWriteSideInDataRecvdState() {} - // Return the current flow control send window in bytes. + // Return the current stream-level flow control send window in bytes. std::optional<QuicByteCount> GetSendWindow() const; std::optional<QuicByteCount> GetReceiveWindow() const; @@ -497,10 +501,6 @@ // Write buffered data (in send buffer) at |level|. void WriteBufferedData(EncryptionLevel level); - // Returns the min of stream level flow control window size and connection - // level flow control window size. - QuicByteCount CalculateSendWindowSize() const; - // Called when bytes are sent to the peer. void AddBytesSent(QuicByteCount bytes);
diff --git a/quiche/quic/test_tools/quic_test_utils.cc b/quiche/quic/test_tools/quic_test_utils.cc index c5ae06f..6298cda 100644 --- a/quiche/quic/test_tools/quic_test_utils.cc +++ b/quiche/quic/test_tools/quic_test_utils.cc
@@ -42,6 +42,7 @@ using testing::_; using testing::Invoke; +using testing::Return; namespace quic { namespace test { @@ -449,7 +450,10 @@ return false; } -MockQuicConnectionVisitor::MockQuicConnectionVisitor() {} +MockQuicConnectionVisitor::MockQuicConnectionVisitor() { + ON_CALL(*this, GetFlowControlSendWindowSize(_)) + .WillByDefault(Return(std::numeric_limits<QuicByteCount>::max())); +} MockQuicConnectionVisitor::~MockQuicConnectionVisitor() {}
diff --git a/quiche/quic/test_tools/quic_test_utils.h b/quiche/quic/test_tools/quic_test_utils.h index 55b867f..b5df446 100644 --- a/quiche/quic/test_tools/quic_test_utils.h +++ b/quiche/quic/test_tools/quic_test_utils.h
@@ -509,6 +509,8 @@ MOCK_METHOD(void, OnServerPreferredAddressAvailable, (const QuicSocketAddress&), (override)); MOCK_METHOD(void, MaybeBundleOpportunistically, (), (override)); + MOCK_METHOD(QuicByteCount, GetFlowControlSendWindowSize, (QuicStreamId), + (override)); }; class MockQuicConnectionHelper : public QuicConnectionHelperInterface { @@ -1406,6 +1408,8 @@ (HasRetransmittableData retransmittable, IsHandshake handshake), (override)); MOCK_METHOD(void, MaybeBundleOpportunistically, (), (override)); + MOCK_METHOD(QuicByteCount, GetFlowControlSendWindowSize, (QuicStreamId), + (override)); MOCK_METHOD(SerializedPacketFate, GetSerializedPacketFate, (bool, EncryptionLevel), (override)); };
diff --git a/quiche/quic/test_tools/simulator/quic_endpoint.h b/quiche/quic/test_tools/simulator/quic_endpoint.h index a7f3638..11d3ee5 100644 --- a/quiche/quic/test_tools/simulator/quic_endpoint.h +++ b/quiche/quic/test_tools/simulator/quic_endpoint.h
@@ -112,6 +112,9 @@ void OnServerPreferredAddressAvailable( const QuicSocketAddress& /*server_preferred_address*/) override {} void MaybeBundleOpportunistically() override {} + QuicByteCount GetFlowControlSendWindowSize(QuicStreamId /*id*/) override { + return std::numeric_limits<QuicByteCount>::max(); + } // End QuicConnectionVisitorInterface implementation.