gfe-relnote: In QUIC, deprecate queued_control_frames_ from QuicPacketGenerator. Protected by gfe2_reloadable_flag_quic_deprecate_queued_control_frames. Change void AddControlFrame(const QuicFrame& frame) to bool ConsumeRetransmittableControlFrame(const QuicFrame& frame). PiperOrigin-RevId: 246495306 Change-Id: I48a645ea0ef88636afb725abf88da5a4f3125a07
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index aa7b55a..cf36197 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -1785,13 +1785,19 @@ } bool QuicConnection::SendControlFrame(const QuicFrame& frame) { - if (!CanWrite(HAS_RETRANSMITTABLE_DATA) && frame.type != PING_FRAME) { + if (!packet_generator_.deprecate_queued_control_frames() && + !CanWrite(HAS_RETRANSMITTABLE_DATA) && frame.type != PING_FRAME) { QUIC_DVLOG(1) << ENDPOINT << "Failed to send control frame: " << frame; // Do not check congestion window for ping. return false; } ScopedPacketFlusher flusher(this, SEND_ACK_IF_PENDING); - packet_generator_.AddControlFrame(frame); + const bool consumed = + packet_generator_.ConsumeRetransmittableControlFrame(frame); + if (packet_generator_.deprecate_queued_control_frames() && !consumed) { + QUIC_DVLOG(1) << ENDPOINT << "Failed to send control frame: " << frame; + return false; + } if (frame.type == PING_FRAME) { // Flush PING frame immediately. packet_generator_.FlushAllQueuedFrames(); @@ -3054,7 +3060,7 @@ if (transport_version() == QUIC_VERSION_99) { frame->close_type = IETF_QUIC_TRANSPORT_CONNECTION_CLOSE; } - packet_generator_.AddControlFrame(QuicFrame(frame)); + packet_generator_.ConsumeRetransmittableControlFrame(QuicFrame(frame)); packet_generator_.FlushAllQueuedFrames(); }
diff --git a/quic/core/quic_packet_generator.cc b/quic/core/quic_packet_generator.cc index d3d950c..7e98274 100644 --- a/quic/core/quic_packet_generator.cc +++ b/quic/core/quic_packet_generator.cc
@@ -30,7 +30,10 @@ random_generator_(random_generator), fully_pad_crypto_handshake_packets_(true), deprecate_ack_bundling_mode_( - GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) {} + GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)), + deprecate_queued_control_frames_( + deprecate_ack_bundling_mode_ && + GetQuicReloadableFlag(quic_deprecate_queued_control_frames)) {} QuicPacketGenerator::~QuicPacketGenerator() { DeleteFrames(&queued_control_frames_); @@ -53,14 +56,37 @@ SendQueuedFrames(/*flush=*/false); } -void QuicPacketGenerator::AddControlFrame(const QuicFrame& frame) { +bool QuicPacketGenerator::ConsumeRetransmittableControlFrame( + const QuicFrame& frame) { QUIC_BUG_IF(IsControlFrame(frame.type) && !GetControlFrameId(frame)) << "Adding a control frame with no control frame id: " << frame; + DCHECK(QuicUtils::IsRetransmittableFrame(frame.type)) << frame; if (deprecate_ack_bundling_mode_) { MaybeBundleAckOpportunistically(); } + if (deprecate_queued_control_frames_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_deprecate_queued_control_frames); + if (packet_creator_.HasPendingFrames()) { + if (packet_creator_.AddSavedFrame(frame, next_transmission_type_)) { + // There is pending frames and current frame fits. + return true; + } + } + DCHECK(!packet_creator_.HasPendingFrames()); + if (frame.type != PING_FRAME && frame.type != CONNECTION_CLOSE_FRAME && + !delegate_->ShouldGeneratePacket(HAS_RETRANSMITTABLE_DATA, + NOT_HANDSHAKE)) { + // Do not check congestion window for ping or connection close frames. + return false; + } + const bool success = + packet_creator_.AddSavedFrame(frame, next_transmission_type_); + DCHECK(success); + return success; + } queued_control_frames_.push_back(frame); SendQueuedFrames(/*flush=*/false); + return true; } size_t QuicPacketGenerator::ConsumeCryptoData(EncryptionLevel level,
diff --git a/quic/core/quic_packet_generator.h b/quic/core/quic_packet_generator.h index dc191a0..047295d 100644 --- a/quic/core/quic_packet_generator.h +++ b/quic/core/quic_packet_generator.h
@@ -92,7 +92,9 @@ // CreateAckFrame() when the packet is serialized. void SetShouldSendAck(bool also_send_stop_waiting); - void AddControlFrame(const QuicFrame& frame); + // Consumes retransmittable control |frame|. Returns true if the frame is + // successfully consumed. Returns false otherwise. + bool ConsumeRetransmittableControlFrame(const QuicFrame& frame); // Given some data, may consume part or all of it and pass it to the // packet creator to be serialized into packets. If not in batch @@ -251,6 +253,10 @@ return deprecate_ack_bundling_mode_; } + bool deprecate_queued_control_frames() const { + return deprecate_queued_control_frames_; + } + private: friend class test::QuicPacketGeneratorPeer; @@ -281,6 +287,8 @@ DelegateInterface* delegate_; QuicPacketCreator packet_creator_; + // TODO(fayang): remove this when deprecating + // quic_deprecate_queued_control_frames. QuicFrames queued_control_frames_; // Transmission type of the next serialized packet. @@ -309,6 +317,9 @@ // Latched value of quic_deprecate_ack_bundling_mode. const bool deprecate_ack_bundling_mode_; + + // Latched value of quic_deprecate_queued_control_frames. + const bool deprecate_queued_control_frames_; }; } // namespace quic
diff --git a/quic/core/quic_packet_generator_test.cc b/quic/core/quic_packet_generator_test.cc index bd867b8..dfd9ae2 100644 --- a/quic/core/quic_packet_generator_test.cc +++ b/quic/core/quic_packet_generator_test.cc
@@ -118,7 +118,8 @@ delegate_(static_cast<MockDelegate*>(delegate)), producer_(producer) {} - void AddControlFrame(const QuicFrame& frame, bool bundle_ack) { + bool ConsumeRetransmittableControlFrame(const QuicFrame& frame, + bool bundle_ack) { if (GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode) && !QuicPacketGeneratorPeer::GetPacketCreator(this)->has_ack()) { QuicFrames frames; @@ -131,7 +132,7 @@ .WillOnce(Return(frames)); } } - QuicPacketGenerator::AddControlFrame(frame); + return QuicPacketGenerator::ConsumeRetransmittableControlFrame(frame); } QuicConsumedData ConsumeDataFastPath(QuicStreamId id, @@ -414,26 +415,45 @@ TEST_F(QuicPacketGeneratorTest, AddControlFrame_NotWritable) { delegate_.SetCanNotWrite(); - generator_.AddControlFrame(QuicFrame(CreateRstStreamFrame()), - /*bundle_ack=*/false); - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_TRUE(generator_.HasRetransmittableFrames()); + QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); + const bool consumed = + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/false); + if (generator_.deprecate_queued_control_frames()) { + EXPECT_FALSE(consumed); + EXPECT_FALSE(generator_.HasQueuedFrames()); + EXPECT_FALSE(generator_.HasRetransmittableFrames()); + delete rst_frame; + } else { + EXPECT_TRUE(generator_.HasQueuedFrames()); + EXPECT_TRUE(generator_.HasRetransmittableFrames()); + } } TEST_F(QuicPacketGeneratorTest, AddControlFrame_OnlyAckWritable) { delegate_.SetCanWriteOnlyNonRetransmittable(); - generator_.AddControlFrame(QuicFrame(CreateRstStreamFrame()), - /*bundle_ack=*/false); - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_TRUE(generator_.HasRetransmittableFrames()); + QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); + const bool consumed = + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/false); + if (generator_.deprecate_queued_control_frames()) { + EXPECT_FALSE(consumed); + EXPECT_FALSE(generator_.HasQueuedFrames()); + EXPECT_FALSE(generator_.HasRetransmittableFrames()); + delete rst_frame; + } else { + EXPECT_TRUE(generator_.HasQueuedFrames()); + EXPECT_TRUE(generator_.HasRetransmittableFrames()); + } } TEST_F(QuicPacketGeneratorTest, AddControlFrame_WritableAndShouldNotFlush) { delegate_.SetCanWriteAnything(); - generator_.AddControlFrame(QuicFrame(CreateRstStreamFrame()), - /*bundle_ack=*/false); + generator_.ConsumeRetransmittableControlFrame( + QuicFrame(CreateRstStreamFrame()), + /*bundle_ack=*/false); EXPECT_TRUE(generator_.HasQueuedFrames()); EXPECT_TRUE(generator_.HasRetransmittableFrames()); } @@ -441,8 +461,17 @@ TEST_F(QuicPacketGeneratorTest, AddControlFrame_NotWritableBatchThenFlush) { delegate_.SetCanNotWrite(); - generator_.AddControlFrame(QuicFrame(CreateRstStreamFrame()), - /*bundle_ack=*/false); + QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); + const bool consumed = + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/false); + if (generator_.deprecate_queued_control_frames()) { + EXPECT_FALSE(consumed); + EXPECT_FALSE(generator_.HasQueuedFrames()); + EXPECT_FALSE(generator_.HasRetransmittableFrames()); + delete rst_frame; + return; + } EXPECT_TRUE(generator_.HasQueuedFrames()); EXPECT_TRUE(generator_.HasRetransmittableFrames()); generator_.Flush(); @@ -467,8 +496,9 @@ EXPECT_CALL(delegate_, OnSerializedPacket(_)) .WillOnce(Invoke(this, &QuicPacketGeneratorTest::SavePacket)); - generator_.AddControlFrame(QuicFrame(CreateRstStreamFrame()), - /*bundle_ack=*/false); + generator_.ConsumeRetransmittableControlFrame( + QuicFrame(CreateRstStreamFrame()), + /*bundle_ack=*/false); generator_.Flush(); EXPECT_FALSE(generator_.HasQueuedFrames()); EXPECT_FALSE(generator_.HasRetransmittableFrames()); @@ -778,10 +808,18 @@ if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { generator_.SetShouldSendAck(false); } - generator_.AddControlFrame(QuicFrame(CreateRstStreamFrame()), - /*bundle_ack=*/true); - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_TRUE(generator_.HasRetransmittableFrames()); + QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); + const bool success = + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/true); + if (generator_.deprecate_queued_control_frames()) { + EXPECT_FALSE(success); + EXPECT_FALSE(generator_.HasQueuedFrames()); + EXPECT_FALSE(generator_.HasRetransmittableFrames()); + } else { + EXPECT_TRUE(generator_.HasQueuedFrames()); + EXPECT_TRUE(generator_.HasRetransmittableFrames()); + } delegate_.SetCanWriteAnything(); @@ -789,11 +827,20 @@ EXPECT_CALL(delegate_, GetUpdatedAckFrame()) .WillOnce(Return(QuicFrame(&ack_frame_))); } + if (generator_.deprecate_queued_control_frames()) { + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/false); + } // Create a 10000 byte IOVector. CreateData(10000); EXPECT_CALL(delegate_, OnSerializedPacket(_)) .WillRepeatedly(Invoke(this, &QuicPacketGeneratorTest::SavePacket)); + if (generator_.deprecate_queued_control_frames()) { + generator_.ConsumeRetransmittableControlFrame( + QuicFrame(CreateRstStreamFrame()), + /*bundle_ack=*/true); + } QuicConsumedData consumed = generator_.ConsumeData( QuicUtils::GetHeadersStreamId(framer_.transport_version()), &iov_, 1u, iov_.iov_len, 0, FIN); @@ -863,10 +910,18 @@ if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { generator_.SetShouldSendAck(false); } - generator_.AddControlFrame(QuicFrame(CreateRstStreamFrame()), - /*bundle_ack=*/true); - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_TRUE(generator_.HasRetransmittableFrames()); + QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); + const bool consumed = + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/true); + if (generator_.deprecate_queued_control_frames()) { + EXPECT_FALSE(consumed); + EXPECT_FALSE(generator_.HasQueuedFrames()); + EXPECT_FALSE(generator_.HasRetransmittableFrames()); + } else { + EXPECT_TRUE(generator_.HasQueuedFrames()); + EXPECT_TRUE(generator_.HasRetransmittableFrames()); + } EXPECT_FALSE(generator_.HasPendingStreamFramesOfStream(3)); delegate_.SetCanWriteAnything(); @@ -877,13 +932,18 @@ EXPECT_CALL(delegate_, GetUpdatedAckFrame()) .WillOnce(Return(QuicFrame(&ack_frame_))); } - + if (generator_.deprecate_queued_control_frames()) { + EXPECT_TRUE( + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/false)); + } // Send some data and a control frame MakeIOVector("quux", &iov_); generator_.ConsumeData(3, &iov_, 1u, iov_.iov_len, 0, NO_FIN); if (framer_.transport_version() != QUIC_VERSION_99) { - generator_.AddControlFrame(QuicFrame(CreateGoAwayFrame()), - /*bundle_ack=*/false); + generator_.ConsumeRetransmittableControlFrame( + QuicFrame(CreateGoAwayFrame()), + /*bundle_ack=*/false); } EXPECT_TRUE(generator_.HasPendingStreamFramesOfStream(3)); @@ -918,10 +978,18 @@ if (!GetQuicReloadableFlag(quic_deprecate_ack_bundling_mode)) { generator_.SetShouldSendAck(false); } - generator_.AddControlFrame(QuicFrame(CreateRstStreamFrame()), - /*bundle_ack=*/true); - EXPECT_TRUE(generator_.HasQueuedFrames()); - EXPECT_TRUE(generator_.HasRetransmittableFrames()); + QuicRstStreamFrame* rst_frame = CreateRstStreamFrame(); + const bool success = + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/true); + if (generator_.deprecate_queued_control_frames()) { + EXPECT_FALSE(success); + EXPECT_FALSE(generator_.HasQueuedFrames()); + EXPECT_FALSE(generator_.HasRetransmittableFrames()); + } else { + EXPECT_TRUE(generator_.HasQueuedFrames()); + EXPECT_TRUE(generator_.HasRetransmittableFrames()); + } delegate_.SetCanWriteAnything(); @@ -940,7 +1008,11 @@ EXPECT_CALL(delegate_, OnSerializedPacket(_)) .WillOnce(Invoke(this, &QuicPacketGeneratorTest::SavePacket)); } - + if (generator_.deprecate_queued_control_frames()) { + EXPECT_TRUE( + generator_.ConsumeRetransmittableControlFrame(QuicFrame(rst_frame), + /*bundle_ack=*/false)); + } // Send enough data to exceed one packet size_t data_len = kDefaultMaxPacketSize + 100; CreateData(data_len); @@ -949,8 +1021,9 @@ EXPECT_EQ(data_len, consumed.bytes_consumed); EXPECT_TRUE(consumed.fin_consumed); if (framer_.transport_version() != QUIC_VERSION_99) { - generator_.AddControlFrame(QuicFrame(CreateGoAwayFrame()), - /*bundle_ack=*/false); + generator_.ConsumeRetransmittableControlFrame( + QuicFrame(CreateGoAwayFrame()), + /*bundle_ack=*/false); } generator_.Flush(); @@ -1355,7 +1428,8 @@ if (framer_.transport_version() == QUIC_VERSION_99) { frame->close_type = IETF_QUIC_TRANSPORT_CONNECTION_CLOSE; } - generator_.AddControlFrame(QuicFrame(frame), /*bundle_ack=*/false); + generator_.ConsumeRetransmittableControlFrame(QuicFrame(frame), + /*bundle_ack=*/false); EXPECT_TRUE(generator_.HasQueuedFrames()); EXPECT_TRUE(generator_.HasRetransmittableFrames()); }