Skip to content

Commit

Permalink
Default to PTOing PING frames.
Browse files Browse the repository at this point in the history
Summary: This kinda makes sense to just use as a default.

Reviewed By: kvtsoy, sharmafb

Differential Revision: D64066392

fbshipit-source-id: 0915f163c0483af6bec014bde61e82b6ee2ac6cb
  • Loading branch information
Matt Joras authored and facebook-github-bot committed Oct 9, 2024
1 parent 16ca1bc commit 28748bf
Show file tree
Hide file tree
Showing 5 changed files with 2 additions and 46 deletions.
4 changes: 1 addition & 3 deletions quic/api/QuicTransportFunctions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -801,9 +801,7 @@ void updateConnection(
case QuicWriteFrame::Type::PingFrame:
conn.pendingEvents.sendPing = false;
isPing = true;
if (conn.transportSettings.ptoPingFrames) {
retransmittable = true;
}
retransmittable = true;
conn.numPingFramesSent++;
break;
case QuicWriteFrame::Type::QuicSimpleFrame: {
Expand Down
13 changes: 0 additions & 13 deletions quic/api/test/QuicTransportFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -206,21 +206,8 @@ class QuicTransportFunctionsTest : public Test {
std::unique_ptr<MockQuicStats> quicStats_;
};

TEST_F(QuicTransportFunctionsTest, PingPacketGoesToOPList) {
auto conn = createConn();
auto packet = buildEmptyPacket(*conn, PacketNumberSpace::AppData);
packet.packet.frames.push_back(PingFrame());
EXPECT_EQ(0, conn->outstandings.packets.size());
updateConnection(
*conn, none, packet.packet, Clock::now(), 50, 0, false /* isDSRPacket */);
EXPECT_EQ(1, conn->outstandings.packets.size());
// But it won't set loss detection alarm
EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm);
}

TEST_F(QuicTransportFunctionsTest, PingPacketGoesToOPListAndLossAlarm) {
auto conn = createConn();
conn->transportSettings.ptoPingFrames = true;
auto packet = buildEmptyPacket(*conn, PacketNumberSpace::AppData);
packet.packet.frames.push_back(PingFrame());
EXPECT_EQ(0, conn->outstandings.packets.size());
Expand Down
4 changes: 1 addition & 3 deletions quic/codec/QuicPacketRebuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,7 @@ Optional<ClonedPacketIdentifier> PacketRebuilder::rebuildFromPacket(
case QuicWriteFrame::Type::PingFrame: {
const PingFrame& pingFrame = *frame.asPingFrame();
writeSuccess = writeFrame(pingFrame, builder_) != 0;
if (conn_.transportSettings.ptoPingFrames) {
notPureAck |= writeSuccess;
}
notPureAck |= writeSuccess;
break;
}
case QuicWriteFrame::Type::QuicSimpleFrame: {
Expand Down
25 changes: 0 additions & 25 deletions quic/codec/test/QuicPacketRebuilderTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -522,30 +522,6 @@ TEST_F(QuicPacketRebuilderTest, CloneCounter) {
EXPECT_EQ(1, conn.outstandings.numClonedPackets());
}

TEST_F(QuicPacketRebuilderTest, PurePingWontRebuild) {
ShortHeader shortHeader1(
ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
RegularQuicPacketBuilder regularBuilder(
kDefaultUDPSendPacketLen, std::move(shortHeader1), 0);
regularBuilder.encodePacketHeader();
PingFrame pingFrame;
writeFrame(pingFrame, regularBuilder);
auto packet = std::move(regularBuilder).buildPacket();
auto outstandingPacket = makeDummyOutstandingPacket(packet.packet, 50);
EXPECT_EQ(1, outstandingPacket.packet.frames.size());
QuicServerConnectionState conn(
FizzServerQuicHandshakeContext::Builder().build());
ShortHeader shortHeader2(
ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
RegularQuicPacketBuilder regularBuilder2(
kDefaultUDPSendPacketLen, std::move(shortHeader2), 0);
regularBuilder2.encodePacketHeader();
PacketRebuilder rebuilder(regularBuilder2, conn);
EXPECT_EQ(none, rebuilder.rebuildFromPacket(outstandingPacket));
EXPECT_FALSE(outstandingPacket.maybeClonedPacketIdentifier.has_value());
EXPECT_EQ(0, conn.outstandings.numClonedPackets());
}

TEST_F(QuicPacketRebuilderTest, PurePingWillRebuild) {
ShortHeader shortHeader1(
ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
Expand All @@ -559,7 +535,6 @@ TEST_F(QuicPacketRebuilderTest, PurePingWillRebuild) {
EXPECT_EQ(1, outstandingPacket.packet.frames.size());
QuicServerConnectionState conn(
FizzServerQuicHandshakeContext::Builder().build());
conn.transportSettings.ptoPingFrames = true;
ShortHeader shortHeader2(
ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
RegularQuicPacketBuilder regularBuilder2(
Expand Down
2 changes: 0 additions & 2 deletions quic/state/TransportSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -410,8 +410,6 @@ struct TransportSettings {
// receiving and then processing.
bool networkDataPerSocketRead{false};
bool cloneAllPacketsWithCryptoFrame{false};
// Whether we allow PTOing of packets with just PING frames.
bool ptoPingFrames{false};
// Always send multiple PTOs even if we don't have multiple packets.
bool alwaysPtoMultiple{false};
};
Expand Down

0 comments on commit 28748bf

Please sign in to comment.