Deprecate --quic_dispatcher_replace_cid_on_first_packet. PiperOrigin-RevId: 678257063
diff --git a/quiche/common/quiche_feature_flags_list.h b/quiche/common/quiche_feature_flags_list.h index 22baac6..902a6db 100755 --- a/quiche/common/quiche_feature_flags_list.h +++ b/quiche/common/quiche_feature_flags_list.h
@@ -54,7 +54,6 @@ QUICHE_FLAG(bool, quiche_reloadable_flag_quic_testonly_default_true, true, true, "A testonly reloadable flag that will always default to true.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_use_received_client_addresses_cache, true, true, "If true, use a LRU cache to record client addresses of packets received on server's original address.") QUICHE_FLAG(bool, quiche_restart_flag_quic_dispatcher_ack_buffered_initial_packets, true, true, "If both this flag and --quic_dispatcher_replace_cid_on_first_packet are true, QUIC dispatcher will ack INITIAL packets once it is saved into buffered packet store.") -QUICHE_FLAG(bool, quiche_restart_flag_quic_dispatcher_replace_cid_on_first_packet, true, true, "If true, QuicDispatcher will always generate the replaced connection ID when the first INITIAL packet is received, even if it's buffered in the packet store.") QUICHE_FLAG(bool, quiche_restart_flag_quic_support_ect1, false, false, "When true, allows sending of QUIC packets marked ECT(1). A different flag (TBD) will actually utilize this capability to send ECT(1).") QUICHE_FLAG(bool, quiche_restart_flag_quic_support_release_time_for_gso, false, false, "If true, QuicGsoBatchWriter will support release time if it is available and the process has the permission to do so.") QUICHE_FLAG(bool, quiche_restart_flag_quic_testonly_default_false, false, false, "A testonly restart flag that will always default to false.")
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc index 978db9d..671eff1 100644 --- a/quiche/quic/core/http/end_to_end_test.cc +++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -308,8 +308,7 @@ } bool DispatcherAckEnabled() const { - return GetQuicRestartFlag(quic_dispatcher_ack_buffered_initial_packets) && - GetQuicRestartFlag(quic_dispatcher_replace_cid_on_first_packet); + return GetQuicRestartFlag(quic_dispatcher_ack_buffered_initial_packets); } void set_smaller_flow_control_receive_window() {
diff --git a/quiche/quic/core/quic_buffered_packet_store.cc b/quiche/quic/core/quic_buffered_packet_store.cc index 09f4bf4..bec282b 100644 --- a/quiche/quic/core/quic_buffered_packet_store.cc +++ b/quiche/quic/core/quic_buffered_packet_store.cc
@@ -131,87 +131,36 @@ packet_info.long_packet_type == INITIAL); QUIC_BUG_IF(quic_bug_12410_1, !GetQuicFlag(quic_allow_chlo_buffering)) << "Shouldn't buffer packets if disabled via flag."; - QUIC_BUG_IF(quic_bug_12410_2, - is_chlo && connections_with_chlo_.contains(connection_id)) - << "Shouldn't buffer duplicated CHLO on connection " << connection_id; QUIC_BUG_IF(quic_bug_12410_4, is_chlo && !version.IsKnown()) << "Should have version for CHLO packet."; - bool is_first_packet; - BufferedPacketListNode* node = nullptr; - - if (replace_cid_on_first_packet_) { - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 1, - 13); - auto iter = buffered_session_map_.find(connection_id); - is_first_packet = (iter == buffered_session_map_.end()); - if (is_first_packet) { - if (ShouldNotBufferPacket(is_chlo)) { - // Drop the packet if the upper limit of undecryptable packets has been - // reached or the whole capacity of the store has been reached. - return TOO_MANY_CONNECTIONS; - } - iter = buffered_session_map_.emplace_hint( - iter, connection_id, std::make_shared<BufferedPacketListNode>()); - iter->second->ietf_quic = ietf_quic; - iter->second->version = version; - iter->second->original_connection_id = connection_id; - iter->second->creation_time = clock_->ApproximateNow(); - buffered_sessions_.push_back(iter->second.get()); - ++num_buffered_sessions_; + auto iter = buffered_session_map_.find(connection_id); + const bool is_first_packet = (iter == buffered_session_map_.end()); + if (is_first_packet) { + if (ShouldNotBufferPacket(is_chlo)) { + // Drop the packet if the upper limit of undecryptable packets has been + // reached or the whole capacity of the store has been reached. + return TOO_MANY_CONNECTIONS; } - node = iter->second.get(); - QUICHE_DCHECK(buffered_session_map_.contains(connection_id)); - } else { - is_first_packet = !undecryptable_packets_.contains(connection_id); - if (is_first_packet) { - if (ShouldNotBufferPacket(is_chlo)) { - // Drop the packet if the upper limit of undecryptable packets has been - // reached or the whole capacity of the store has been reached. - return TOO_MANY_CONNECTIONS; - } - undecryptable_packets_.emplace( - std::make_pair(connection_id, BufferedPacketList())); - undecryptable_packets_.back().second.ietf_quic = ietf_quic; - undecryptable_packets_.back().second.version = version; - } - QUICHE_DCHECK(undecryptable_packets_.contains(connection_id)); + iter = buffered_session_map_.emplace_hint( + iter, connection_id, std::make_shared<BufferedPacketListNode>()); + iter->second->ietf_quic = ietf_quic; + iter->second->version = version; + iter->second->original_connection_id = connection_id; + iter->second->creation_time = clock_->ApproximateNow(); + buffered_sessions_.push_back(iter->second.get()); + ++num_buffered_sessions_; } + QUICHE_DCHECK(buffered_session_map_.contains(connection_id)); - BufferedPacketList& queue = - replace_cid_on_first_packet_ - ? *node - : undecryptable_packets_.find(connection_id)->second; + BufferedPacketListNode& queue = *iter->second; - if (replace_cid_on_first_packet_) { - // TODO(wub): Rename kDefaultMaxUndecryptablePackets when deprecating - // --quic_dispatcher_replace_cid_on_first_packet. - if (!is_chlo && - queue.buffered_packets.size() >= kDefaultMaxUndecryptablePackets) { - // If there are kMaxBufferedPacketsPerConnection packets buffered up for - // this connection, drop the current packet. - return TOO_MANY_PACKETS; - } - } else { - if (!is_chlo) { - // If current packet is not CHLO, it might not be buffered because store - // only buffers certain number of undecryptable packets per connection. - size_t num_non_chlo_packets = - connections_with_chlo_.contains(connection_id) - ? (queue.buffered_packets.size() - 1) - : queue.buffered_packets.size(); - if (num_non_chlo_packets >= kDefaultMaxUndecryptablePackets) { - // If there are kMaxBufferedPacketsPerConnection packets buffered up for - // this connection, drop the current packet. - return TOO_MANY_PACKETS; - } - } - - if (queue.buffered_packets.empty()) { - // If this is the first packet arrived on a new connection, initialize the - // creation time. - queue.creation_time = clock_->ApproximateNow(); - } + // TODO(wub): Rename kDefaultMaxUndecryptablePackets to kMaxBufferedPackets. + if (!is_chlo && + queue.buffered_packets.size() >= kDefaultMaxUndecryptablePackets) { + // If there are kMaxBufferedPacketsPerConnection packets buffered up for + // this connection, drop the current packet. + return TOO_MANY_PACKETS; } BufferedPacket new_entry(std::unique_ptr<QuicReceivedPacket>(packet.Clone()), @@ -223,17 +172,12 @@ queue.parsed_chlo = std::move(parsed_chlo); // Set the version of buffered packets of this connection on CHLO. queue.version = version; - if (replace_cid_on_first_packet_) { - if (!buffered_sessions_with_chlo_.is_linked(node)) { - buffered_sessions_with_chlo_.push_back(node); - ++num_buffered_sessions_with_chlo_; - } else { - QUIC_BUG(quic_store_session_already_has_chlo) - << "Buffered session already has CHLO"; - } + if (!buffered_sessions_with_chlo_.is_linked(&queue)) { + buffered_sessions_with_chlo_.push_back(&queue); + ++num_buffered_sessions_with_chlo_; } else { - queue.connection_id_generator = &connection_id_generator; - connections_with_chlo_[connection_id] = false; // Dummy value. + QUIC_BUG(quic_store_session_already_has_chlo) + << "Buffered session already has CHLO"; } } else { // Buffer non-CHLO packets in arrival order. @@ -253,10 +197,8 @@ MaybeSetExpirationAlarm(); - if (replace_cid_on_first_packet_ && is_ietf_initial_packet && - version.UsesTls() && !queue.HasAttemptedToReplaceConnectionId()) { - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 2, - 13); + if (is_ietf_initial_packet && version.UsesTls() && + !queue.HasAttemptedToReplaceConnectionId()) { queue.SetAttemptedToReplaceConnectionId(&connection_id_generator); std::optional<QuicConnectionId> replaced_connection_id = connection_id_generator.MaybeReplaceConnectionId(connection_id, @@ -281,7 +223,7 @@ case VisitorInterface::HandleCidCollisionResult::kOk: queue.replaced_connection_id = *replaced_connection_id; buffered_session_map_.insert( - {*replaced_connection_id, node->shared_from_this()}); + {*replaced_connection_id, queue.shared_from_this()}); break; case VisitorInterface::HandleCidCollisionResult::kCollision: return CID_COLLISION; @@ -428,17 +370,11 @@ bool QuicBufferedPacketStore::HasBufferedPackets( QuicConnectionId connection_id) const { - if (replace_cid_on_first_packet_) { - return buffered_session_map_.contains(connection_id); - } - return undecryptable_packets_.contains(connection_id); + return buffered_session_map_.contains(connection_id); } bool QuicBufferedPacketStore::HasChlosBuffered() const { - if (replace_cid_on_first_packet_) { - return num_buffered_sessions_with_chlo_ != 0; - } - return !connections_with_chlo_.empty(); + return num_buffered_sessions_with_chlo_ != 0; } const BufferedPacketList* QuicBufferedPacketStore::GetPacketList( @@ -486,54 +422,11 @@ BufferedPacketList QuicBufferedPacketStore::DeliverPackets( QuicConnectionId connection_id) { - if (!replace_cid_on_first_packet_) { - BufferedPacketList packets_to_deliver; - auto it = undecryptable_packets_.find(connection_id); - if (it != undecryptable_packets_.end()) { - packets_to_deliver = std::move(it->second); - undecryptable_packets_.erase(connection_id); - std::list<BufferedPacket> initial_packets; - std::list<BufferedPacket> other_packets; - for (auto& packet : packets_to_deliver.buffered_packets) { - QuicLongHeaderType long_packet_type = INVALID_PACKET_TYPE; - PacketHeaderFormat unused_format; - bool unused_version_flag; - bool unused_use_length_prefix; - QuicVersionLabel unused_version_label; - ParsedQuicVersion unused_parsed_version = UnsupportedQuicVersion(); - QuicConnectionId unused_destination_connection_id; - QuicConnectionId unused_source_connection_id; - std::optional<absl::string_view> unused_retry_token; - std::string unused_detailed_error; - - // We don't need to pass |generator| because we already got the correct - // connection ID length when we buffered the packet and indexed by - // connection ID. - QuicErrorCode error_code = QuicFramer::ParsePublicHeaderDispatcher( - *packet.packet, connection_id.length(), &unused_format, - &long_packet_type, &unused_version_flag, &unused_use_length_prefix, - &unused_version_label, &unused_parsed_version, - &unused_destination_connection_id, &unused_source_connection_id, - &unused_retry_token, &unused_detailed_error); - - if (error_code == QUIC_NO_ERROR && long_packet_type == INITIAL) { - initial_packets.push_back(std::move(packet)); - } else { - other_packets.push_back(std::move(packet)); - } - } - - initial_packets.splice(initial_packets.end(), other_packets); - packets_to_deliver.buffered_packets = std::move(initial_packets); - } - return packets_to_deliver; - } - auto it = buffered_session_map_.find(connection_id); if (it == buffered_session_map_.end()) { return BufferedPacketList(); } - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 3, 13); + std::shared_ptr<BufferedPacketListNode> node = it->second->shared_from_this(); RemoveFromStore(*node); std::list<BufferedPacket> initial_packets; @@ -552,13 +445,6 @@ } void QuicBufferedPacketStore::DiscardPackets(QuicConnectionId connection_id) { - if (!replace_cid_on_first_packet_) { - undecryptable_packets_.erase(connection_id); - connections_with_chlo_.erase(connection_id); - return; - } - - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 4, 13); auto it = buffered_session_map_.find(connection_id); if (it == buffered_session_map_.end()) { return; @@ -568,7 +454,6 @@ } void QuicBufferedPacketStore::RemoveFromStore(BufferedPacketListNode& node) { - QUICHE_DCHECK(replace_cid_on_first_packet_); QUICHE_DCHECK_EQ(buffered_sessions_with_chlo_.size(), num_buffered_sessions_with_chlo_); QUICHE_DCHECK_EQ(buffered_sessions_.size(), num_buffered_sessions_); @@ -610,40 +495,16 @@ } void QuicBufferedPacketStore::DiscardAllPackets() { - if (!replace_cid_on_first_packet_) { - undecryptable_packets_.clear(); - connections_with_chlo_.clear(); - } else { - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 5, - 13); - buffered_sessions_with_chlo_.clear(); - num_buffered_sessions_with_chlo_ = 0; - buffered_sessions_.clear(); - num_buffered_sessions_ = 0; - buffered_session_map_.clear(); - } + buffered_sessions_with_chlo_.clear(); + num_buffered_sessions_with_chlo_ = 0; + buffered_sessions_.clear(); + num_buffered_sessions_ = 0; + buffered_session_map_.clear(); expiration_alarm_->Cancel(); } void QuicBufferedPacketStore::OnExpirationTimeout() { QuicTime expiration_time = clock_->ApproximateNow() - connection_life_span_; - if (!replace_cid_on_first_packet_) { - while (!undecryptable_packets_.empty()) { - auto& entry = undecryptable_packets_.front(); - if (entry.second.creation_time > expiration_time) { - break; - } - QuicConnectionId connection_id = entry.first; - visitor_->OnExpiredPackets(connection_id, std::move(entry.second)); - undecryptable_packets_.pop_front(); - connections_with_chlo_.erase(connection_id); - } - if (!undecryptable_packets_.empty()) { - MaybeSetExpirationAlarm(); - } - return; - } - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 6, 13); while (!buffered_sessions_.empty()) { BufferedPacketListNode& node = buffered_sessions_.front(); if (node.creation_time > expiration_time) { @@ -666,26 +527,19 @@ } bool QuicBufferedPacketStore::ShouldNotBufferPacket(bool is_chlo) const { - size_t num_connections = replace_cid_on_first_packet_ - ? num_buffered_sessions_ - : undecryptable_packets_.size(); - - bool is_store_full = num_connections >= kDefaultMaxConnectionsInStore; + const bool is_store_full = + num_buffered_sessions_ >= kDefaultMaxConnectionsInStore; if (is_chlo) { return is_store_full; } - size_t num_connections_with_chlo = replace_cid_on_first_packet_ - ? num_buffered_sessions_with_chlo_ - : connections_with_chlo_.size(); - QUIC_BUG_IF(quic_store_too_many_connections_with_chlo, - num_connections < num_connections_with_chlo) - << "num_connections: " << num_connections - << ", num_connections_with_chlo: " << num_connections_with_chlo; + num_buffered_sessions_ < num_buffered_sessions_with_chlo_) + << "num_connections: " << num_buffered_sessions_ + << ", num_connections_with_chlo: " << num_buffered_sessions_with_chlo_; size_t num_connections_without_chlo = - num_connections - num_connections_with_chlo; + num_buffered_sessions_ - num_buffered_sessions_with_chlo_; bool reach_non_chlo_limit = num_connections_without_chlo >= kMaxConnectionsWithoutCHLO; @@ -694,29 +548,11 @@ BufferedPacketList QuicBufferedPacketStore::DeliverPacketsForNextConnection( QuicConnectionId* connection_id) { - if (!replace_cid_on_first_packet_) { - if (connections_with_chlo_.empty()) { - // Returns empty list if no CHLO has been buffered. - return BufferedPacketList(); - } - *connection_id = connections_with_chlo_.front().first; - connections_with_chlo_.pop_front(); - - BufferedPacketList packets = DeliverPackets(*connection_id); - QUICHE_DCHECK(!packets.buffered_packets.empty() && - packets.parsed_chlo.has_value()) - << "Try to deliver connectons without CHLO. # packets:" - << packets.buffered_packets.size() - << ", has_parsed_chlo:" << packets.parsed_chlo.has_value(); - return packets; - } - if (buffered_sessions_with_chlo_.empty()) { // Returns empty list if no CHLO has been buffered. return BufferedPacketList(); } - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 7, 13); *connection_id = buffered_sessions_with_chlo_.front().original_connection_id; BufferedPacketList packet_list = DeliverPackets(*connection_id); QUICHE_DCHECK(!packet_list.buffered_packets.empty() && @@ -729,14 +565,11 @@ bool QuicBufferedPacketStore::HasChloForConnection( QuicConnectionId connection_id) { - if (replace_cid_on_first_packet_) { - auto it = buffered_session_map_.find(connection_id); - if (it == buffered_session_map_.end()) { - return false; - } - return it->second->parsed_chlo.has_value(); + auto it = buffered_session_map_.find(connection_id); + if (it == buffered_session_map_.end()) { + return false; } - return connections_with_chlo_.contains(connection_id); + return it->second->parsed_chlo.has_value(); } bool QuicBufferedPacketStore::IngestPacketForTlsChloExtraction( @@ -752,28 +585,6 @@ QUICHE_DCHECK_NE(tls_alert, nullptr); QUICHE_DCHECK_EQ(version.handshake_protocol, PROTOCOL_TLS1_3); - if (!replace_cid_on_first_packet_) { - auto it = undecryptable_packets_.find(connection_id); - if (it == undecryptable_packets_.end()) { - QUIC_BUG(quic_bug_10838_1) - << "Cannot ingest packet for unknown connection ID " << connection_id; - return false; - } - it->second.tls_chlo_extractor.IngestPacket(version, packet); - if (!it->second.tls_chlo_extractor.HasParsedFullChlo()) { - *tls_alert = it->second.tls_chlo_extractor.tls_alert(); - return false; - } - const TlsChloExtractor& tls_chlo_extractor = it->second.tls_chlo_extractor; - *out_supported_groups = tls_chlo_extractor.supported_groups(); - *out_alpns = tls_chlo_extractor.alpns(); - *out_sni = tls_chlo_extractor.server_name(); - *out_resumption_attempted = tls_chlo_extractor.resumption_attempted(); - *out_early_data_attempted = tls_chlo_extractor.early_data_attempted(); - return true; - } - - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 8, 13); auto it = buffered_session_map_.find(connection_id); if (it == buffered_session_map_.end()) { QUIC_BUG(quic_bug_10838_1)
diff --git a/quiche/quic/core/quic_buffered_packet_store.h b/quiche/quic/core/quic_buffered_packet_store.h index 62cc462..80ebbc1 100644 --- a/quiche/quic/core/quic_buffered_packet_store.h +++ b/quiche/quic/core/quic_buffered_packet_store.h
@@ -128,7 +128,6 @@ // stored when the replaced CID is generated. ConnectionIdGeneratorInterface* connection_id_generator = nullptr; // The original connection ID of the connection. - // Only used when replace_cid_on_first_packet_ is true. QuicConnectionId original_connection_id; // Set to the result of ConnectionIdGenerator::MaybeReplaceConnectionId, // when the first IETF INITIAL packet is enqueued. @@ -142,10 +141,6 @@ absl::InlinedVector<DispatcherSentPacket, 2> dispatcher_sent_packets; }; - using BufferedPacketMap = - quiche::QuicheLinkedHashMap<QuicConnectionId, BufferedPacketList, - QuicConnectionIdHash>; - // Tag type for the list of sessions with full CHLO buffered. struct QUICHE_EXPORT BufferedSessionsWithChloList {}; @@ -268,10 +263,6 @@ // Is there any connection in the store that contains a full CHLO? bool HasChlosBuffered() const; - bool replace_cid_on_first_packet() const { - return replace_cid_on_first_packet_; - } - // Returns the BufferedPacketList for |connection_id|, returns // nullptr if not found. const BufferedPacketList* GetPacketList( @@ -302,9 +293,6 @@ // void RemoveFromStore(BufferedPacketListNode& node); - const bool replace_cid_on_first_packet_ = - GetQuicRestartFlag(quic_dispatcher_replace_cid_on_first_packet); - // Debug helper to check invariants that need to be true for |packet_list|, // assuming |packet_list| is in |buffer_session_map_|. Returns true if all // invariants are true, and false otherwise. @@ -325,25 +313,18 @@ QuicDispatcherStats& stats_; - // A map to store packet queues with creation time for each connection. - // Only used when !replace_cid_on_first_packet_. - BufferedPacketMap undecryptable_packets_; - // Map from connection ID to the list of buffered packets for that connection. // The key can be either the original or the replaced connection ID. // The value is never nullptr. - // Only used when replace_cid_on_first_packet_ is true. absl::flat_hash_map<QuicConnectionId, std::shared_ptr<BufferedPacketListNode>, QuicConnectionIdHash> buffered_session_map_; // Main list of all buffered sessions, in insertion order. - // Only used when replace_cid_on_first_packet_ is true. quiche::QuicheIntrusiveList<BufferedPacketListNode> buffered_sessions_; size_t num_buffered_sessions_ = 0; // Secondary list of all buffered sessions with full CHLO. - // Only used when replace_cid_on_first_packet_ is true. quiche::QuicheIntrusiveList<BufferedPacketListNode, BufferedSessionsWithChloList> buffered_sessions_with_chlo_; @@ -362,14 +343,7 @@ // packets staying in the store for too long. std::unique_ptr<QuicAlarm> expiration_alarm_; - // Keeps track of connection with CHLO buffered up already and the order they - // arrive. - // Only used when !replace_cid_on_first_packet_. - quiche::QuicheLinkedHashMap<QuicConnectionId, bool, QuicConnectionIdHash> - connections_with_chlo_; - const bool ack_buffered_initial_packets_ = - replace_cid_on_first_packet_ && GetQuicRestartFlag(quic_dispatcher_ack_buffered_initial_packets); };
diff --git a/quiche/quic/core/quic_buffered_packet_store_test.cc b/quiche/quic/core/quic_buffered_packet_store_test.cc index 507c980..265d909 100644 --- a/quiche/quic/core/quic_buffered_packet_store_test.cc +++ b/quiche/quic/core/quic_buffered_packet_store_test.cc
@@ -198,10 +198,7 @@ TEST_F(QuicBufferedPacketStoreTest, FailToBufferTooManyPacketsOnExistingConnection) { // Max number of packets that can be buffered per connection. - const size_t kMaxPacketsPerConnection = - store_.replace_cid_on_first_packet() - ? kDefaultMaxUndecryptablePackets - : kDefaultMaxUndecryptablePackets + 1; + const size_t kMaxPacketsPerConnection = kDefaultMaxUndecryptablePackets; QuicConnectionId connection_id = TestConnectionId(1); EXPECT_EQ(QuicBufferedPacketStore::SUCCESS, EnqueuePacketToStore(store_, connection_id, @@ -298,11 +295,7 @@ store_.DeliverPacketsForNextConnection(&delivered_conn_id); EXPECT_EQ(1u, packet_list.buffered_packets.size()); EXPECT_EQ(delivered_conn_id, TestConnectionId(1)); - if (GetQuicRestartFlag(quic_dispatcher_replace_cid_on_first_packet)) { - EXPECT_EQ(packet_list.connection_id_generator, nullptr); - } else { - EXPECT_EQ(packet_list.connection_id_generator, &connection_id_generator_); - } + EXPECT_EQ(packet_list.connection_id_generator, nullptr); } TEST_F(QuicBufferedPacketStoreTest, GeneratorIgnoredForNonChlo) { @@ -322,11 +315,7 @@ store_.DeliverPacketsForNextConnection(&delivered_conn_id); EXPECT_EQ(2u, packet_list.buffered_packets.size()); EXPECT_EQ(delivered_conn_id, TestConnectionId(1)); - if (GetQuicRestartFlag(quic_dispatcher_replace_cid_on_first_packet)) { - EXPECT_EQ(packet_list.connection_id_generator, nullptr); - } else { - EXPECT_EQ(packet_list.connection_id_generator, &connection_id_generator_); - } + EXPECT_EQ(packet_list.connection_id_generator, nullptr); } TEST_F(QuicBufferedPacketStoreTest, EnqueueChloOnTooManyDifferentConnections) { @@ -385,11 +374,7 @@ EXPECT_EQ(2u, packet_list.buffered_packets.size()); EXPECT_EQ(TestConnectionId(1u), delivered_conn_id); } - if (GetQuicRestartFlag(quic_dispatcher_replace_cid_on_first_packet)) { - EXPECT_EQ(packet_list.connection_id_generator, nullptr); - } else { - EXPECT_EQ(packet_list.connection_id_generator, &connection_id_generator_); - } + EXPECT_EQ(packet_list.connection_id_generator, nullptr); } EXPECT_FALSE(store_.HasChlosBuffered()); } @@ -448,11 +433,7 @@ // Connection 3 is the next to be delivered as connection 1 already expired. EXPECT_EQ(connection_id3, delivered_conn_id); - if (GetQuicRestartFlag(quic_dispatcher_replace_cid_on_first_packet)) { - EXPECT_EQ(packet_list.connection_id_generator, nullptr); - } else { - EXPECT_EQ(packet_list.connection_id_generator, &connection_id_generator_); - } + EXPECT_EQ(packet_list.connection_id_generator, nullptr); ASSERT_EQ(1u, packet_list.buffered_packets.size()); // Packets in connection 3 should use another peer address. EXPECT_EQ(another_client_address, @@ -584,11 +565,7 @@ // Since connection_id_2's chlo arrives, verify version is set. EXPECT_EQ(valid_version_, packets.version); - if (store_.replace_cid_on_first_packet()) { - EXPECT_FALSE(store_.HasChlosBuffered()); - } else { - EXPECT_TRUE(store_.HasChlosBuffered()); - } + EXPECT_FALSE(store_.HasChlosBuffered()); // Discard the packets for connection 2 store_.DiscardPackets(connection_id_2); EXPECT_FALSE(store_.HasChlosBuffered());
diff --git a/quiche/quic/core/quic_dispatcher.cc b/quiche/quic/core/quic_dispatcher.cc index 38cbd0c..d739cfb 100644 --- a/quiche/quic/core/quic_dispatcher.cc +++ b/quiche/quic/core/quic_dispatcher.cc
@@ -695,9 +695,6 @@ case EnqueuePacketResult::SUCCESS: break; case EnqueuePacketResult::CID_COLLISION: - QUICHE_DCHECK(buffered_packets_.replace_cid_on_first_packet()); - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, - 9, 13); buffered_packets_.DiscardPackets( packet_info.destination_connection_id); ABSL_FALLTHROUGH_INTENDED; @@ -1142,9 +1139,8 @@ return false; } -// TODO(wub): After deprecating --quic_dispatcher_replace_cid_on_first_packet, -// remove |server_connection_id| because |early_arrived_packets| already -// contains the original and replaced connection ID. +// TODO(wub): Remove |server_connection_id| because |early_arrived_packets| +// already contains the original and replaced connection ID. void QuicDispatcher::OnExpiredPackets( QuicConnectionId server_connection_id, BufferedPacketList early_arrived_packets) { @@ -1255,9 +1251,6 @@ case EnqueuePacketResult::SUCCESS: break; case EnqueuePacketResult::CID_COLLISION: - QUICHE_DCHECK(buffered_packets_.replace_cid_on_first_packet()); - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, - 10, 13); buffered_packets_.DiscardPackets( packet_info->destination_connection_id); ABSL_FALLTHROUGH_INTENDED; @@ -1270,77 +1263,40 @@ return; } - if (buffered_packets_.replace_cid_on_first_packet()) { - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 11, - 13); - BufferedPacketList packet_list = buffered_packets_.DeliverPackets( - packet_info->destination_connection_id); - // Get original_connection_id from buffered packets because - // destination_connection_id may be replaced connection_id if any packets - // have been sent by packet store. - QuicConnectionId original_connection_id = - packet_list.buffered_packets.empty() - ? packet_info->destination_connection_id - : packet_list.original_connection_id; + BufferedPacketList packet_list = + buffered_packets_.DeliverPackets(packet_info->destination_connection_id); + // Get original_connection_id from buffered packets because + // destination_connection_id may be replaced connection_id if any packets + // have been sent by packet store. + QuicConnectionId original_connection_id = + packet_list.buffered_packets.empty() + ? packet_info->destination_connection_id + : packet_list.original_connection_id; - TlsChloExtractor::State chlo_extractor_state = - packet_list.buffered_packets.empty() - ? TlsChloExtractor::State::kParsedFullSinglePacketChlo - : packet_list.tls_chlo_extractor.state(); - - auto session_ptr = CreateSessionFromChlo( - original_connection_id, packet_list.replaced_connection_id, parsed_chlo, - packet_info->version, packet_info->self_address, - packet_info->peer_address, chlo_extractor_state, - packet_list.connection_id_generator, - packet_list.dispatcher_sent_packets); - if (session_ptr == nullptr) { - // The only reason that CreateSessionFromChlo returns nullptr is because - // of CID collision, which can only happen if CreateSessionFromChlo - // attempted to replace the CID, CreateSessionFromChlo only replaces the - // CID when connection_id_generator is nullptr. - QUICHE_DCHECK_EQ(packet_list.connection_id_generator, nullptr); - return; - } - // Process the current packet first, then deliver queued-up packets. - // Note that multi-packet CHLOs, if received in packet number order, will - // not be delivered in the same order. This needs to be fixed. - session_ptr->ProcessUdpPacket(packet_info->self_address, - packet_info->peer_address, - packet_info->packet); - DeliverPacketsToSession(packet_list.buffered_packets, session_ptr.get()); - --new_sessions_allowed_per_event_loop_; - return; - } + TlsChloExtractor::State chlo_extractor_state = + packet_list.buffered_packets.empty() + ? TlsChloExtractor::State::kParsedFullSinglePacketChlo + : packet_list.tls_chlo_extractor.state(); auto session_ptr = CreateSessionFromChlo( - packet_info->destination_connection_id, std::nullopt, parsed_chlo, + original_connection_id, packet_list.replaced_connection_id, parsed_chlo, packet_info->version, packet_info->self_address, - packet_info->peer_address, - TlsChloExtractor::State::kParsedFullSinglePacketChlo, - &ConnectionIdGenerator(), - /*dispatcher_sent_packets=*/{}); + packet_info->peer_address, chlo_extractor_state, + packet_list.connection_id_generator, packet_list.dispatcher_sent_packets); if (session_ptr == nullptr) { + // The only reason that CreateSessionFromChlo returns nullptr is because + // of CID collision, which can only happen if CreateSessionFromChlo + // attempted to replace the CID, CreateSessionFromChlo only replaces the + // CID when connection_id_generator is nullptr. + QUICHE_DCHECK_EQ(packet_list.connection_id_generator, nullptr); return; } - std::list<BufferedPacket> packets = - buffered_packets_.DeliverPackets(packet_info->destination_connection_id) - .buffered_packets; - if (packet_info->destination_connection_id != session_ptr->connection_id()) { - // Provide the calling function with access to the new connection ID. - packet_info->destination_connection_id = session_ptr->connection_id(); - if (!packets.empty()) { - QUIC_CODE_COUNT( - quic_delivered_buffered_packets_to_connection_with_replaced_id); - } - } - // Process CHLO at first. + // Process the current packet first, then deliver queued-up packets. + // Note that multi-packet CHLOs, if received in packet number order, will + // not be delivered in the same order. This needs to be fixed. session_ptr->ProcessUdpPacket(packet_info->self_address, packet_info->peer_address, packet_info->packet); - // Deliver queued-up packets in the same order as they arrived. - // Do this even when flag is off because there might be still some packets - // buffered in the store before flag is turned off. - DeliverPacketsToSession(packets, session_ptr.get()); + DeliverPacketsToSession(packet_list.buffered_packets, session_ptr.get()); --new_sessions_allowed_per_event_loop_; } @@ -1401,84 +1357,41 @@ connection_id_generator = &ConnectionIdGenerator(); } std::optional<QuicConnectionId> server_connection_id; - if (buffered_packets_.replace_cid_on_first_packet()) { - if (should_generate_cid) { - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 12, - 13); - server_connection_id = connection_id_generator->MaybeReplaceConnectionId( - original_connection_id, version); - // Normalize the output of MaybeReplaceConnectionId. - if (server_connection_id.has_value() && - (server_connection_id->IsEmpty() || - *server_connection_id == original_connection_id)) { - server_connection_id.reset(); - } - QUIC_DVLOG(1) << "MaybeReplaceConnectionId(" << original_connection_id - << ") = " - << (server_connection_id.has_value() - ? server_connection_id->ToString() - : "nullopt"); - if (server_connection_id.has_value()) { - switch (HandleConnectionIdCollision( - original_connection_id, *server_connection_id, self_address, - peer_address, version, &parsed_chlo)) { - case VisitorInterface::HandleCidCollisionResult::kOk: - break; - case VisitorInterface::HandleCidCollisionResult::kCollision: - return nullptr; - } - } - } else { - QUIC_RESTART_FLAG_COUNT_N(quic_dispatcher_replace_cid_on_first_packet, 13, - 13); - server_connection_id = replaced_connection_id; - } - } else { + if (should_generate_cid) { server_connection_id = connection_id_generator->MaybeReplaceConnectionId( original_connection_id, version); + // Normalize the output of MaybeReplaceConnectionId. + if (server_connection_id.has_value() && + (server_connection_id->IsEmpty() || + *server_connection_id == original_connection_id)) { + server_connection_id.reset(); + } + QUIC_DVLOG(1) << "MaybeReplaceConnectionId(" << original_connection_id + << ") = " + << (server_connection_id.has_value() + ? server_connection_id->ToString() + : "nullopt"); + + if (server_connection_id.has_value()) { + switch (HandleConnectionIdCollision( + original_connection_id, *server_connection_id, self_address, + peer_address, version, &parsed_chlo)) { + case VisitorInterface::HandleCidCollisionResult::kOk: + break; + case VisitorInterface::HandleCidCollisionResult::kCollision: + return nullptr; + } + } + } else { + server_connection_id = replaced_connection_id; } + const bool connection_id_replaced = server_connection_id.has_value(); if (!connection_id_replaced) { server_connection_id = original_connection_id; } - if (!buffered_packets_.replace_cid_on_first_packet()) { - QUIC_CODE_COUNT(quic_connection_id_chosen); - if (reference_counted_session_map_.count(*server_connection_id) > 0) { - // The new connection ID is owned by another session. Avoid creating one - // altogether, as this connection attempt cannot possibly succeed. - QUIC_CODE_COUNT(quic_connection_id_collision); - QuicConnection* other_connection = - reference_counted_session_map_[*server_connection_id]->connection(); - if (other_connection != nullptr) { // Just make sure there is no crash. - QUIC_LOG_EVERY_N_SEC(ERROR, 10) - << "QUIC Connection ID collision. original_connection_id:" - << original_connection_id.ToString() - << " server_connection_id:" << server_connection_id->ToString() - << ", version:" << version << ", self_address:" << self_address - << ", peer_address:" << peer_address - << ", parsed_chlo:" << parsed_chlo - << ", other peer address: " << other_connection->peer_address() - << ", other CIDs: " - << quiche::PrintElements( - other_connection->GetActiveServerConnectionIds()) - << ", other stats: " << other_connection->GetStats(); - } - if (connection_id_replaced) { - QUIC_CODE_COUNT(quic_replaced_connection_id_collision); - // The original connection ID does not correspond to an existing - // session. It is safe to send CONNECTION_CLOSE and add to TIME_WAIT. - StatelesslyTerminateConnection( - self_address, peer_address, original_connection_id, - IETF_QUIC_LONG_HEADER_PACKET, - /*version_flag=*/true, version.HasLengthPrefixedConnectionIds(), - version, QUIC_HANDSHAKE_FAILED, - "Connection ID collision, please retry", - QuicTimeWaitListManager::SEND_CONNECTION_CLOSE_PACKETS); - } - return nullptr; - } - } + // Creates a new session and process all buffered packets for this connection. std::string alpn = SelectAlpn(parsed_chlo.alpns); std::unique_ptr<QuicSession> session = @@ -1543,7 +1456,6 @@ const QuicSocketAddress& self_address, const QuicSocketAddress& peer_address, ParsedQuicVersion version, const ParsedClientHello* parsed_chlo) { - QUICHE_DCHECK(buffered_packets_.replace_cid_on_first_packet()); HandleCidCollisionResult result = HandleCidCollisionResult::kOk; auto existing_session_iter = reference_counted_session_map_.find(replaced_connection_id);
diff --git a/quiche/quic/core/quic_dispatcher_test.cc b/quiche/quic/core/quic_dispatcher_test.cc index 45d02ea..6a46ce3 100644 --- a/quiche/quic/core/quic_dispatcher_test.cc +++ b/quiche/quic/core/quic_dispatcher_test.cc
@@ -2803,7 +2803,7 @@ for (uint64_t conn_id = 1; conn_id <= kNumCHLOs; ++conn_id) { const bool should_drop = (conn_id > kMaxNumSessionsToCreate + kDefaultMaxConnectionsInStore); - if (store->replace_cid_on_first_packet() && !should_drop) { + if (!should_drop) { // MaybeReplaceConnectionId will be called once per connection, whether it // is buffered or not. EXPECT_CALL(connection_id_generator_, @@ -2812,12 +2812,6 @@ } if (conn_id <= kMaxNumSessionsToCreate) { - if (!store->replace_cid_on_first_packet()) { - EXPECT_CALL( - connection_id_generator_, - MaybeReplaceConnectionId(TestConnectionId(conn_id), version_)) - .WillOnce(Return(std::nullopt)); - } EXPECT_CALL( *dispatcher_, CreateQuicSession(TestConnectionId(conn_id), _, client_addr_, @@ -2857,11 +2851,6 @@ ++conn_id) { // MaybeReplaceConnectionId should have been called once per buffered // session. - if (!store->replace_cid_on_first_packet()) { - EXPECT_CALL(connection_id_generator_, - MaybeReplaceConnectionId(TestConnectionId(conn_id), version_)) - .WillOnce(Return(std::nullopt)); - } EXPECT_CALL( *dispatcher_, CreateQuicSession(TestConnectionId(conn_id), _, client_addr_, @@ -2928,8 +2917,7 @@ MockConnectionIdGenerator generator2; EXPECT_CALL(*dispatcher_, ConnectionIdGenerator()) .WillRepeatedly(ReturnRef(generator2)); - const bool buffered_store_replace_cid = - store->replace_cid_on_first_packet() && version_.UsesTls(); + const bool buffered_store_replace_cid = version_.UsesTls(); if (buffered_store_replace_cid) { // generator2 should be used to replace the connection ID when the first // IETF INITIAL is enqueued. @@ -2946,12 +2934,8 @@ if (!buffered_store_replace_cid) { // QuicDispatcher should attempt to replace the CID when creating the - // QuicSession. If flag is false, it should use the latched |generator2|, - // otherwise it should use |connection_id_generator_|. - MockConnectionIdGenerator& generator = store->replace_cid_on_first_packet() - ? connection_id_generator_ - : generator2; - EXPECT_CALL(generator, + // QuicSession. + EXPECT_CALL(connection_id_generator_, MaybeReplaceConnectionId(TestConnectionId(conn_id), version_)) .WillOnce(Return(std::nullopt)); } @@ -3079,13 +3063,8 @@ QuicBufferedPacketStorePeer::FindBufferedPackets(store, last_connection_id); ASSERT_NE(last_connection_buffered_packets, nullptr); - if (store->replace_cid_on_first_packet()) { - ASSERT_EQ(last_connection_buffered_packets->buffered_packets.size(), - kDefaultMaxUndecryptablePackets); - } else { - ASSERT_EQ(last_connection_buffered_packets->buffered_packets.size(), - kDefaultMaxUndecryptablePackets + 1); - } + ASSERT_EQ(last_connection_buffered_packets->buffered_packets.size(), + kDefaultMaxUndecryptablePackets); // All buffered packets should be delivered to the session. EXPECT_CALL(*reinterpret_cast<MockQuicConnection*>(session1_->connection()), ProcessUdpPacket(_, _, _)) @@ -3131,7 +3110,7 @@ ValidatePacket(TestConnectionId(conn_id), packet); } }))); - } else if (!(store->replace_cid_on_first_packet() && version_.UsesTls())) { + } else if (!version_.UsesTls()) { expect_generator_is_called_ = false; } ProcessFirstFlight(TestConnectionId(conn_id)); @@ -3266,10 +3245,6 @@ class DualCIDBufferedPacketStoreTest : public BufferedPacketStoreTest { protected: void SetUp() override { - if (!GetQuicRestartFlag(quic_dispatcher_replace_cid_on_first_packet)) { - GTEST_SKIP(); - } - BufferedPacketStoreTest::SetUp(); QuicDispatcherPeer::set_new_sessions_allowed_per_event_loop( dispatcher_.get(), 0);
diff --git a/quiche/quic/test_tools/quic_buffered_packet_store_peer.cc b/quiche/quic/test_tools/quic_buffered_packet_store_peer.cc index 0e2aa15..4d6a972 100644 --- a/quiche/quic/test_tools/quic_buffered_packet_store_peer.cc +++ b/quiche/quic/test_tools/quic_buffered_packet_store_peer.cc
@@ -22,19 +22,11 @@ const QuicBufferedPacketStore::BufferedPacketList* QuicBufferedPacketStorePeer::FindBufferedPackets( const QuicBufferedPacketStore* store, QuicConnectionId connection_id) { - if (store->replace_cid_on_first_packet()) { - auto it = store->buffered_session_map_.find(connection_id); - if (it == store->buffered_session_map_.end()) { - return nullptr; - } - return it->second.get(); - } - - auto it = store->undecryptable_packets_.find(connection_id); - if (it == store->undecryptable_packets_.end()) { + auto it = store->buffered_session_map_.find(connection_id); + if (it == store->buffered_session_map_.end()) { return nullptr; } - return &it->second; + return it->second.get(); } } // namespace test