From d31df23ca5d0b254114770a23a271d8fe51eb1dc Mon Sep 17 00:00:00 2001 From: Konstantin Tsoy Date: Tue, 12 Apr 2022 09:38:19 -0700 Subject: [PATCH] Do not add ack frames to packet when it was rebuilt in cloning scheduler Summary: We may end up writing acks after a length-less stream frame and it will be considered garbage data by the peer. The acks will be written later as usual, outside of cloning scheduler. Reviewed By: mjoras Differential Revision: D35561912 fbshipit-source-id: 9334523b984870fa525c856a504ae7d2ae4f34c3 --- quic/api/QuicPacketScheduler.cpp | 8 -------- quic/api/test/QuicPacketSchedulerTest.cpp | 4 ++-- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 176ec7bb6..8559810e6 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -887,14 +887,6 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( // Rebuilder will write the rest of frames auto rebuildResult = rebuilder.rebuildFromPacket(outstandingPacket); if (rebuildResult) { - if (conn_.version.has_value() && - conn_.version.value() == QuicVersion::MVFST_EXPERIMENTAL2) { - // Check if we have any acks pending and write those into the cloned - // packet as well. - if (frameScheduler_.hasPendingAcks()) { - frameScheduler_.writeNextAcks(*internalBuilder); - } - } return SchedulingResult( std::move(rebuildResult), std::move(*internalBuilder).buildPacket()); } else if ( diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index d77463fb1..22c5fe87c 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -792,7 +792,7 @@ TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasHandshakeDataAndAcks) { EXPECT_TRUE(result.packetEvent.has_value()); EXPECT_TRUE(result.packet.has_value()); - // Cloned packet has to have at least one ack frame AND the crypto data. + // Cloned packet has to have crypto data and no acks. bool hasAckFrame = false; bool hasCryptoFrame = false; for (auto iter = result.packet->packet.frames.cbegin(); @@ -810,7 +810,7 @@ TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasHandshakeDataAndAcks) { break; } } - EXPECT_TRUE(hasAckFrame); + EXPECT_FALSE(hasAckFrame); EXPECT_TRUE(hasCryptoFrame); }