In quic, let client bundle handshake data (containing client finished) with handshake ack. protected by existing gfe2_reloadable_flag_quic_bundle_crypto_data_with_initial_ack.
PiperOrigin-RevId: 317663970
Change-Id: Ibe9b10b676899e7496e46810533447fedaf1b3a8
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 27bb4c4..b5c496f 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -4237,15 +4237,16 @@
return ENCRYPTION_INITIAL;
}
-void QuicConnection::MaybeBundleCryptoDataWithInitialAck() {
+void QuicConnection::MaybeBundleCryptoDataWithAckOfSpace(
+ PacketNumberSpace space) {
DCHECK(SupportsMultiplePacketNumberSpaces());
- const QuicTime initial_ack_timeout =
- uber_received_packet_manager_.GetAckTimeout(INITIAL_DATA);
- if (!initial_ack_timeout.IsInitialized() ||
- (initial_ack_timeout > clock_->ApproximateNow() &&
- initial_ack_timeout >
- uber_received_packet_manager_.GetEarliestAckTimeout())) {
- // Not going to send initial ACK.
+ QUIC_BUG_IF(space == APPLICATION_DATA);
+ const QuicTime ack_timeout =
+ uber_received_packet_manager_.GetAckTimeout(space);
+ if (!ack_timeout.IsInitialized() ||
+ (ack_timeout > clock_->ApproximateNow() &&
+ ack_timeout > uber_received_packet_manager_.GetEarliestAckTimeout())) {
+ // No pending ACK of space.
return;
}
if (coalesced_packet_.length() > 0) {
@@ -4253,9 +4254,8 @@
// packets.
return;
}
- // Initial ACK will be padded to full anyway, so try to bundle INITIAL crypto
- // data.
- sent_packet_manager_.RetransmitInitialDataIfAny();
+
+ sent_packet_manager_.RetransmitDataOfSpaceIfAny(space);
}
void QuicConnection::SendAllPendingAcks() {
@@ -4265,9 +4265,13 @@
QuicTime earliest_ack_timeout =
uber_received_packet_manager_.GetEarliestAckTimeout();
QUIC_BUG_IF(!earliest_ack_timeout.IsInitialized());
- if (GetQuicReloadableFlag(quic_bundle_crypto_data_with_initial_ack) &&
- perspective() == Perspective::IS_SERVER) {
- MaybeBundleCryptoDataWithInitialAck();
+ if (GetQuicReloadableFlag(quic_bundle_crypto_data_with_initial_ack)) {
+ // On the server side, sends INITIAL data with INITIAL ACK. On the client
+ // side, sends HANDSHAKE data (containing client Finished) with HANDSHAKE
+ // ACK.
+ PacketNumberSpace space =
+ perspective() == Perspective::IS_SERVER ? INITIAL_DATA : HANDSHAKE_DATA;
+ MaybeBundleCryptoDataWithAckOfSpace(space);
earliest_ack_timeout =
uber_received_packet_manager_.GetEarliestAckTimeout();
if (!earliest_ack_timeout.IsInitialized()) {
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h
index 6cf01f0..bc30a70 100644
--- a/quic/core/quic_connection.h
+++ b/quic/core/quic_connection.h
@@ -1318,8 +1318,9 @@
bool ValidateConfigConnectionIds(const QuicConfig& config);
bool ValidateConfigConnectionIdsOld(const QuicConfig& config);
- // Called when ACK alarm goes off. Try to bundle INITIAL data with the ACK.
- void MaybeBundleCryptoDataWithInitialAck();
+ // Called when ACK alarm goes off. Try to bundle crypto data with the ACK of
+ // |space|.
+ void MaybeBundleCryptoDataWithAckOfSpace(PacketNumberSpace space);
// Returns true if an undecryptable packet of |decryption_level| should be
// buffered (such that connection can try to decrypt it later).
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 44a1b69..affd89f 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1373,9 +1373,8 @@
level);
}
- size_t ProcessCryptoPacketAtLevel(uint64_t number,
- EncryptionLevel /*level*/) {
- QuicPacketHeader header = ConstructPacketHeader(number, ENCRYPTION_INITIAL);
+ size_t ProcessCryptoPacketAtLevel(uint64_t number, EncryptionLevel level) {
+ QuicPacketHeader header = ConstructPacketHeader(number, level);
QuicFrames frames;
if (QuicVersionUsesCryptoFrames(connection_.transport_version())) {
frames.push_back(QuicFrame(&crypto_frame_));
@@ -1385,10 +1384,10 @@
frames.push_back(QuicFrame(QuicPaddingFrame(-1)));
std::unique_ptr<QuicPacket> packet = ConstructPacket(header, frames);
char buffer[kMaxOutgoingPacketSize];
- peer_creator_.set_encryption_level(ENCRYPTION_INITIAL);
- size_t encrypted_length = peer_framer_.EncryptPayload(
- ENCRYPTION_INITIAL, QuicPacketNumber(number), *packet, buffer,
- kMaxOutgoingPacketSize);
+ peer_creator_.set_encryption_level(level);
+ size_t encrypted_length =
+ peer_framer_.EncryptPayload(level, QuicPacketNumber(number), *packet,
+ buffer, kMaxOutgoingPacketSize);
connection_.ProcessUdpPacket(
kSelfAddress, kPeerAddress,
QuicReceivedPacket(buffer, encrypted_length, clock_.Now(), false));
@@ -11257,7 +11256,7 @@
EXPECT_EQ(0u, QuicConnectionPeer::NumUndecryptablePackets(&connection_));
}
-TEST_P(QuicConnectionTest, BundleInitialDataWithInitialAck) {
+TEST_P(QuicConnectionTest, ServerBundlesInitialDataWithInitialAck) {
if (!connection_.SupportsMultiplePacketNumberSpaces()) {
return;
}
@@ -11308,6 +11307,44 @@
}
}
+TEST_P(QuicConnectionTest, ClientBundlesHandshakeDataWithHandshakeAck) {
+ if (!connection_.SupportsMultiplePacketNumberSpaces()) {
+ return;
+ }
+ EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective());
+ if (QuicVersionUsesCryptoFrames(connection_.transport_version())) {
+ EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber());
+ }
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber());
+ use_tagging_decrypter();
+ connection_.SetEncrypter(ENCRYPTION_HANDSHAKE,
+ std::make_unique<TaggingEncrypter>(0x02));
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE);
+ SetDecrypter(ENCRYPTION_HANDSHAKE,
+ std::make_unique<StrictTaggingDecrypter>(0x02));
+ peer_framer_.SetEncrypter(ENCRYPTION_HANDSHAKE,
+ std::make_unique<TaggingEncrypter>(0x02));
+ // Receives packet 1000 in handshake data.
+ ProcessCryptoPacketAtLevel(1000, ENCRYPTION_HANDSHAKE);
+ EXPECT_TRUE(connection_.HasPendingAcks());
+
+ EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(2);
+ connection_.SendCryptoDataWithString("foo", 0, ENCRYPTION_HANDSHAKE);
+
+ // Receives packet 1001 in handshake data.
+ ProcessCryptoPacketAtLevel(1001, ENCRYPTION_HANDSHAKE);
+ EXPECT_TRUE(connection_.HasPendingAcks());
+ // Receives packet 1002 in handshake data.
+ ProcessCryptoPacketAtLevel(1002, ENCRYPTION_HANDSHAKE);
+ EXPECT_FALSE(writer_->ack_frames().empty());
+ if (GetQuicReloadableFlag(quic_bundle_crypto_data_with_initial_ack)) {
+ // Verify CRYPTO frame is bundled with HANDSHAKE ACK.
+ EXPECT_FALSE(writer_->crypto_frames().empty());
+ } else {
+ EXPECT_TRUE(writer_->crypto_frames().empty());
+ }
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index eeeddf2..86628ef 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -916,11 +916,11 @@
pto_exponential_backoff_start_point_ = exponential_backoff_start_point;
}
-void QuicSentPacketManager::RetransmitInitialDataIfAny() {
+void QuicSentPacketManager::RetransmitDataOfSpaceIfAny(
+ PacketNumberSpace space) {
DCHECK(supports_multiple_packet_number_spaces());
- if (!unacked_packets_.GetLastInFlightPacketSentTime(INITIAL_DATA)
- .IsInitialized()) {
- // No in flight initial data.
+ if (!unacked_packets_.GetLastInFlightPacketSentTime(space).IsInitialized()) {
+ // No in flight data of space.
return;
}
QuicPacketNumber packet_number = unacked_packets_.GetLeastUnacked();
@@ -928,8 +928,7 @@
it != unacked_packets_.end(); ++it, ++packet_number) {
if (it->state == OUTSTANDING &&
unacked_packets_.HasRetransmittableFrames(*it) &&
- unacked_packets_.GetPacketNumberSpace(it->encryption_level) ==
- INITIAL_DATA) {
+ unacked_packets_.GetPacketNumberSpace(it->encryption_level) == space) {
DCHECK(it->in_flight);
MarkForRetransmission(packet_number, PTO_RETRANSMISSION);
return;
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h
index ca43d3d..72307b3 100644
--- a/quic/core/quic_sent_packet_manager.h
+++ b/quic/core/quic_sent_packet_manager.h
@@ -394,8 +394,8 @@
void StartExponentialBackoffAfterNthPto(
size_t exponential_backoff_start_point);
- // Called to retransmit in flight INITIAL packet if any.
- void RetransmitInitialDataIfAny();
+ // Called to retransmit in flight packet of |space| if any.
+ void RetransmitDataOfSpaceIfAny(PacketNumberSpace space);
bool supports_multiple_packet_number_spaces() const {
return unacked_packets_.supports_multiple_packet_number_spaces();
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc
index 33b2470..23aec1e 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -4079,7 +4079,7 @@
.WillOnce(WithArgs<1>(Invoke([this](TransmissionType type) {
RetransmitDataPacket(4, type, ENCRYPTION_INITIAL);
})));
- manager_.RetransmitInitialDataIfAny();
+ manager_.RetransmitDataOfSpaceIfAny(INITIAL_DATA);
// Verify PTO is re-armed based on packet 2.
EXPECT_EQ(packet2_sent_time + expected_pto_delay,
manager_.GetRetransmissionTime());
@@ -4090,7 +4090,7 @@
.WillOnce(WithArgs<1>(Invoke([this](TransmissionType type) {
RetransmitDataPacket(5, type, ENCRYPTION_INITIAL);
})));
- manager_.RetransmitInitialDataIfAny();
+ manager_.RetransmitDataOfSpaceIfAny(INITIAL_DATA);
// Verify PTO does not change.
EXPECT_EQ(packet2_sent_time + expected_pto_delay,
manager_.GetRetransmissionTime());
