diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index 8df33bd5f..ef8dd6306 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -2480,26 +2480,37 @@ TEST_F(QuicTransportTest, ClonePathResponse) { conn.outstandings.packets.begin(), conn.outstandings.packets.end(), findFrameInPacketFunc()); - EXPECT_EQ(numPathResponsePackets, 1); + EXPECT_EQ(numPathResponsePackets, 2); } -TEST_F(QuicTransportTest, DoNotResendPathResponseOnLoss) { +TEST_F(QuicTransportTest, ResendPathResponseOnLoss) { auto& conn = transport_->getConnectionState(); + auto& outstandingPackets = conn.outstandings.packets; - EXPECT_EQ(conn.pendingEvents.frames.size(), 0); - PathResponseFrame pathResponse(123); - sendSimpleFrame(conn, pathResponse); - EXPECT_EQ(conn.pendingEvents.frames.size(), 1); + sendSimpleFrame(conn, PathResponseFrame(folly::Random::rand64())); transport_->updateWriteLooper(true); loopForWrites(); - EXPECT_EQ(conn.pendingEvents.frames.size(), 0); - EXPECT_EQ(1, conn.outstandings.packets.size()); - auto packet = + // pathResponseFrame should no longer be in pendingEvents + EXPECT_EQ(conn.pendingEvents.frames.size(), 0); + // verify presence of frame in outstanding packet + EXPECT_EQ(outstandingPackets.size(), 1); + auto& packet = getLastOutstandingPacket(conn, PacketNumberSpace::AppData)->packet; + auto numPathResponseFrames = std::count_if( + outstandingPackets.begin(), + outstandingPackets.end(), + findFrameInPacketFunc()); + EXPECT_EQ(numPathResponseFrames, 1); + // pathResponseFrame should be queued for re-tx on packet loss markPacketLoss(conn, packet, false); - EXPECT_EQ(conn.pendingEvents.frames.size(), 0); + EXPECT_EQ(conn.pendingEvents.frames.size(), 1); + numPathResponseFrames = std::count_if( + conn.pendingEvents.frames.begin(), + conn.pendingEvents.frames.end(), + [](const auto& frame) { return frame.asPathResponseFrame(); }); + EXPECT_EQ(numPathResponseFrames, 1); } TEST_F(QuicTransportTest, SendNewConnectionIdFrame) { diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 64c29823d..6d2e37b76 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -591,10 +591,8 @@ void onConnectionMigration( previousPeerAddresses.end(), newPeerAddress); if (it == previousPeerAddresses.end()) { - // Send new path challenge - uint64_t pathData; - folly::Random::secureRandom(&pathData, sizeof(pathData)); - conn.pendingEvents.pathChallenge = PathChallengeFrame(pathData); + // send new path challenge + conn.pendingEvents.pathChallenge.emplace(folly::Random::secureRand64()); // If we are already in the middle of a migration reset // the available bytes in the rate-limited window, but keep the diff --git a/quic/state/SimpleFrameFunctions.cpp b/quic/state/SimpleFrameFunctions.cpp index c02064f86..85d2f7575 100644 --- a/quic/state/SimpleFrameFunctions.cpp +++ b/quic/state/SimpleFrameFunctions.cpp @@ -35,8 +35,7 @@ folly::Optional updateSimpleFrameOnPacketClone( } return QuicSimpleFrame(frame); case QuicSimpleFrame::Type::PathResponseFrame: - // Do not clone PATH_RESPONSE to avoid buffering - return folly::none; + return QuicSimpleFrame(frame); case QuicSimpleFrame::Type::NewConnectionIdFrame: case QuicSimpleFrame::Type::MaxStreamsFrame: case QuicSimpleFrame::Type::HandshakeDoneFrame: @@ -91,12 +90,11 @@ void updateSimpleFrameOnPacketLoss( break; } case QuicSimpleFrame::Type::PathResponseFrame: { - // Do not retransmit PATH_RESPONSE to avoid buffering + conn.pendingEvents.frames.push_back(*frame.asPathResponseFrame()); break; } case QuicSimpleFrame::Type::HandshakeDoneFrame: { - const auto& handshakeDoneFrame = *frame.asHandshakeDoneFrame(); - conn.pendingEvents.frames.push_back(handshakeDoneFrame); + conn.pendingEvents.frames.push_back(*frame.asHandshakeDoneFrame()); break; } case QuicSimpleFrame::Type::NewConnectionIdFrame: