gfe-relnote: In QUIC, add connected_ check in OnCanWrite and flusher's destructor. Protected by gfe2_reloadable_flag_quic_check_connected_before_flush. PiperOrigin-RevId: 251653969 Change-Id: I93d9204391c7b900d0abb73aab9cab70f7ffabe9
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 1c589d6..83e6f04 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2094,6 +2094,10 @@ } void QuicConnection::OnCanWrite() { + if (GetQuicReloadableFlag(quic_check_connected_before_flush) && !connected_) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_check_connected_before_flush, 2, 2); + return; + } DCHECK(!writer_->IsWriteBlocked()); // Add a flusher to ensure the connection is marked app-limited. @@ -3459,6 +3463,11 @@ if (connection_ == nullptr) { return; } + if (GetQuicReloadableFlag(quic_check_connected_before_flush) && + !connection_->connected()) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_check_connected_before_flush, 1, 2); + return; + } if (flush_and_set_pending_retransmission_alarm_on_delete_) { if (connection_->packet_generator_.deprecate_ack_bundling_mode()) {
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 9cca434..ba40e7a 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -1943,7 +1943,11 @@ EXPECT_EQ(0u, connection_.GetStats().packets_discarded); connection_.OnCanWrite(); - EXPECT_EQ(1u, connection_.GetStats().packets_discarded); + if (GetQuicReloadableFlag(quic_check_connected_before_flush)) { + EXPECT_EQ(0u, connection_.GetStats().packets_discarded); + } else { + EXPECT_EQ(1u, connection_.GetStats().packets_discarded); + } } TEST_P(QuicConnectionTest, ReceiveConnectivityProbingAtServer) { @@ -8505,6 +8509,44 @@ EXPECT_EQ(TestConnectionId(0x33), connection_.client_connection_id()); } +// Regression test for b/134416344. +TEST_P(QuicConnectionTest, CheckConnectedBeforeFlush) { + // This test mimics a scenario where a connection processes 2 packets and the + // 2nd packet contains connection close frame. When the 2nd flusher goes out + // of scope, a delayed ACK is pending, and ACK alarm should not be scheduled + // because connection is disconnected. + EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); + EXPECT_CALL(visitor_, OnConnectionClosed(_, _, _)); + EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective()); + std::unique_ptr<QuicConnectionCloseFrame> connection_close_frame( + new QuicConnectionCloseFrame(QUIC_INTERNAL_ERROR)); + if (connection_.transport_version() == QUIC_VERSION_99) { + connection_close_frame->close_type = IETF_QUIC_TRANSPORT_CONNECTION_CLOSE; + } + // Received 2 packets. + QuicFrame frame; + if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { + frame = QuicFrame(&crypto_frame_); + EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); + } else { + frame = QuicFrame(QuicStreamFrame( + QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, + 0u, QuicStringPiece())); + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); + } + ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + QuicAlarm* ack_alarm = QuicConnectionPeer::GetAckAlarm(&connection_); + EXPECT_TRUE(ack_alarm->IsSet()); + ProcessFramePacketWithAddresses(QuicFrame(connection_close_frame.get()), + kSelfAddress, kPeerAddress); + if (GetQuicReloadableFlag(quic_check_connected_before_flush)) { + // Verify ack alarm is not set. + EXPECT_FALSE(ack_alarm->IsSet()); + } else { + EXPECT_TRUE(ack_alarm->IsSet()); + } +} + } // namespace } // namespace test } // namespace quic