gfe-relnote: Do not flush clear coalesced packets while sending multiple connection close packets. Protected by --gfe2_reloadable_flag_quic_close_all_encryptions_levels2 PiperOrigin-RevId: 282844268 Change-Id: I005056ad594f776a9a36198d012f7169737622b3
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index ed1fac4..9ecfa94 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2964,7 +2964,7 @@ void QuicConnection::SendConnectionClosePacket(QuicErrorCode error, const std::string& details) { - if (!GetQuicReloadableFlag(quic_close_all_encryptions_levels)) { + if (!GetQuicReloadableFlag(quic_close_all_encryptions_levels2)) { QUIC_DLOG(INFO) << ENDPOINT << "Sending connection close packet."; SetDefaultEncryptionLevel(GetConnectionCloseEncryptionLevel()); if (version().CanSendCoalescedPackets()) { @@ -2994,7 +2994,15 @@ } const EncryptionLevel current_encryption_level = encryption_level_; ScopedPacketFlusher flusher(this); - QUIC_RELOADABLE_FLAG_COUNT(quic_close_all_encryptions_levels); + QUIC_RELOADABLE_FLAG_COUNT(quic_close_all_encryptions_levels2); + + // Now that the connection is being closed, discard any unsent packets + // so the only packets to be sent will be connection close packets. + if (version().CanSendCoalescedPackets()) { + coalesced_packet_.Clear(); + } + ClearQueuedPackets(); + for (EncryptionLevel level : {ENCRYPTION_INITIAL, ENCRYPTION_HANDSHAKE, ENCRYPTION_ZERO_RTT, ENCRYPTION_FORWARD_SECURE}) { @@ -3004,10 +3012,6 @@ QUIC_DLOG(INFO) << ENDPOINT << "Sending connection close packet at level: " << EncryptionLevelToString(level); SetDefaultEncryptionLevel(level); - if (version().CanSendCoalescedPackets()) { - coalesced_packet_.Clear(); - } - ClearQueuedPackets(); // If there was a packet write error, write the smallest close possible. // When multiple packet number spaces are supported, an ACK frame will // be bundled by the ScopedPacketFlusher. Otherwise, an ACK must be sent @@ -3022,14 +3026,13 @@ framer_.current_received_frame_type()); packet_creator_.ConsumeRetransmittableControlFrame(QuicFrame(frame)); packet_creator_.FlushCurrentPacket(); - if (!version().CanSendCoalescedPackets()) { - ClearQueuedPackets(); - } } if (version().CanSendCoalescedPackets()) { FlushCoalescedPacket(); - ClearQueuedPackets(); } + // Since the connection is closing, if the connection close packets were not + // sent, then they should be discarded. + ClearQueuedPackets(); SetDefaultEncryptionLevel(current_encryption_level); }
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 0c6c542..37d3d36 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -337,6 +337,7 @@ final_bytes_of_previous_packet_(0), use_tagging_decrypter_(false), packets_write_attempts_(0), + connection_close_packets_(0), clock_(clock), write_pause_time_delta_(QuicTime::Delta::Zero()), max_packet_size_(kMaxOutgoingPacketSize), @@ -405,7 +406,9 @@ last_packet_size_ = packet.length(); last_packet_header_ = framer_.header(); - + if (!framer_.connection_close_frames().empty()) { + ++connection_close_packets_; + } if (!write_pause_time_delta_.IsZero()) { clock_->AdvanceTime(write_pause_time_delta_); } @@ -553,6 +556,10 @@ uint32_t packets_write_attempts() { return packets_write_attempts_; } + uint32_t connection_close_packets() const { + return connection_close_packets_; + } + void Reset() { framer_.Reset(); } void SetSupportedVersions(const ParsedQuicVersionVector& versions) { @@ -586,6 +593,7 @@ uint32_t final_bytes_of_previous_packet_; bool use_tagging_decrypter_; uint32_t packets_write_attempts_; + uint32_t connection_close_packets_; MockClock* clock_; // If non-zero, the clock will pause during WritePacket for this amount of // time. @@ -8044,7 +8052,7 @@ ProcessAckPacket(&ack); } -TEST_P(QuicConnectionTest, DonotForceSendingAckOnPacketTooLarge) { +TEST_P(QuicConnectionTest, DoNotForceSendingAckOnPacketTooLarge) { EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); // Send an ack by simulating delayed ack alarm firing. ProcessPacket(1); @@ -8055,13 +8063,74 @@ EXPECT_CALL(visitor_, OnConnectionClosed(_, _)); SimulateNextPacketTooLarge(); connection_.SendStreamDataWithString(3, "foo", 0, NO_FIN); - EXPECT_EQ(1u, writer_->frame_count()); - EXPECT_FALSE(writer_->connection_close_frames().empty()); + EXPECT_EQ(1u, writer_->connection_close_frames().size()); // Ack frame is not bundled in connection close packet. EXPECT_TRUE(writer_->ack_frames().empty()); + if (writer_->padding_frames().empty()) { + EXPECT_EQ(1u, writer_->frame_count()); + } else { + EXPECT_EQ(2u, writer_->frame_count()); + } + TestConnectionCloseQuicErrorCode(QUIC_PACKET_WRITE_ERROR); } +TEST_P(QuicConnectionTest, CloseConnectionAllLevels) { + SetQuicReloadableFlag(quic_close_all_encryptions_levels2, true); + + EXPECT_CALL(visitor_, OnConnectionClosed(_, _)); + const QuicErrorCode kQuicErrorCode = QUIC_INTERNAL_ERROR; + connection_.CloseConnection( + kQuicErrorCode, "Some random error message", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); + + EXPECT_EQ(2u, QuicConnectionPeer::GetNumEncryptionLevels(&connection_)); + + TestConnectionCloseQuicErrorCode(kQuicErrorCode); + EXPECT_EQ(1u, writer_->connection_close_frames().size()); + + if (!connection_.version().CanSendCoalescedPackets()) { + // Each connection close packet should be sent in distinct UDP packets. + EXPECT_EQ(QuicConnectionPeer::GetNumEncryptionLevels(&connection_), + writer_->connection_close_packets()); + EXPECT_EQ(QuicConnectionPeer::GetNumEncryptionLevels(&connection_), + writer_->packets_write_attempts()); + return; + } + + // A single UDP packet should be sent with multiple connection close packets + // coalesced together. + EXPECT_EQ(1u, writer_->packets_write_attempts()); + + // Only the first packet has been processed yet. + EXPECT_EQ(1u, writer_->connection_close_packets()); + + // ProcessPacket resets the visitor and frees the coalesced packet. + ASSERT_TRUE(writer_->coalesced_packet() != nullptr); + auto packet = writer_->coalesced_packet()->Clone(); + writer_->framer()->ProcessPacket(*packet); + EXPECT_EQ(1u, writer_->connection_close_packets()); + ASSERT_TRUE(writer_->coalesced_packet() == nullptr); +} + +TEST_P(QuicConnectionTest, CloseConnectionOneLevel) { + SetQuicReloadableFlag(quic_close_all_encryptions_levels2, false); + + EXPECT_CALL(visitor_, OnConnectionClosed(_, _)); + const QuicErrorCode kQuicErrorCode = QUIC_INTERNAL_ERROR; + connection_.CloseConnection( + kQuicErrorCode, "Some random error message", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); + + EXPECT_EQ(2u, QuicConnectionPeer::GetNumEncryptionLevels(&connection_)); + + TestConnectionCloseQuicErrorCode(kQuicErrorCode); + EXPECT_EQ(1u, writer_->connection_close_frames().size()); + EXPECT_EQ(1u, writer_->connection_close_packets()); + EXPECT_EQ(1u, writer_->packets_write_attempts()); + ASSERT_TRUE(writer_->coalesced_packet() == nullptr); +} + // Regression test for b/63620844. TEST_P(QuicConnectionTest, FailedToWriteHandshakePacket) { SimulateNextPacketTooLarge();
diff --git a/quic/test_tools/quic_connection_peer.cc b/quic/test_tools/quic_connection_peer.cc index 79fc7e3..7b9264c 100644 --- a/quic/test_tools/quic_connection_peer.cc +++ b/quic/test_tools/quic_connection_peer.cc
@@ -333,5 +333,18 @@ connection->SendConnectionClosePacket(error, details); } +// static +size_t QuicConnectionPeer::GetNumEncryptionLevels(QuicConnection* connection) { + size_t count = 0; + for (EncryptionLevel level : + {ENCRYPTION_INITIAL, ENCRYPTION_HANDSHAKE, ENCRYPTION_ZERO_RTT, + ENCRYPTION_FORWARD_SECURE}) { + if (connection->framer_.HasEncrypterOfEncryptionLevel(level)) { + ++count; + } + } + return count; +} + } // namespace test } // namespace quic
diff --git a/quic/test_tools/quic_connection_peer.h b/quic/test_tools/quic_connection_peer.h index 3ded2e0..b7140de 100644 --- a/quic/test_tools/quic_connection_peer.h +++ b/quic/test_tools/quic_connection_peer.h
@@ -133,6 +133,8 @@ static void SendConnectionClosePacket(QuicConnection* connection, QuicErrorCode error, const std::string& details); + + static size_t GetNumEncryptionLevels(QuicConnection* connection); }; } // namespace test