Remove return value of QuicConnection::MaybeBundleOpportunistically by flushing ACK frame. This is a refactor only change based on Fan's cl/557317758. Protected by FLAGS_quic_reloadable_flag_quic_flush_ack_in_maybe_bundle. PiperOrigin-RevId: 558150088
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index 857cf47..cee8ef1 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -3187,6 +3187,12 @@ visitor_->MaybeBundleOpportunistically(); } + if (packet_creator_.flush_ack_in_maybe_bundle() && + (packet_creator_.has_ack() || !CanWrite(NO_RETRANSMITTABLE_DATA))) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_flush_ack_in_maybe_bundle, 2, 3); + return {}; + } + QuicFrames frames; const bool has_pending_ack = uber_received_packet_manager_ @@ -3205,7 +3211,15 @@ << encryption_level_ << " ACK, " << (has_pending_ack ? "" : "!") << "has_pending_ack"; frames.push_back(updated_ack_frame); - // TODO(fayang): remove return value by FlushAckFrame here. + if (packet_creator_.flush_ack_in_maybe_bundle()) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_flush_ack_in_maybe_bundle, 3, 3); + const bool flushed = packet_creator_.FlushAckFrame(frames); + QUIC_BUG_IF(failed_to_flush_ack, !flushed) + << ENDPOINT << "Failed to flush ACK frame"; + return {}; + } + // TODO(wub): remove return value when deprecating + // quic_flush_ack_in_maybe_bundle. return frames; }
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h index 6744b55..b586bdc 100644 --- a/quiche/quic/core/quic_flags_list.h +++ b/quiche/quic/core/quic_flags_list.h
@@ -65,6 +65,8 @@ QUIC_FLAG(quic_reloadable_flag_quic_do_not_write_when_no_client_cid_available, true) // If true, enable server retransmittable on wire PING. QUIC_FLAG(quic_reloadable_flag_quic_enable_server_on_wire_ping, true) +// If true, flush ACK frame in QuicConnection::MaybeBundleOpportunistically. +QUIC_FLAG(quic_reloadable_flag_quic_flush_ack_in_maybe_bundle, false) // If true, include stream information in idle timeout connection close detail. QUIC_FLAG(quic_reloadable_flag_quic_add_stream_info_to_idle_close_detail, true) // If true, reject or send error response code upon receiving invalid request or response headers.
diff --git a/quiche/quic/core/quic_packet_creator.cc b/quiche/quic/core/quic_packet_creator.cc index f0fa133..37baedf 100644 --- a/quiche/quic/core/quic_packet_creator.cc +++ b/quiche/quic/core/quic_packet_creator.cc
@@ -1512,6 +1512,11 @@ } void QuicPacketCreator::MaybeBundleOpportunistically() { + if (flush_ack_in_maybe_bundle_) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_flush_ack_in_maybe_bundle, 1, 3); + delegate_->MaybeBundleOpportunistically(); + return; + } if (has_ack()) { // Ack already queued, nothing to do. return;
diff --git a/quiche/quic/core/quic_packet_creator.h b/quiche/quic/core/quic_packet_creator.h index 88f17d5..a4af3f6 100644 --- a/quiche/quic/core/quic_packet_creator.h +++ b/quiche/quic/core/quic_packet_creator.h
@@ -477,6 +477,8 @@ const QuicSocketAddress& peer_address() const { return packet_.peer_address; } + bool flush_ack_in_maybe_bundle() const { return flush_ack_in_maybe_bundle_; } + private: friend class test::QuicPacketCreatorPeer; @@ -673,6 +675,9 @@ // accept. There is no limit for QUIC_CRYPTO connections, but QUIC+TLS // negotiates this during the handshake. QuicByteCount max_datagram_frame_size_; + + const bool flush_ack_in_maybe_bundle_ = + GetQuicReloadableFlag(quic_flush_ack_in_maybe_bundle); }; } // namespace quic
diff --git a/quiche/quic/core/quic_packet_creator_test.cc b/quiche/quic/core/quic_packet_creator_test.cc index 30c8173..ebaf724 100644 --- a/quiche/quic/core/quic_packet_creator_test.cc +++ b/quiche/quic/core/quic_packet_creator_test.cc
@@ -2548,11 +2548,17 @@ bool ConsumeRetransmittableControlFrame(const QuicFrame& frame, bool bundle_ack) { - if (!has_ack()) { - QuicFrames frames; - if (bundle_ack) { - frames.push_back(QuicFrame(&ack_frame_)); - } + QuicFrames frames; + if (bundle_ack) { + frames.push_back(QuicFrame(&ack_frame_)); + } + if (GetQuicReloadableFlag(quic_flush_ack_in_maybe_bundle)) { + EXPECT_CALL(*delegate_, MaybeBundleOpportunistically()) + .WillOnce(Invoke([this, frames = std::move(frames)] { + FlushAckFrame(frames); + return QuicFrames(); + })); + } else if (!has_ack()) { if (delegate_->ShouldGeneratePacket(NO_RETRANSMITTABLE_DATA, NOT_HANDSHAKE)) { EXPECT_CALL(*delegate_, MaybeBundleOpportunistically()) @@ -2580,8 +2586,10 @@ if (!data.empty()) { producer_->SaveStreamData(id, data); } - if (!has_ack() && delegate_->ShouldGeneratePacket(NO_RETRANSMITTABLE_DATA, - NOT_HANDSHAKE)) { + if (GetQuicReloadableFlag(quic_flush_ack_in_maybe_bundle)) { + EXPECT_CALL(*delegate_, MaybeBundleOpportunistically()).Times(1); + } else if (!has_ack() && delegate_->ShouldGeneratePacket( + NO_RETRANSMITTABLE_DATA, NOT_HANDSHAKE)) { EXPECT_CALL(*delegate_, MaybeBundleOpportunistically()).Times(1); } return QuicPacketCreator::ConsumeData(id, data.length(), offset, state); @@ -2600,8 +2608,10 @@ size_t ConsumeCryptoData(EncryptionLevel level, absl::string_view data, QuicStreamOffset offset) { producer_->SaveCryptoData(level, offset, data); - if (!has_ack() && delegate_->ShouldGeneratePacket(NO_RETRANSMITTABLE_DATA, - NOT_HANDSHAKE)) { + if (GetQuicReloadableFlag(quic_flush_ack_in_maybe_bundle)) { + EXPECT_CALL(*delegate_, MaybeBundleOpportunistically()).Times(1); + } else if (!has_ack() && delegate_->ShouldGeneratePacket( + NO_RETRANSMITTABLE_DATA, NOT_HANDSHAKE)) { EXPECT_CALL(*delegate_, MaybeBundleOpportunistically()).Times(1); } return QuicPacketCreator::ConsumeCryptoData(level, data.length(), offset);