gfe-relnote: In QUIC sent packet manager, replace handshake_confirmed_ and forward_secure_packet_acked_ with handshake_state_. No functional change expected. Not protected. Also rename QuicConnection::IsHandshakeConfirmed to QuicConnection::IsHandshakeComplete. PiperOrigin-RevId: 281069350 Change-Id: Id40366a9148add245798b1805ae5f1e3a46d5fc6
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 0d5e1d8..d077196 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2508,7 +2508,8 @@ char* QuicConnection::GetPacketBuffer() { if (version().CanSendCoalescedPackets() && - !sent_packet_manager_.forward_secure_packet_acked()) { + sent_packet_manager_.handshake_state() < + QuicSentPacketManager::HANDSHAKE_CONFIRMED) { // Do not use writer's packet buffer for coalesced packets which may contain // multiple QUIC packets. return nullptr; @@ -2658,7 +2659,7 @@ void QuicConnection::OnRetransmissionTimeout() { DCHECK(!sent_packet_manager_.unacked_packets().empty() || (sent_packet_manager_.handshake_mode_disabled() && - !sent_packet_manager_.handshake_confirmed())); + !IsHandshakeComplete())); const QuicPacketNumber previous_created_packet_number = packet_creator_.packet_number(); if (close_connection_after_five_rtos_ && @@ -3702,7 +3703,7 @@ DCHECK(fill_up_link_during_probing_); // Don't send probing retransmissions until the handshake has completed. - if (!sent_packet_manager_.handshake_confirmed() || + if (!IsHandshakeComplete() || sent_packet_manager().HasUnackedCryptoPackets()) { return; } @@ -3898,7 +3899,7 @@ if (perspective_ == Perspective::IS_CLIENT) { return encryption_level_; } - if (sent_packet_manager_.handshake_confirmed()) { + if (IsHandshakeComplete()) { // A forward secure packet has been received. QUIC_BUG_IF(encryption_level_ != ENCRYPTION_FORWARD_SECURE) << ENDPOINT << "Unexpected connection close encryption level " @@ -4113,7 +4114,8 @@ return SEND_TO_WRITER; } if (version().CanSendCoalescedPackets() && - !sent_packet_manager_.forward_secure_packet_acked() && + sent_packet_manager_.handshake_state() < + QuicSentPacketManager::HANDSHAKE_CONFIRMED && !is_mtu_discovery) { // Before receiving ACK for any 1-RTT packets, always try to coalesce // packet (except MTU discovery packet).
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 6277f14..90369fc 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -859,9 +859,10 @@ NOT_PADDED_PING, // Set if the packet is not {PING, PADDING}. }; - // Whether the handshake is confirmed from this connection's perspective. - bool IsHandshakeConfirmed() const { - return sent_packet_manager_.handshake_confirmed(); + // Whether the handshake completes from this connection's perspective. + bool IsHandshakeComplete() const { + return sent_packet_manager_.handshake_state() >= + QuicSentPacketManager::HANDSHAKE_COMPLETE; } // Returns the largest received packet number sent by peer.
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 5194a9c..9c2a1c8 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -672,7 +672,7 @@ if (!QuicUtils::IsCryptoStreamId(transport_version(), id) && this->encryption_level() == ENCRYPTION_INITIAL) { this->SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); - if (perspective() == Perspective::IS_CLIENT && !IsHandshakeConfirmed()) { + if (perspective() == Perspective::IS_CLIENT && !IsHandshakeComplete()) { OnHandshakeComplete(); } if (version().SupportsAntiAmplificationLimit()) {
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc index 91d9b69..31faade 100644 --- a/quic/core/quic_dispatcher.cc +++ b/quic/core/quic_dispatcher.cc
@@ -578,7 +578,7 @@ !connection->termination_packets()->empty()) { action = QuicTimeWaitListManager::SEND_TERMINATION_PACKETS; } else { - if (!connection->IsHandshakeConfirmed()) { + if (!connection->IsHandshakeComplete()) { if (!VersionHasIetfInvariantHeader(connection->transport_version())) { QUIC_CODE_COUNT(gquic_add_to_time_wait_list_with_handshake_failed); } else {
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 408cc2c..ba3c081 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -92,7 +92,7 @@ min_rto_timeout_( QuicTime::Delta::FromMilliseconds(kMinRetransmissionTimeMs)), largest_mtu_acked_(0), - handshake_confirmed_(false), + handshake_state_(HANDSHAKE_START), peer_max_ack_delay_( QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs)), rtt_updated_(false), @@ -101,7 +101,6 @@ max_probe_packets_per_pto_(2), consecutive_pto_count_(0), handshake_mode_disabled_(false), - forward_secure_packet_acked_(false), skip_packet_number_for_pto_(false), always_include_max_ack_delay_for_pto_timeout_(true), pto_exponential_backoff_start_point_(0), @@ -337,11 +336,12 @@ } void QuicSentPacketManager::SetHandshakeConfirmed() { - if (!neuter_handshake_packets_once_ || !handshake_confirmed_) { + if (!neuter_handshake_packets_once_ || + handshake_state_ < HANDSHAKE_COMPLETE) { if (neuter_handshake_packets_once_) { QUIC_RELOADABLE_FLAG_COUNT_N(quic_neuter_handshake_packets_once2, 1, 3); } - handshake_confirmed_ = true; + handshake_state_ = HANDSHAKE_COMPLETE; NeuterHandshakePackets(); } } @@ -670,7 +670,7 @@ QuicSentPacketManager::RetransmissionTimeoutMode QuicSentPacketManager::OnRetransmissionTimeout() { DCHECK(unacked_packets_.HasInFlightPackets() || - (handshake_mode_disabled_ && !handshake_confirmed_)); + (handshake_mode_disabled_ && handshake_state_ < HANDSHAKE_COMPLETE)); DCHECK_EQ(0u, pending_timer_transmission_count_); // Handshake retransmission, timer based loss detection, TLP, and RTO are // implemented with a single alarm. The handshake alarm is set when the @@ -848,8 +848,8 @@ QuicSentPacketManager::RetransmissionTimeoutMode QuicSentPacketManager::GetRetransmissionMode() const { DCHECK(unacked_packets_.HasInFlightPackets() || - (handshake_mode_disabled_ && !handshake_confirmed_)); - if (!handshake_mode_disabled_ && !handshake_confirmed_ && + (handshake_mode_disabled_ && handshake_state_ < HANDSHAKE_COMPLETE)); + if (!handshake_mode_disabled_ && handshake_state_ < HANDSHAKE_COMPLETE && unacked_packets_.HasPendingCryptoPackets()) { return HANDSHAKE_MODE; } @@ -934,7 +934,7 @@ const QuicTime QuicSentPacketManager::GetRetransmissionTime() const { if (!unacked_packets_.HasInFlightPackets() && - (!handshake_mode_disabled_ || handshake_confirmed_ || + (!handshake_mode_disabled_ || handshake_state_ >= HANDSHAKE_COMPLETE || unacked_packets_.perspective() == Perspective::IS_SERVER)) { // Do not set the timer if there is nothing in flight. However, to avoid // handshake deadlock due to anti-amplification limit, client needs to set @@ -1225,7 +1225,7 @@ } last_ack_frame_.packets.Add(acked_packet.packet_number); if (info->encryption_level == ENCRYPTION_FORWARD_SECURE) { - forward_secure_packet_acked_ = true; + handshake_state_ = HANDSHAKE_CONFIRMED; } largest_packet_peer_knows_is_acked_.UpdateMax(info->largest_acked); if (supports_multiple_packet_number_spaces()) {
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index 4c2b429..5a3f2ca 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -151,8 +151,9 @@ return pacing_sender_.max_pacing_rate(); } - // Set handshake_confirmed_ to true and neuter packets in HANDSHAKE packet - // number space. + // Called to mark the handshake state complete, and all handshake packets are + // neutered. + // TODO(fayang): Rename this function to OnHandshakeComplete. void SetHandshakeConfirmed(); // Requests retransmission of all unacked packets of |retransmission_type|. @@ -363,7 +364,7 @@ return largest_packet_peer_knows_is_acked_; } - bool handshake_confirmed() const { return handshake_confirmed_; } + HandshakeState handshake_state() const { return handshake_state_; } size_t pending_timer_transmission_count() const { return pending_timer_transmission_count_; @@ -414,10 +415,6 @@ bool handshake_mode_disabled() const { return handshake_mode_disabled_; } - bool forward_secure_packet_acked() const { - return forward_secure_packet_acked_; - } - bool skip_packet_number_for_pto() const { return skip_packet_number_for_pto_; } @@ -598,11 +595,8 @@ // Calls into |send_algorithm_| for the underlying congestion control. PacingSender pacing_sender_; - // Set to true after the crypto handshake has successfully completed. After - // this is true we no longer use HANDSHAKE_MODE, and further frames sent on - // the crypto stream (i.e. SCUP messages) are treated like normal - // retransmittable frames. - bool handshake_confirmed_; + // Indicates current handshake state. + HandshakeState handshake_state_; // Records bandwidth from server to client in normal operation, over periods // of time with no loss events. @@ -644,10 +638,6 @@ // True if HANDSHAKE mode has been disabled. bool handshake_mode_disabled_; - // True if an acknowledgment has been received for a sent - // ENCRYPTION_FORWARD_SECURE packet. - bool forward_secure_packet_acked_; - // If true, skip packet number before sending the last PTO retransmission. bool skip_packet_number_for_pto_;
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 3ea50fd..8dd0c30 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -2758,7 +2758,8 @@ } TEST_F(QuicSentPacketManagerTest, ForwardSecurePacketAcked) { - EXPECT_FALSE(manager_.forward_secure_packet_acked()); + EXPECT_LT(manager_.handshake_state(), + QuicSentPacketManager::HANDSHAKE_CONFIRMED); SendDataPacket(1, ENCRYPTION_INITIAL); // Ack packet 1. ExpectAck(1); @@ -2768,7 +2769,8 @@ EXPECT_EQ(PACKETS_NEWLY_ACKED, manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(1), ENCRYPTION_INITIAL)); - EXPECT_FALSE(manager_.forward_secure_packet_acked()); + EXPECT_LT(manager_.handshake_state(), + QuicSentPacketManager::HANDSHAKE_CONFIRMED); SendDataPacket(2, ENCRYPTION_ZERO_RTT); // Ack packet 2. @@ -2779,7 +2781,8 @@ EXPECT_EQ(PACKETS_NEWLY_ACKED, manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(2), ENCRYPTION_FORWARD_SECURE)); - EXPECT_FALSE(manager_.forward_secure_packet_acked()); + EXPECT_LT(manager_.handshake_state(), + QuicSentPacketManager::HANDSHAKE_CONFIRMED); SendDataPacket(3, ENCRYPTION_FORWARD_SECURE); // Ack packet 3. @@ -2790,7 +2793,8 @@ EXPECT_EQ(PACKETS_NEWLY_ACKED, manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3), ENCRYPTION_FORWARD_SECURE)); - EXPECT_TRUE(manager_.forward_secure_packet_acked()); + EXPECT_EQ(manager_.handshake_state(), + QuicSentPacketManager::HANDSHAKE_CONFIRMED); } TEST_F(QuicSentPacketManagerTest, PtoTimeoutIncludesMaxAckDelay) {
diff --git a/quic/core/quic_session.h b/quic/core/quic_session.h index c4f2d98..03090d8 100644 --- a/quic/core/quic_session.h +++ b/quic/core/quic_session.h
@@ -73,6 +73,7 @@ }; // CryptoHandshakeEvent enumerates the events generated by a QuicCryptoStream. + // TODO(fayang): Replace this enum and with HandshakeState. enum CryptoHandshakeEvent { // ENCRYPTION_ESTABLISHED indicates that a client hello has been sent and // subsequent packets will be encrypted. (Client only.)
diff --git a/quic/core/tls_handshaker_test.cc b/quic/core/tls_handshaker_test.cc index d3463a8..e9dbd0d 100644 --- a/quic/core/tls_handshaker_test.cc +++ b/quic/core/tls_handshaker_test.cc
@@ -331,8 +331,8 @@ EXPECT_TRUE(client_stream_->encryption_established()); EXPECT_TRUE(server_stream_->handshake_confirmed()); EXPECT_TRUE(server_stream_->encryption_established()); - EXPECT_TRUE(client_conn_->IsHandshakeConfirmed()); - EXPECT_TRUE(server_conn_->IsHandshakeConfirmed()); + EXPECT_TRUE(client_conn_->IsHandshakeComplete()); + EXPECT_TRUE(server_conn_->IsHandshakeComplete()); const auto& client_crypto_params = client_stream_->crypto_negotiated_params(); @@ -367,8 +367,8 @@ }; TEST_F(TlsHandshakerTest, CryptoHandshake) { - EXPECT_FALSE(client_conn_->IsHandshakeConfirmed()); - EXPECT_FALSE(server_conn_->IsHandshakeConfirmed()); + EXPECT_FALSE(client_conn_->IsHandshakeComplete()); + EXPECT_FALSE(server_conn_->IsHandshakeComplete()); EXPECT_CALL(*client_conn_, CloseConnection(_, _, _)).Times(0); EXPECT_CALL(*server_conn_, CloseConnection(_, _, _)).Times(0);