lib/mvfst
1
0
Fork 0
mirror of https://github.com/facebookincubator/mvfst.git synced 2025-08-08 09:42:06 +03:00
Code Activity

PingFrame is not a simple frame

Browse Source 
Summary:
The problem with Ping being a simple frame:
(1) All SimpleFrames are in the same scheduler. So sending ping means we may
also send other frames which can be problematic if we send in Initial or
Handshake space
(2) Ping isn't retranmisttable. But other Simple frames are. So we are
certainly setting this wrong when we send pure Ping packet today.

That being said, there are cases where we need to treat Ping as retransmittable.
One is when it comes to update ack state: If peer sends us Ping, we may want to
Ack early rather than late. so it makes sense to treat Ping as retransmittable.
Another place is insertion into OutstandingPackets list. When our API user sends
Ping, then also add a Ping timeout. Without adding pure Ping packets into OP list,
we won't be able to track the acks to our Pings.

Reviewed By: mjoras

Differential Revision: D21763935

fbshipit-source-id: a04e97b50cf4dd4e3974320a4d2cc16eda48eef9
This commit is contained in:
Yang Chi
2020-06-18 15:28:59 -07:00
committed by Facebook GitHub Bot
parent b8fef40c6d
commit 51b917b0b3
23 changed files with 170 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
quic/api/test/QuicTransportFunctionsTest.cpp
View File

@@ -46,6 +46,7 @@ uint64_t writeProbingDataToSocketForTest(
*conn.clientConnectionId,
*conn.serverConnectionId,
ShortHeaderBuilder(),
EncryptionLevel::AppData,
PacketNumberSpace::AppData,
scheduler,
probesToSend,
@@ -75,6 +76,7 @@ void writeCryptoDataProbesToSocketForTest(
*conn.clientConnectionId,
*conn.serverConnectionId,
LongHeaderBuilder(type),
protectionTypeToEncryptionLevel(longHeaderTypeToProtectionType(type)),
LongHeader::typeToPacketNumberSpace(type),
scheduler,
probesToSend,
@@ -178,6 +180,17 @@ class QuicTransportFunctionsTest : public Test {
std::unique_ptr<MockQuicStats> transportInfoCb_;
};
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, folly::none, packet.packet, Clock::now(), 50);
EXPECT_EQ(1, conn->outstandings.packets.size());
// But it won't set loss detection alarm
EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm);
}
TEST_F(QuicTransportFunctionsTest, TestUpdateConnection) {
auto conn = createConn();
auto mockCongestionController =
@@ -1604,15 +1617,10 @@ TEST_F(QuicTransportFunctionsTest, ProbingNotFallbackToPingWhenNoQuota) {
TEST_F(QuicTransportFunctionsTest, ProbingFallbackToPing) {
auto conn = createConn();
auto mockCongestionController =
std::make_unique<NiceMock<MockCongestionController>>();
auto rawCongestionController = mockCongestionController.get();
conn->congestionController = std::move(mockCongestionController);
EventBase evb;
auto socket =
std::make_unique<NiceMock<folly::test::MockAsyncUDPSocket>>(&evb);
auto rawSocket = socket.get();
EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(1);
EXPECT_CALL(*rawSocket, write(_, _))
.Times(1)
.WillOnce(Invoke([&](const SocketAddress&,
@@ -1629,17 +1637,8 @@ TEST_F(QuicTransportFunctionsTest, ProbingFallbackToPing) {
*aead,
*headerCipher,
getVersion(*conn)));
// Ping is the only non-retransmittable packet that will go into OP list
EXPECT_EQ(1, conn->outstandings.packets.size());
EXPECT_EQ(1, conn->outstandings.packets[0].packet.frames.size());
EXPECT_EQ(
QuicWriteFrame::Type::QuicSimpleFrame_E,
conn->outstandings.packets[0].packet.frames[0].type());
EXPECT_EQ(
QuicSimpleFrame::Type::PingFrame_E,
conn->outstandings.packets[0]
.packet.frames[0]
.asQuicSimpleFrame()
->type());
}
TEST_F(QuicTransportFunctionsTest, TestCryptoWritingIsHandshakeInOutstanding) {