gfe-relnote: In QUIC, do not close connection if received an in-order ACK with decreased largest_acked. Protected by gfe2_reloadable_flag_quic_tolerate_reneging.
By dremel, this connection close occurs rarely in prod: https://screenshot.googleplex.com/jZpfvdY4nzk
Largest_acked can decrease currently in IETF QUIC, though that may change: https://github.com/quicwg/base-drafts/issues/2205
PiperOrigin-RevId: 238228491
Change-Id: If7586fb8da192dea4436a5572c76bb7aeb1a3227
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index f4f15cb..0d7ede9 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -975,7 +975,8 @@
if (!sent_packet_manager_.GetLargestObserved().IsInitialized() ||
largest_acked > sent_packet_manager_.GetLargestObserved()) {
visitor_->OnForwardProgressConfirmed();
- } else if (largest_acked < sent_packet_manager_.GetLargestObserved()) {
+ } else if (!sent_packet_manager_.tolerate_reneging() &&
+ largest_acked < sent_packet_manager_.GetLargestObserved()) {
QUIC_LOG(INFO) << ENDPOINT << "Peer's largest_observed packet decreased:"
<< largest_acked << " vs "
<< sent_packet_manager_.GetLargestObserved()
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index acb76de..93ad4eb 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -2488,10 +2488,14 @@
QuicAckFrame frame2 = InitAckFrame(2);
ProcessAckPacket(&frame2);
- // Now change it to 1, and it should cause a connection error.
- EXPECT_CALL(visitor_, OnConnectionClosed(QUIC_INVALID_ACK_DATA, _,
- ConnectionCloseSource::FROM_SELF));
- EXPECT_CALL(visitor_, OnCanWrite()).Times(0);
+ if (GetQuicReloadableFlag(quic_tolerate_reneging)) {
+ EXPECT_CALL(visitor_, OnCanWrite());
+ } else {
+ // Now change it to 1, and it should cause a connection error.
+ EXPECT_CALL(visitor_, OnConnectionClosed(QUIC_INVALID_ACK_DATA, _,
+ ConnectionCloseSource::FROM_SELF));
+ EXPECT_CALL(visitor_, OnCanWrite()).Times(0);
+ }
ProcessAckPacket(&frame1);
}
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index 93909b7..e078f5f 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -111,7 +111,11 @@
delayed_ack_time_(
QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs)),
rtt_updated_(false),
- acked_packets_iter_(last_ack_frame_.packets.rbegin()) {
+ acked_packets_iter_(last_ack_frame_.packets.rbegin()),
+ tolerate_reneging_(GetQuicReloadableFlag(quic_tolerate_reneging)) {
+ if (tolerate_reneging_) {
+ QUIC_RELOADABLE_FLAG_COUNT(quic_tolerate_reneging);
+ }
SetSendAlgorithm(congestion_control_type);
}
@@ -1063,7 +1067,8 @@
rtt_updated_ =
MaybeUpdateRTT(largest_acked, ack_delay_time, ack_receive_time);
DCHECK(!unacked_packets_.largest_acked().IsInitialized() ||
- largest_acked >= unacked_packets_.largest_acked());
+ largest_acked >= unacked_packets_.largest_acked() ||
+ tolerate_reneging_);
last_ack_frame_.ack_delay_time = ack_delay_time;
acked_packets_iter_ = last_ack_frame_.packets.rbegin();
}
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h
index a50d83c..23f3a9f 100644
--- a/quic/core/quic_sent_packet_manager.h
+++ b/quic/core/quic_sent_packet_manager.h
@@ -351,6 +351,8 @@
// Setting the send algorithm once the connection is underway is dangerous.
void SetSendAlgorithm(SendAlgorithmInterface* send_algorithm);
+ bool tolerate_reneging() const { return tolerate_reneging_; }
+
private:
friend class test::QuicConnectionPeer;
friend class test::QuicSentPacketManagerPeer;
@@ -580,6 +582,9 @@
// A reverse iterator of last_ack_frame_.packets. This is reset in
// OnAckRangeStart, and gradually moves in OnAckRange..
PacketNumberQueue::const_reverse_iterator acked_packets_iter_;
+
+ // Latched value of quic_tolerate_reneging.
+ const bool tolerate_reneging_;
};
} // namespace quic
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc
index eb3c6a8..024b4c7 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -2433,6 +2433,38 @@
EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now()));
}
+TEST_P(QuicSentPacketManagerTest, TolerateReneging) {
+ if (!manager_.tolerate_reneging()) {
+ return;
+ }
+ // Send packets 1 - 20.
+ for (size_t i = 1; i <= 20; ++i) {
+ SendDataPacket(i);
+ }
+ // Ack [5, 7), [10, 12), [15, 17).
+ uint64_t acked1[] = {5, 6, 10, 11, 15, 16};
+ uint64_t lost1[] = {1, 2, 3, 4, 7, 8, 9, 12, 13};
+ ExpectAcksAndLosses(true, acked1, QUIC_ARRAYSIZE(acked1), lost1,
+ QUIC_ARRAYSIZE(lost1));
+ EXPECT_CALL(notifier_, OnFrameLost(_)).Times(AnyNumber());
+ manager_.OnAckFrameStart(QuicPacketNumber(16), QuicTime::Delta::Infinite(),
+ clock_.Now());
+ manager_.OnAckRange(QuicPacketNumber(15), QuicPacketNumber(17));
+ manager_.OnAckRange(QuicPacketNumber(10), QuicPacketNumber(12));
+ manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(7));
+ EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now()));
+
+ // Making sure reneged ACK does not harm. Ack [4, 8), [9, 13).
+ uint64_t acked2[] = {4, 7, 9, 12};
+ ExpectAcksAndLosses(true, acked2, QUIC_ARRAYSIZE(acked2), nullptr, 0);
+ manager_.OnAckFrameStart(QuicPacketNumber(12), QuicTime::Delta::Infinite(),
+ clock_.Now());
+ manager_.OnAckRange(QuicPacketNumber(9), QuicPacketNumber(13));
+ manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(8));
+ EXPECT_TRUE(manager_.OnAckFrameEnd(clock_.Now()));
+ EXPECT_EQ(QuicPacketNumber(16), manager_.GetLargestObserved());
+}
+
} // namespace
} // namespace test
} // namespace quic
