Stop blackhole detection when a duplicate ACK causes packets to be detected lost. Protected by existing gfe2_reloadable_flag_quic_default_enable_5rto_blackhole_detection2.
Do not close connection when blackhole is detected but there is no in flight packets.
PiperOrigin-RevId: 326693232
Change-Id: Id75b01e399fad12dcfc12dabd7245a2f059663de
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index bb0db5c..b7c9bb4 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -4206,6 +4206,13 @@
SetRetransmissionAlarm();
if (acked_new_packet) {
OnForwardProgressMade();
+ } else if (default_enable_5rto_blackhole_detection_ &&
+ !sent_packet_manager_.HasInFlightPackets() &&
+ blackhole_detector_.IsDetectionInProgress()) {
+ // In case no new packets get acknowledged, it is possible packets are
+ // detected lost because of time based loss detection. Cancel blackhole
+ // detection if there is no packets in flight.
+ blackhole_detector_.StopDetection();
}
if (send_stop_waiting) {
@@ -4707,16 +4714,15 @@
}
void QuicConnection::OnBlackholeDetected() {
- QUIC_BUG_IF(default_enable_5rto_blackhole_detection_ &&
- !sent_packet_manager_.HasInFlightPackets())
- << ENDPOINT
- << "Closing connection because of blackhole, but there is no bytes in "
- "flight";
- const std::string error_detail = quiche::QuicheStrCat(
- "Network blackhole detected", sent_packet_manager_.HasInFlightPackets()
- ? "."
- : " with no packets in flight.");
- CloseConnection(QUIC_TOO_MANY_RTOS, error_detail,
+ if (default_enable_5rto_blackhole_detection_ &&
+ !sent_packet_manager_.HasInFlightPackets()) {
+ QUIC_BUG << ENDPOINT
+ << "Blackhole detected, but there is no bytes in flight, version: "
+ << version();
+ // Do not close connection if there is no bytes in flight.
+ return;
+ }
+ CloseConnection(QUIC_TOO_MANY_RTOS, "Network blackhole detected",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
}
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 5340233..d80ca65 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -11698,6 +11698,70 @@
connection_.GetPingAlarm()->Fire();
}
+// Regression test for b/159698337
+TEST_P(QuicConnectionTest, DuplicateAckCausesLostPackets) {
+ if (!GetQuicReloadableFlag(quic_default_enable_5rto_blackhole_detection2)) {
+ return;
+ }
+ // Finish handshake.
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ peer_creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE);
+ notifier_.NeuterUnencryptedData();
+ connection_.NeuterUnencryptedPackets();
+ connection_.OnHandshakeComplete();
+ EXPECT_CALL(visitor_, GetHandshakeState())
+ .WillRepeatedly(Return(HANDSHAKE_COMPLETE));
+
+ std::string data(1200, 'a');
+ // Send data packets 1 - 5.
+ for (size_t i = 0; i < 5; ++i) {
+ SendStreamDataToPeer(
+ GetNthClientInitiatedStreamId(1, connection_.transport_version()), data,
+ i * 1200, i == 4 ? FIN : NO_FIN, nullptr);
+ }
+ ASSERT_TRUE(connection_.BlackholeDetectionInProgress());
+
+ EXPECT_CALL(*send_algorithm_, OnCongestionEvent(_, _, _, _, _)).Times(3);
+
+ // ACK packet 5 and 1 and 2 are detected lost.
+ QuicAckFrame frame =
+ InitAckFrame({{QuicPacketNumber(5), QuicPacketNumber(6)}});
+ LostPacketVector lost_packets;
+ lost_packets.push_back(
+ LostPacket(QuicPacketNumber(1), kMaxOutgoingPacketSize));
+ lost_packets.push_back(
+ LostPacket(QuicPacketNumber(2), kMaxOutgoingPacketSize));
+ EXPECT_CALL(*loss_algorithm_, DetectLosses(_, _, _, _, _, _))
+ .Times(AnyNumber())
+ .WillOnce(DoAll(SetArgPointee<5>(lost_packets),
+ Return(LossDetectionInterface::DetectionStats())));
+ ProcessAckPacket(1, &frame);
+ EXPECT_TRUE(connection_.BlackholeDetectionInProgress());
+ QuicAlarm* retransmission_alarm = connection_.GetRetransmissionAlarm();
+ EXPECT_TRUE(retransmission_alarm->IsSet());
+
+ // ACK packet 1 - 5 and 7.
+ QuicAckFrame frame2 =
+ InitAckFrame({{QuicPacketNumber(1), QuicPacketNumber(6)},
+ {QuicPacketNumber(7), QuicPacketNumber(8)}});
+ ProcessAckPacket(2, &frame2);
+ EXPECT_TRUE(connection_.BlackholeDetectionInProgress());
+
+ // ACK packet 7 again and assume packet 6 is detected lost.
+ QuicAckFrame frame3 =
+ InitAckFrame({{QuicPacketNumber(7), QuicPacketNumber(8)}});
+ lost_packets.clear();
+ lost_packets.push_back(
+ LostPacket(QuicPacketNumber(6), kMaxOutgoingPacketSize));
+ EXPECT_CALL(*loss_algorithm_, DetectLosses(_, _, _, _, _, _))
+ .Times(AnyNumber())
+ .WillOnce(DoAll(SetArgPointee<5>(lost_packets),
+ Return(LossDetectionInterface::DetectionStats())));
+ ProcessAckPacket(3, &frame3);
+ // Make sure loss detection is cancelled even there is no new acked packets.
+ EXPECT_FALSE(connection_.BlackholeDetectionInProgress());
+}
+
} // namespace
} // namespace test
} // namespace quic
