Send pending ACKs after visitor_->OnCanWrite() in QuicConnection::OnCanWrite(). Previously, `QuicConnection::OnCanWrite()` called `SendAllPendingAcks()` before `visitor_->OnCanWrite()`. When a pending ACK's encryption level differed from the connection's current default encryption level (such as during a handshake or 0-RTT connection), `SendAllPendingAcks()` switched encryption levels via `ScopedEncryptionLevelContext`, immediately flushing a standalone ACK packet before `visitor_->OnCanWrite()` could write `CRYPTO` or `STREAM` frames at that encryption level. When `send_acks_after_on_can_write_` is enabled (via `gfe2_reloadable_flag_quic_send_acks_after_on_can_write` and the `SAOW` connection option), `QuicConnection::OnCanWrite()` invokes `visitor_->OnCanWrite()` before `SendAllPendingAcks()` so that outgoing `CRYPTO` and `STREAM` packets opportunistically bundle pending ACKs at the same encryption level, and only remaining unbundled ACKs are sent afterward. Overview of impact including Handshake Simulations: https://docs.google.com/document/d/1CiaBRfPAfKbL0SuEhrBQeZxDeXvdREseGe1AE89pR-I/edit?usp=sharing Protected by quic_reloadable_flag_quic_send_acks_after_on_can_write. PiperOrigin-RevId: 994903307
diff --git a/quiche/common/quiche_feature_flags_list.h b/quiche/common/quiche_feature_flags_list.h index 7f950d8..dab5eb6 100755 --- a/quiche/common/quiche_feature_flags_list.h +++ b/quiche/common/quiche_feature_flags_list.h
@@ -62,6 +62,7 @@ QUICHE_FLAG(bool, quiche_reloadable_flag_quic_reject_empty_cid_in_ncid, false, true, "If true, QUIC framer will reject NEW_CONNECTION_ID frames with empty connection IDs.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_reject_unusable_ech_config, false, true, "When true, calls SSL_set_reject_unusable_ech_config in TlsClientHandshaker.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_require_handshake_confirmation, true, true, "If true, require handshake confirmation for QUIC connections, functionally disabling 0-rtt handshakes.") +QUICHE_FLAG(bool, quiche_reloadable_flag_quic_send_acks_after_on_can_write, false, false, "When true, QuicConnection::OnCanWrite() calls visitor_->OnCanWrite() before SendAllPendingAcks() so outgoing data frames can opportunistically bundle pending ACKs first.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_test_peer_addr_change_after_normalize, false, false, "If true, QuicConnection::ProcessValidatedPacket will use normalized address to test peer address changes.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_testonly_default_false, false, false, "A testonly reloadable flag that will always default to false.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_testonly_default_true, true, true, "A testonly reloadable flag that will always default to true.")
diff --git a/quiche/quic/core/crypto/crypto_protocol.h b/quiche/quic/core/crypto/crypto_protocol.h index c56eefc..c504c5d 100644 --- a/quiche/quic/core/crypto/crypto_protocol.h +++ b/quiche/quic/core/crypto/crypto_protocol.h
@@ -515,6 +515,7 @@ DEFINE_STATIC_QUIC_TAG(CHP1); // 1-packet padding to CHLO. DEFINE_STATIC_QUIC_TAG(CHP2); // 2-packet padding to CHLO. DEFINE_STATIC_QUIC_TAG(BCFR); // Bundle CRYPTO frames when retransmitting. +DEFINE_STATIC_QUIC_TAG(SAOW); // Send ACKs after OnCanWrite. // Stats collection tags DEFINE_STATIC_QUIC_TAG(EPID); // Endpoint identifier.
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc index c29d91e..e805d37 100644 --- a/quiche/quic/core/http/end_to_end_test.cc +++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -9329,6 +9329,15 @@ server_thread_->Resume(); } +TEST_P(EndToEndTest, SendAcksAfterOnCanWrite) { + SetQuicReloadableFlag(quic_send_acks_after_on_can_write, true); + client_extra_copts_.push_back(kSAOW); + SetPacketLossPercentage(30); + ASSERT_TRUE(Initialize()); + EXPECT_TRUE(client_->client()->WaitForHandshakeConfirmed()); + SendSynchronousFooRequestAndCheckResponse(); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index c39b098..8018c7d 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -603,6 +603,12 @@ if (config.HasClientSentConnectionOption(kDFER, perspective_)) { defer_send_in_response_to_packets_ = false; } + if (GetQuicReloadableFlag(quic_send_acks_after_on_can_write) && + version().IsIetfQuic() && + config.HasClientSentConnectionOption(kSAOW, perspective_)) { + send_acks_after_on_can_write_ = true; + } + if (perspective_ == Perspective::IS_CLIENT && config.HasClientSentConnectionOption(kCDFR, perspective_)) { defer_send_in_response_to_packets_ = true; @@ -2988,23 +2994,50 @@ ScopedPacketFlusher flusher(this); WriteQueuedPackets(); - const QuicTime ack_timeout = - uber_received_packet_manager_.GetEarliestAckTimeout(); - if (ack_timeout.IsInitialized() && ack_timeout <= clock_->ApproximateNow()) { - // Send an ACK now because either 1) we were write blocked when we last - // tried to send an ACK, or 2) both ack alarm and send alarm were set to - // go off together. - SendAllPendingAcks(); - } + if (send_acks_after_on_can_write_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_send_acks_after_on_can_write); + // Sending queued packets may have caused the socket to become write + // blocked, or the congestion manager to prohibit sending. + const bool can_write_retransmittable = CanWrite(HAS_RETRANSMITTABLE_DATA); + if (can_write_retransmittable) { + // Tell the session it can write. + visitor_->OnCanWrite(); + } + if (!connected_) { + return; + } + const QuicTime ack_timeout = + uber_received_packet_manager_.GetEarliestAckTimeout(); + if (ack_timeout.IsInitialized() && + ack_timeout <= clock_->ApproximateNow()) { + // Send an ACK now because either 1) we were write blocked when we last + // tried to send an ACK, or 2) both ack alarm and send alarm were set to + // go off together. + SendAllPendingAcks(); + } + if (!connected_ || !can_write_retransmittable) { + return; + } + } else { + const QuicTime ack_timeout = + uber_received_packet_manager_.GetEarliestAckTimeout(); + if (ack_timeout.IsInitialized() && + ack_timeout <= clock_->ApproximateNow()) { + // Send an ACK now because either 1) we were write blocked when we last + // tried to send an ACK, or 2) both ack alarm and send alarm were set to + // go off together. + SendAllPendingAcks(); + } - // Sending queued packets may have caused the socket to become write blocked, - // or the congestion manager to prohibit sending. - if (!CanWrite(HAS_RETRANSMITTABLE_DATA)) { - return; - } + // Sending queued packets may have caused the socket to become write + // blocked, or the congestion manager to prohibit sending. + if (!CanWrite(HAS_RETRANSMITTABLE_DATA)) { + return; + } - // Tell the session it can write. - visitor_->OnCanWrite(); + // Tell the session it can write. + visitor_->OnCanWrite(); + } // After the visitor writes, it may have caused the socket to become write // blocked or the congestion manager to prohibit sending, so check again.
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index 442df61..1bbb824 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -2583,6 +2583,7 @@ // True if spin bit is enabled for this connection. bool spin_bit_enabled_ : 1 = false; bool fix_mtu_discovery_ : 1 = GetQuicReloadableFlag(quic_fix_mtu_discovery); + bool send_acks_after_on_can_write_ : 1 = false; }; } // namespace quic
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index 72258ed..441e8ad 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -19143,6 +19143,83 @@ kMaxNumOfActiveConnectionIds); } +TEST_P(QuicConnectionTest, + SendAcksAfterOnCanWriteDisabledFlushesSeparateAckPacket) { + if (!connection_.SupportsMultiplePacketNumberSpaces()) { + return; + } + SetQuicReloadableFlag(quic_send_acks_after_on_can_write, false); + QuicConfig config; + config.SetConnectionOptionsToSend({kSAOW}); + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); + EXPECT_CALL(*send_algorithm_, EnableECT1()).WillOnce(Return(false)); + EXPECT_CALL(*send_algorithm_, EnableECT0()).WillOnce(Return(false)); + connection_.SetFromConfig(config); + + connection_.SetEncrypter(ENCRYPTION_HANDSHAKE, + std::make_unique<TaggingEncrypter>(0x02)); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE); + + EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(1); + ProcessCryptoPacketAtLevel(1, ENCRYPTION_INITIAL); + EXPECT_EQ(0u, writer_->packets_write_attempts()); + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(100)); + + std::string crypto_data(1000, 'a'); + EXPECT_CALL(visitor_, OnCanWrite()).WillOnce([&]() { + QuicConnection::ScopedEncryptionLevelContext context(&connection_, + ENCRYPTION_INITIAL); + connection_.SendCryptoDataWithString(crypto_data, 0, ENCRYPTION_INITIAL); + }); + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2); + connection_.OnCanWrite(); + + // When send_acks_after_on_can_write is false, SendAllPendingAcks() runs + // before visitor_->OnCanWrite() and flushes a standalone INITIAL ACK packet + // when switching back to ENCRYPTION_HANDSHAKE, resulting in 2 packets. + EXPECT_EQ(2u, writer_->packets_write_attempts()); +} + +TEST_P(QuicConnectionTest, SendAcksAfterOnCanWriteBundlesAckWithCryptoData) { + if (!connection_.SupportsMultiplePacketNumberSpaces()) { + return; + } + SetQuicReloadableFlag(quic_send_acks_after_on_can_write, true); + QuicConfig config; + config.SetConnectionOptionsToSend({kSAOW}); + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); + EXPECT_CALL(*send_algorithm_, EnableECT1()).WillOnce(Return(false)); + EXPECT_CALL(*send_algorithm_, EnableECT0()).WillOnce(Return(false)); + connection_.SetFromConfig(config); + + connection_.SetEncrypter(ENCRYPTION_HANDSHAKE, + std::make_unique<TaggingEncrypter>(0x02)); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE); + + // Receive an INITIAL packet so an INITIAL ACK alarm is scheduled, then + // advance the clock so the ACK timeout is due when OnCanWrite() runs. + EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(1); + ProcessCryptoPacketAtLevel(1, ENCRYPTION_INITIAL); + EXPECT_EQ(0u, writer_->packets_write_attempts()); + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(100)); + + std::string crypto_data(1000, 'a'); + EXPECT_CALL(visitor_, OnCanWrite()).WillOnce([&]() { + QuicConnection::ScopedEncryptionLevelContext context(&connection_, + ENCRYPTION_INITIAL); + connection_.SendCryptoDataWithString(crypto_data, 0, ENCRYPTION_INITIAL); + }); + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); + connection_.OnCanWrite(); + + // Because visitor_->OnCanWrite() ran before SendAllPendingAcks(), the pending + // INITIAL ACK was bundled into the single INITIAL CRYPTO packet rather than + // flushed as a separate INITIAL ACK packet before visitor_->OnCanWrite(). + EXPECT_EQ(1u, writer_->packets_write_attempts()); + EXPECT_EQ(1u, writer_->ack_frames().size()); + EXPECT_GE(writer_->crypto_frames().size(), 1u); +} + } // namespace } // namespace test } // namespace quic