QuicSentPacketManager::OnConnectionMigration() shouldn't mark packet for retransmission if the packet doesn't have transmittable frames. If such packet is still in flight, mark it not to contribute to congestion control.
PiperOrigin-RevId: 353659204
Change-Id: I3bb5f9b89f9c415349ba52614c7522209bd5d047
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index 73ee59e..4f2b200 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -661,7 +661,9 @@
QUIC_BUG_IF(transmission_type != LOSS_RETRANSMISSION &&
transmission_type != RTO_RETRANSMISSION &&
!unacked_packets_.HasRetransmittableFrames(*transmission_info))
- << "transmission_type: " << transmission_type;
+ << "packet number " << packet_number
+ << " transmission_type: " << transmission_type << " transmission_info "
+ << transmission_info->DebugString();
// Handshake packets should never be sent as probing retransmissions.
DCHECK(!transmission_info->has_crypto_handshake ||
transmission_type != PROBING_RETRANSMISSION);
@@ -1610,10 +1612,12 @@
unacked_packets_.RemoveFromInFlight(packet_number);
// Retransmitting these packets with PATH_CHANGE_RETRANSMISSION will mark
// them as useless, thus not contributing to RTT stats.
- MarkForRetransmission(packet_number, PATH_RETRANSMISSION);
- } else {
- it->state = NOT_CONTRIBUTING_RTT;
+ if (unacked_packets_.HasRetransmittableFrames(packet_number)) {
+ MarkForRetransmission(packet_number, PATH_RETRANSMISSION);
+ DCHECK_EQ(it->state, NOT_CONTRIBUTING_RTT);
+ }
}
+ it->state = NOT_CONTRIBUTING_RTT;
}
return old_send_algorithm;
}
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc
index ad30c41..f286de8 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -2333,7 +2333,7 @@
manager_.GetRttStats()->latest_rtt());
SendDataPacket(3, ENCRYPTION_FORWARD_SECURE);
- // Trigger loss timeout and makr packet3 for retransmission.
+ // Trigger loss timeout and mark packet3 for retransmission.
EXPECT_CALL(*loss_algorithm, GetLossTimeout())
.WillOnce(Return(clock_.Now() + QuicTime::Delta::FromMilliseconds(10)));
EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _))
@@ -2349,8 +2349,8 @@
EXPECT_EQ(0u, BytesInFlight());
// Migrate again with unACK'ed but not in-flight packet.
- // Packet3 shouldn't be marked for retransmission again as it is not in
- // flight.
+ // Packet3 shouldn't be marked for retransmission again as it is not in
+ // flight.
old_send_algorithm =
manager_.OnConnectionMigration(/*reset_send_algorithm=*/true);
@@ -2382,6 +2382,79 @@
ENCRYPTION_FORWARD_SECURE));
EXPECT_EQ(0u, BytesInFlight());
EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero());
+
+ SendDataPacket(4, ENCRYPTION_FORWARD_SECURE);
+ // Trigger loss timeout and mark packet4 for retransmission.
+ EXPECT_CALL(*loss_algorithm, GetLossTimeout())
+ .WillOnce(Return(clock_.Now() + QuicTime::Delta::FromMilliseconds(10)));
+ EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _))
+ .WillOnce(WithArgs<5>(Invoke([](LostPacketVector* packet_lost) {
+ packet_lost->emplace_back(QuicPacketNumber(4u), kDefaultLength);
+ return LossDetectionInterface::DetectionStats();
+ })));
+ EXPECT_CALL(notifier_, OnFrameLost(_));
+ EXPECT_CALL(*send_algorithm_,
+ OnCongestionEvent(false, kDefaultLength, _, _, _));
+ EXPECT_CALL(*network_change_visitor_, OnCongestionChange());
+ manager_.OnRetransmissionTimeout();
+ EXPECT_EQ(0u, BytesInFlight());
+
+ // Application retransmit the data as LOSS_RETRANSMISSION.
+ RetransmitDataPacket(5, LOSS_RETRANSMISSION, ENCRYPTION_FORWARD_SECURE);
+ EXPECT_EQ(kDefaultLength, BytesInFlight());
+
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(30));
+ // Receiving an ACK for packet4 should update RTT, but not bytes in flight.
+ // This spurious retransmission should be reported to the loss algorithm.
+ manager_.OnAckFrameStart(QuicPacketNumber(4), QuicTime::Delta::Infinite(),
+ clock_.Now());
+ manager_.OnAckRange(QuicPacketNumber(4), QuicPacketNumber(5));
+ EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _));
+ EXPECT_CALL(*loss_algorithm, SpuriousLossDetected(_, _, _, _, _));
+ EXPECT_CALL(*send_algorithm_,
+ OnCongestionEvent(/*rtt_updated=*/true, kDefaultLength, _, _, _));
+ EXPECT_CALL(*network_change_visitor_, OnCongestionChange());
+ EXPECT_CALL(notifier_, OnFrameAcked(_, _, _));
+ EXPECT_EQ(PACKETS_NEWLY_ACKED,
+ manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3),
+ ENCRYPTION_FORWARD_SECURE));
+ EXPECT_EQ(kDefaultLength, BytesInFlight());
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(30),
+ manager_.GetRttStats()->latest_rtt());
+
+ // Migrate again with in-flight packet5 whose retransmittable frames are all
+ // ACKed. Packet5 should be marked for retransmission but nothing to
+ // retransmit.
+ EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillOnce(Return(false));
+ EXPECT_CALL(notifier_, OnFrameLost(_)).Times(0u);
+ old_send_algorithm =
+ manager_.OnConnectionMigration(/*reset_send_algorithm=*/true);
+ EXPECT_EQ(default_init_rtt, rtt_stats->initial_rtt());
+ EXPECT_EQ(0u, manager_.GetConsecutiveRtoCount());
+ EXPECT_EQ(0u, manager_.GetConsecutiveTlpCount());
+ EXPECT_EQ(0u, BytesInFlight());
+ EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero());
+
+ manager_.SetSendAlgorithm(old_send_algorithm.release());
+
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10));
+ // Receiving an ACK for packet5 shouldn't update RTT. Though packet 5 was
+ // marked for retransmission, this spurious retransmission shouldn't be
+ // reported to the loss algorithm.
+ manager_.OnAckFrameStart(QuicPacketNumber(5), QuicTime::Delta::Infinite(),
+ clock_.Now());
+ manager_.OnAckRange(QuicPacketNumber(5), QuicPacketNumber(6));
+ EXPECT_CALL(*loss_algorithm, DetectLosses(_, _, _, _, _, _));
+ EXPECT_CALL(*loss_algorithm, SpuriousLossDetected(_, _, _, _, _)).Times(0u);
+ EXPECT_CALL(*send_algorithm_,
+ OnCongestionEvent(/*rtt_updated=*/false, 0, _, _, _));
+ EXPECT_CALL(*network_change_visitor_, OnCongestionChange());
+ EXPECT_CALL(notifier_, OnFrameAcked(_, _, _));
+ EXPECT_EQ(PACKETS_NEWLY_ACKED,
+ manager_.OnAckFrameEnd(clock_.Now(), QuicPacketNumber(3),
+ ENCRYPTION_FORWARD_SECURE));
+ EXPECT_EQ(0u, BytesInFlight());
+ EXPECT_TRUE(manager_.GetRttStats()->latest_rtt().IsZero());
}
TEST_F(QuicSentPacketManagerTest, PathMtuIncreased) {
