Deprecate --gfe2_reloadable_flag_quic_give_sent_packet_to_debug_visitor_after_sent. Startblock: after 2020-11-21 09:00 in Google/US-CAM PiperOrigin-RevId: 343648304 Change-Id: Ibc53f46deea52a1357d81c2c4d79f8dba80b303a
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index b6ea5b1..90a1739 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -3052,12 +3052,6 @@ packet_send_time = packet_send_time + result.send_time_offset; } - if (!sent_packet_manager_.give_sent_packet_to_debug_visitor_after_sent() && - debug_visitor_ != nullptr) { - // Pass the write result to the visitor. - debug_visitor_->OnPacketSent(*packet, packet->transmission_type, - packet_send_time); - } if (IsRetransmittable(*packet) == HAS_RETRANSMITTABLE_DATA && !is_termination_packet) { // Start blackhole/path degrading detections if the sent packet is not @@ -3100,10 +3094,7 @@ << ENDPOINT << "Trying to start blackhole detection without no bytes in flight"; - if (sent_packet_manager_.give_sent_packet_to_debug_visitor_after_sent() && - debug_visitor_ != nullptr) { - QUIC_RELOADABLE_FLAG_COUNT_N( - quic_give_sent_packet_to_debug_visitor_after_sent, 1, 2); + if (debug_visitor_ != nullptr) { if (sent_packet_manager_.unacked_packets().empty()) { QUIC_BUG << "Unacked map is empty right after packet is sent"; } else { @@ -4492,21 +4483,12 @@ return false; } - if (!sent_packet_manager_.give_sent_packet_to_debug_visitor_after_sent() && - debug_visitor_ != nullptr) { - debug_visitor_->OnPacketSent(*packet, packet->transmission_type, - packet_send_time); - } - // Send in currrent path. Call OnPacketSent regardless of the write result. sent_packet_manager_.OnPacketSent(packet.get(), packet_send_time, packet->transmission_type, NO_RETRANSMITTABLE_DATA, measure_rtt); - if (sent_packet_manager_.give_sent_packet_to_debug_visitor_after_sent() && - debug_visitor_ != nullptr) { - QUIC_RELOADABLE_FLAG_COUNT_N( - quic_give_sent_packet_to_debug_visitor_after_sent, 2, 2); + if (debug_visitor_ != nullptr) { if (sent_packet_manager_.unacked_packets().empty()) { QUIC_BUG << "Unacked map is empty right after packet is sent"; } else {
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 599099d..599331d 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -225,12 +225,6 @@ ~QuicConnectionDebugVisitor() override {} // Called when a packet has been sent. - // TODO(wub): Delete when deprecating - // --quic_give_sent_packet_to_debug_visitor_after_sent. - virtual void OnPacketSent(const SerializedPacket& /*serialized_packet*/, - TransmissionType /*transmission_type*/, - QuicTime /*sent_time*/) {} - virtual void OnPacketSent(QuicPacketNumber /*packet_number*/, QuicPacketLength /*packet_length*/, bool /*has_crypto_handshake*/,
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 93169ea..a329404 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -6768,20 +6768,10 @@ MockQuicConnectionDebugVisitor debug_visitor; connection_.set_debug_visitor(&debug_visitor); - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)).Times(1); - } + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); connection_.SendStreamDataWithString(1, "foo", 0, NO_FIN); - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)).Times(1); - } + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); connection_.SendConnectivityProbingPacket(writer_.get(), connection_.peer_address()); } @@ -6908,12 +6898,7 @@ CongestionBlockWrites(); connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)).Times(1); - } + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); EXPECT_CALL(debug_visitor, OnPingSent()).Times(1); connection_.SendControlFrame(QuicFrame(QuicPingFrame(1))); EXPECT_FALSE(connection_.HasQueuedData()); @@ -6925,12 +6910,7 @@ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)).Times(1); - } + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); EXPECT_EQ(0u, connection_.GetStats().blocked_frames_sent); connection_.SendControlFrame(QuicFrame(new QuicBlockedFrame(1, 3))); EXPECT_EQ(1u, connection_.GetStats().blocked_frames_sent); @@ -6946,12 +6926,7 @@ QuicBlockedFrame blocked(1, 3); EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(0); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)).Times(0); - } + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(0); EXPECT_EQ(0u, connection_.GetStats().blocked_frames_sent); connection_.SendControlFrame(QuicFrame(&blocked)); EXPECT_EQ(0u, connection_.GetStats().blocked_frames_sent); @@ -7682,45 +7657,26 @@ } // Expect them retransmitted in cyclic order (foo, bar, test, foo, bar...). QuicPacketCount sent_count = 0; - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)) - .WillRepeatedly( - Invoke([this, &sent_count](QuicPacketNumber, QuicPacketLength, bool, - TransmissionType, EncryptionLevel, - const QuicFrames&, const QuicFrames&, - QuicTime) { - ASSERT_EQ(1u, writer_->stream_frames().size()); - if (connection_.version().CanSendCoalescedPackets()) { - // There is a delay of sending coalesced packet, so (6, 0, 3, 6, - // 0...). - EXPECT_EQ(3 * ((sent_count + 2) % 3), - writer_->stream_frames()[0]->offset); - } else { - // Identify the frames by stream offset (0, 3, 6, 0, 3...). - EXPECT_EQ(3 * (sent_count % 3), - writer_->stream_frames()[0]->offset); - } - sent_count++; - })); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)) - .WillRepeatedly(Invoke([this, &sent_count](const SerializedPacket&, - TransmissionType, QuicTime) { - ASSERT_EQ(1u, writer_->stream_frames().size()); - if (connection_.version().CanSendCoalescedPackets()) { - // There is a delay of sending coalesced packet, so (6, 0, 3, 6, - // 0...). - EXPECT_EQ(3 * ((sent_count + 2) % 3), - writer_->stream_frames()[0]->offset); - } else { - // Identify the frames by stream offset (0, 3, 6, 0, 3...). - EXPECT_EQ(3 * (sent_count % 3), - writer_->stream_frames()[0]->offset); - } - sent_count++; - })); - } + + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)) + .WillRepeatedly(Invoke( + [this, &sent_count](QuicPacketNumber, QuicPacketLength, bool, + TransmissionType, EncryptionLevel, + const QuicFrames&, const QuicFrames&, QuicTime) { + ASSERT_EQ(1u, writer_->stream_frames().size()); + if (connection_.version().CanSendCoalescedPackets()) { + // There is a delay of sending coalesced packet, so (6, 0, 3, 6, + // 0...). + EXPECT_EQ(3 * ((sent_count + 2) % 3), + writer_->stream_frames()[0]->offset); + } else { + // Identify the frames by stream offset (0, 3, 6, 0, 3...). + EXPECT_EQ(3 * (sent_count % 3), + writer_->stream_frames()[0]->offset); + } + sent_count++; + })); + EXPECT_CALL(*send_algorithm_, ShouldSendProbingPacket()) .WillRepeatedly(Return(true)); EXPECT_CALL(visitor_, SendProbingData()).WillRepeatedly([this] { @@ -7744,12 +7700,7 @@ MockQuicConnectionDebugVisitor debug_visitor; connection_.set_debug_visitor(&debug_visitor); - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(0); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)).Times(0); - } + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(0); EXPECT_CALL(*send_algorithm_, ShouldSendProbingPacket()) .WillRepeatedly(Return(true)); EXPECT_CALL(visitor_, SendProbingData()).WillRepeatedly([this] { @@ -9715,12 +9666,7 @@ } MockQuicConnectionDebugVisitor debug_visitor; connection_.set_debug_visitor(&debug_visitor); - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(3); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)).Times(3); - } + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(3); EXPECT_CALL(debug_visitor, OnCoalescedPacketSent(_, _)).Times(1); EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(1); { @@ -9762,12 +9708,7 @@ MockQuicConnectionDebugVisitor debug_visitor; connection_.set_debug_visitor(&debug_visitor); - if (connection_.sent_packet_manager() - .give_sent_packet_to_debug_visitor_after_sent()) { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); - } else { - EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _)).Times(1); - } + EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _)).Times(1); // Our TestPacketWriter normally parses the sent packet using the version // from the connection, so here we need to tell it to use the encapsulation
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 2fd5426..7ead17a 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -57,7 +57,6 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_pto_pending_timer_count, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_undecryptable_packets2, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_willing_and_able_to_write2, true) -QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_give_sent_packet_to_debug_visitor_after_sent, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_goaway_with_max_stream_id, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_granular_qpack_error_codes, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_key_update_supported, true)
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index 7dad0b0..f6a6fa8 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -456,10 +456,6 @@ QuicTime GetEarliestPacketSentTimeForPto( PacketNumberSpace* packet_number_space) const; - bool give_sent_packet_to_debug_visitor_after_sent() const { - return give_sent_packet_to_debug_visitor_after_sent_; - } - private: friend class test::QuicConnectionPeer; friend class test::QuicSentPacketManagerPeer; @@ -740,9 +736,6 @@ // The number of PTOs needed for path degrading alarm. If equals to 0, the // traditional path degrading mechanism will be used. int num_ptos_for_path_degrading_; - - const bool give_sent_packet_to_debug_visitor_after_sent_ = - GetQuicReloadableFlag(quic_give_sent_packet_to_debug_visitor_after_sent); }; } // namespace quic
diff --git a/quic/core/quic_trace_visitor.cc b/quic/core/quic_trace_visitor.cc index 7ac7b7b..a419808 100644 --- a/quic/core/quic_trace_visitor.cc +++ b/quic/core/quic_trace_visitor.cc
@@ -44,70 +44,6 @@ } } -void QuicTraceVisitor::OnPacketSent(const SerializedPacket& serialized_packet, - TransmissionType /*transmission_type*/, - QuicTime sent_time) { - quic_trace::Event* event = trace_.add_events(); - event->set_event_type(quic_trace::PACKET_SENT); - event->set_time_us(ConvertTimestampToRecordedFormat(sent_time)); - event->set_packet_number(serialized_packet.packet_number.ToUint64()); - event->set_packet_size(serialized_packet.encrypted_length); - event->set_encryption_level( - EncryptionLevelToProto(serialized_packet.encryption_level)); - - for (const QuicFrame& frame : serialized_packet.retransmittable_frames) { - switch (frame.type) { - case STREAM_FRAME: - case RST_STREAM_FRAME: - case CONNECTION_CLOSE_FRAME: - case WINDOW_UPDATE_FRAME: - case BLOCKED_FRAME: - case PING_FRAME: - case HANDSHAKE_DONE_FRAME: - case ACK_FREQUENCY_FRAME: - PopulateFrameInfo(frame, event->add_frames()); - break; - - case PADDING_FRAME: - case MTU_DISCOVERY_FRAME: - case STOP_WAITING_FRAME: - case ACK_FRAME: - QUIC_BUG - << "Frames of type are not retransmittable and are not supposed " - "to be in retransmittable_frames"; - break; - - // New IETF frames, not used in current gQUIC version. - case NEW_CONNECTION_ID_FRAME: - case RETIRE_CONNECTION_ID_FRAME: - case MAX_STREAMS_FRAME: - case STREAMS_BLOCKED_FRAME: - case PATH_RESPONSE_FRAME: - case PATH_CHALLENGE_FRAME: - case STOP_SENDING_FRAME: - case MESSAGE_FRAME: - case CRYPTO_FRAME: - case NEW_TOKEN_FRAME: - break; - - // Ignore gQUIC-specific frames. - case GOAWAY_FRAME: - break; - - case NUM_FRAME_TYPES: - QUIC_BUG << "Unknown frame type encountered"; - break; - } - } - - // Output PCC DebugState on packet sent for analysis. - if (connection_->sent_packet_manager() - .GetSendAlgorithm() - ->GetCongestionControlType() == kPCC) { - PopulateTransportState(event->mutable_transport_state()); - } -} - void QuicTraceVisitor::OnPacketSent( QuicPacketNumber packet_number, QuicPacketLength packet_length,
diff --git a/quic/core/quic_trace_visitor.h b/quic/core/quic_trace_visitor.h index 889d90a..0e6a385 100644 --- a/quic/core/quic_trace_visitor.h +++ b/quic/core/quic_trace_visitor.h
@@ -18,12 +18,6 @@ public: explicit QuicTraceVisitor(const QuicConnection* connection); - // TODO(wub): Delete when deprecating - // --quic_give_sent_packet_to_debug_visitor_after_sent. - void OnPacketSent(const SerializedPacket& serialized_packet, - TransmissionType transmission_type, - QuicTime sent_time) override; - void OnPacketSent(QuicPacketNumber packet_number, QuicPacketLength packet_length, bool has_crypto_handshake,
diff --git a/quic/test_tools/quic_test_utils.h b/quic/test_tools/quic_test_utils.h index f764e0b..5c217c7 100644 --- a/quic/test_tools/quic_test_utils.h +++ b/quic/test_tools/quic_test_utils.h
@@ -1457,13 +1457,6 @@ MockQuicConnectionDebugVisitor(); ~MockQuicConnectionDebugVisitor() override; - // TODO(wub): Delete when deprecating - // --quic_give_sent_packet_to_debug_visitor_after_sent. - MOCK_METHOD(void, - OnPacketSent, - (const SerializedPacket&, TransmissionType, QuicTime), - (override)); - MOCK_METHOD(void, OnPacketSent, (QuicPacketNumber,