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());
}
