Flush pending padding bytes before default_path_ is moved in connection migration. Protected by FLAGS_quic_reloadable_flag_quic_flush_pending_frames_and_padding_bytes_on_migration. PiperOrigin-RevId: 405888129
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index a93715d..aa598e4 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -5272,13 +5272,25 @@ << "EffectivePeerMigration started without address change."; return; } - // There could be pending NEW_TOKEN_FRAME triggered by non-probing - // PATH_RESPONSE_FRAME in the same packet. - if (packet_creator_.HasPendingFrames()) { + if (GetQuicReloadableFlag( + quic_flush_pending_frames_and_padding_bytes_on_migration)) { + QUIC_RELOADABLE_FLAG_COUNT( + quic_flush_pending_frames_and_padding_bytes_on_migration); + // There could be pending NEW_TOKEN_FRAME triggered by non-probing + // PATH_RESPONSE_FRAME in the same packet or pending padding bytes in the + // packet creator. packet_creator_.FlushCurrentPacket(); + packet_creator_.SendRemainingPendingPadding(); if (!connected_) { return; } + } else { + if (packet_creator_.HasPendingFrames()) { + packet_creator_.FlushCurrentPacket(); + if (!connected_) { + return; + } + } } // Action items:
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 09b4ebe..1b37830 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -512,6 +512,15 @@ return false; } + void SendOrQueuePacket(SerializedPacket packet) override { + QuicConnection::SendOrQueuePacket(std::move(packet)); + self_address_on_default_path_while_sending_packet_ = self_address(); + } + + QuicSocketAddress self_address_on_default_path_while_sending_packet() { + return self_address_on_default_path_while_sending_packet_; + } + SimpleDataProducer* producer() { return &producer_; } using QuicConnection::active_effective_peer_migration_type; @@ -538,6 +547,8 @@ SimpleSessionNotifier* notifier_; std::unique_ptr<QuicSocketAddress> next_effective_peer_addr_; + + QuicSocketAddress self_address_on_default_path_while_sending_packet_; }; enum class AckResponse { kDefer, kImmediate }; @@ -1320,15 +1331,15 @@ void set_perspective(Perspective perspective) { connection_.set_perspective(perspective); if (perspective == Perspective::IS_SERVER) { + QuicConfig config; if (!GetQuicReloadableFlag( quic_remove_connection_migration_connection_option)) { - QuicConfig config; QuicTagVector connection_options; connection_options.push_back(kRVCM); config.SetInitialReceivedConnectionOptions(connection_options); - EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); - connection_.SetFromConfig(config); } + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); + connection_.SetFromConfig(config); connection_.set_can_truncate_connection_ids(true); QuicConnectionPeer::SetNegotiatedVersion(&connection_); @@ -1509,8 +1520,7 @@ }; // Run all end to end tests with all supported versions. -INSTANTIATE_TEST_SUITE_P(QuicConnectionTests, - QuicConnectionTest, +INSTANTIATE_TEST_SUITE_P(QuicConnectionTests, QuicConnectionTest, ::testing::ValuesIn(GetTestParams()), ::testing::PrintToStringParamName()); @@ -2067,6 +2077,58 @@ } } +// Regression test for b/200020764. +TEST_P(QuicConnectionTest, ConnetcionMigrationWithPendingPaddingBytes) { + // TODO(haoyuewang) Move these test setup code to a common member function. + set_perspective(Perspective::IS_SERVER); + if (!connection_.connection_migration_use_new_cid()) { + return; + } + QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); + connection_.CreateConnectionIdManager(); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + QuicConnectionPeer::SetPeerAddress(&connection_, kPeerAddress); + QuicConnectionPeer::SetEffectivePeerAddress(&connection_, kPeerAddress); + QuicConnectionPeer::SetAddressValidated(&connection_); + + // Sends new server CID to client. + QuicConnectionId new_cid; + EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) + .WillOnce(Invoke([&](const QuicConnectionId& cid) { new_cid = cid; })); + EXPECT_CALL(visitor_, SendNewConnectionId(_)); + // Discard INITIAL key. + connection_.RemoveEncrypter(ENCRYPTION_INITIAL); + connection_.NeuterUnencryptedPackets(); + connection_.OnHandshakeComplete(); + EXPECT_CALL(visitor_, GetHandshakeState()) + .WillRepeatedly(Return(HANDSHAKE_CONFIRMED)); + + auto* packet_creator = QuicConnectionPeer::GetPacketCreator(&connection_); + packet_creator->FlushCurrentPacket(); + packet_creator->AddPendingPadding(50u); + const QuicSocketAddress kPeerAddress3 = + QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/56789); + auto ack_frame = InitAckFrame(1); + EXPECT_CALL(*send_algorithm_, OnCongestionEvent(_, _, _, _, _)); + EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1); + ProcessFramesPacketWithAddresses({QuicFrame(&ack_frame)}, kSelfAddress, + kPeerAddress3, ENCRYPTION_FORWARD_SECURE); + if (GetQuicReloadableFlag( + quic_flush_pending_frames_and_padding_bytes_on_migration)) { + // Any pending frames/padding should be flushed before default_path_ is + // temporarily reset. + ASSERT_EQ(connection_.self_address_on_default_path_while_sending_packet() + .host() + .address_family(), + IpAddressFamily::IP_V6); + } else { + ASSERT_EQ(connection_.self_address_on_default_path_while_sending_packet() + .host() + .address_family(), + IpAddressFamily::IP_UNSPEC); + } +} + // Regression test for b/196208556. TEST_P(QuicConnectionTest, ReversePathValidationResponseReceivedFromUnexpectedPeerAddress) { @@ -10595,10 +10657,8 @@ } void QuicConnectionTest::TestClientRetryHandling( - bool invalid_retry_tag, - bool missing_original_id_in_config, - bool wrong_original_id_in_config, - bool missing_retry_id_in_config, + bool invalid_retry_tag, bool missing_original_id_in_config, + bool wrong_original_id_in_config, bool missing_retry_id_in_config, bool wrong_retry_id_in_config) { if (invalid_retry_tag) { ASSERT_FALSE(missing_original_id_in_config);
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 509bae5..77d96ec 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -75,6 +75,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_drop_unsent_path_response, true) // If true, enable server retransmittable on wire PING. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_enable_server_on_wire_ping, true) +// If true, flush pending frames as well as pending padding bytes on connection migration. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_flush_pending_frames_and_padding_bytes_on_migration, false) // If true, ietf connection migration is no longer conditioned on connection option RVCM. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_remove_connection_migration_connection_option, true) // If true, ignore peer_max_ack_delay during handshake.