Skip to content

Commit

Permalink
Make the AckState for Initial/Handshake a unique_ptr
Browse files Browse the repository at this point in the history 
Summary:
We don't need to carry these states after the handshake is confirmed, so make them pointers instead. This will facilitate adding a structure to the AckState for tracking duplicate packets.

(Note: this ignores all push blocking failures!)

Reviewed By: hanidamlaj

Differential Revision: D41626895

fbshipit-source-id: d8ac960b3672b9bb9adaaececa53a1203ec801e0
  • Loading branch information
@facebook-github-bot
Matt Joras authored and facebook-github-bot committed Dec 20, 2022
1 parent 1a41bc7 commit 1275798
Show file tree
Hide file tree
Showing 20 changed files with 281 additions and 217 deletions.
8 changes: 6 additions & 2 deletions quic/api/QuicTransportBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,12 @@ void QuicTransportBase::closeImpl(
conn_->pendingEvents = QuicConnectionStateBase::PendingEvents();
conn_->streamManager->clearActionable();
conn_->streamManager->clearWritable();
conn_->ackStates.initialAckState.acks.clear();
conn_->ackStates.handshakeAckState.acks.clear();
if (conn_->ackStates.initialAckState) {
conn_->ackStates.initialAckState->acks.clear();
}
if (conn_->ackStates.handshakeAckState) {
conn_->ackStates.handshakeAckState->acks.clear();
}
conn_->ackStates.appDataAckState.acks.clear();

if (transportReadyNotified_) {
Expand Down
64 changes: 39 additions & 25 deletions quic/api/QuicTransportFunctions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,15 @@ std::string largestAckScheduledToString(
const quic::QuicConnectionStateBase& conn) noexcept {
return folly::to<std::string>(
"[",
optionalToString(conn.ackStates.initialAckState.largestAckScheduled),
optionalToString(
conn.ackStates.initialAckState
? conn.ackStates.initialAckState->largestAckScheduled
: folly::none),
",",
optionalToString(conn.ackStates.handshakeAckState.largestAckScheduled),
optionalToString(
conn.ackStates.handshakeAckState
? conn.ackStates.handshakeAckState->largestAckScheduled
: folly::none),
",",
optionalToString(conn.ackStates.appDataAckState.largestAckScheduled),
"]");
Expand All @@ -61,35 +67,20 @@ std::string largestAckToSendToString(
const quic::QuicConnectionStateBase& conn) noexcept {
return folly::to<std::string>(
"[",
optionalToString(largestAckToSend(conn.ackStates.initialAckState)),
optionalToString(
conn.ackStates.initialAckState
? largestAckToSend(*conn.ackStates.initialAckState)
: folly::none),
",",
optionalToString(largestAckToSend(conn.ackStates.handshakeAckState)),
optionalToString(
conn.ackStates.handshakeAckState
? largestAckToSend(*conn.ackStates.handshakeAckState)
: folly::none),
",",
optionalToString(largestAckToSend(conn.ackStates.appDataAckState)),
"]");
}

bool toWriteInitialAcks(const quic::QuicConnectionStateBase& conn) {
return (
conn.initialWriteCipher &&
hasAcksToSchedule(conn.ackStates.initialAckState) &&
conn.ackStates.initialAckState.needsToSendAckImmediately);
}

bool toWriteHandshakeAcks(const quic::QuicConnectionStateBase& conn) {
return (
conn.handshakeWriteCipher &&
hasAcksToSchedule(conn.ackStates.handshakeAckState) &&
conn.ackStates.handshakeAckState.needsToSendAckImmediately);
}

bool toWriteAppDataAcks(const quic::QuicConnectionStateBase& conn) {
return (
conn.oneRttWriteCipher &&
hasAcksToSchedule(conn.ackStates.appDataAckState) &&
conn.ackStates.appDataAckState.needsToSendAckImmediately);
}

using namespace quic;

/**
Expand Down Expand Up @@ -1796,11 +1787,13 @@ void handshakeConfirmed(QuicConnectionStateBase& conn) {
conn.readCodec->setInitialReadCipher(nullptr);
conn.readCodec->setInitialHeaderCipher(nullptr);
implicitAckCryptoStream(conn, EncryptionLevel::Initial);
conn.ackStates.initialAckState.reset();
conn.handshakeWriteCipher.reset();
conn.handshakeWriteHeaderCipher.reset();
conn.readCodec->setHandshakeReadCipher(nullptr);
conn.readCodec->setHandshakeHeaderCipher(nullptr);
implicitAckCryptoStream(conn, EncryptionLevel::Handshake);
conn.ackStates.handshakeAckState.reset();
}

bool hasInitialOrHandshakeCiphers(QuicConnectionStateBase& conn) {
Expand Down Expand Up @@ -1839,4 +1832,25 @@ bool setCustomTransportParameter(
return true;
}

bool toWriteInitialAcks(const quic::QuicConnectionStateBase& conn) {
return (
conn.initialWriteCipher && conn.ackStates.initialAckState &&
hasAcksToSchedule(*conn.ackStates.initialAckState) &&
conn.ackStates.initialAckState->needsToSendAckImmediately);
}

bool toWriteHandshakeAcks(const quic::QuicConnectionStateBase& conn) {
return (
conn.handshakeWriteCipher && conn.ackStates.handshakeAckState &&
hasAcksToSchedule(*conn.ackStates.handshakeAckState) &&
conn.ackStates.handshakeAckState->needsToSendAckImmediately);
}

bool toWriteAppDataAcks(const quic::QuicConnectionStateBase& conn) {
return (
conn.oneRttWriteCipher &&
hasAcksToSchedule(conn.ackStates.appDataAckState) &&
conn.ackStates.appDataAckState.needsToSendAckImmediately);
}

} // namespace quic
4 changes: 4 additions & 0 deletions quic/api/QuicTransportFunctions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,4 +344,8 @@ bool setCustomTransportParameter(
std::unique_ptr<CustomTransportParameter> customParam,
std::vector<TransportParameter>& customTransportParameters);

bool toWriteInitialAcks(const quic::QuicConnectionStateBase& conn);
bool toWriteHandshakeAcks(const quic::QuicConnectionStateBase& conn);
bool toWriteAppDataAcks(const quic::QuicConnectionStateBase& conn);

} // namespace quic
94 changes: 47 additions & 47 deletions quic/api/test/QuicPacketSchedulerTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoPaddingInitialPacket) {
RegularQuicPacketBuilder builder(
conn.udpSendPacketLen,
std::move(longHeader),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));
FrameScheduler cryptoOnlyScheduler =
std::move(
FrameScheduler::Builder(
Expand Down Expand Up @@ -207,10 +207,10 @@ TEST_F(QuicPacketSchedulerTest, PaddingInitialPureAcks) {
RegularQuicPacketBuilder builder(
conn.udpSendPacketLen,
std::move(longHeader),
conn.ackStates.handshakeAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState.largestRecvdPacketTime = Clock::now();
conn.ackStates.initialAckState.needsToSendAckImmediately = true;
conn.ackStates.initialAckState.acks.insert(10);
conn.ackStates.handshakeAckState->largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestRecvdPacketTime = Clock::now();
conn.ackStates.initialAckState->needsToSendAckImmediately = true;
conn.ackStates.initialAckState->acks.insert(10);
FrameScheduler acksOnlyScheduler =
std::move(
FrameScheduler::Builder(
Expand Down Expand Up @@ -241,10 +241,10 @@ TEST_F(QuicPacketSchedulerTest, InitialPaddingDoesNotUseWrapper) {
RegularQuicPacketBuilder builder(
conn.udpSendPacketLen,
std::move(longHeader),
conn.ackStates.handshakeAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState.largestRecvdPacketTime = Clock::now();
conn.ackStates.initialAckState.needsToSendAckImmediately = true;
conn.ackStates.initialAckState.acks.insert(10);
conn.ackStates.handshakeAckState->largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestRecvdPacketTime = Clock::now();
conn.ackStates.initialAckState->needsToSendAckImmediately = true;
conn.ackStates.initialAckState->acks.insert(10);
FrameScheduler acksOnlyScheduler =
std::move(
FrameScheduler::Builder(
Expand Down Expand Up @@ -275,7 +275,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoServerInitialPadded) {
RegularQuicPacketBuilder builder1(
conn.udpSendPacketLen,
std::move(longHeader1),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));
FrameScheduler scheduler =
std::move(
FrameScheduler::Builder(
Expand Down Expand Up @@ -308,7 +308,7 @@ TEST_F(QuicPacketSchedulerTest, PadTwoInitialPackets) {
RegularQuicPacketBuilder builder1(
conn.udpSendPacketLen,
std::move(longHeader1),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));
FrameScheduler scheduler =
std::move(
FrameScheduler::Builder(
Expand Down Expand Up @@ -336,7 +336,7 @@ TEST_F(QuicPacketSchedulerTest, PadTwoInitialPackets) {
RegularQuicPacketBuilder builder2(
conn.udpSendPacketLen,
std::move(longHeader2),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));
writeDataToQuicStream(
conn.cryptoState->initialStream, folly::IOBuf::copyBuffer("shlo again"));
auto result2 = scheduler.scheduleFramesForPacket(
Expand All @@ -359,7 +359,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoPaddingRetransmissionClientInitial) {
RegularQuicPacketBuilder builder(
conn.udpSendPacketLen,
std::move(longHeader),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));
FrameScheduler scheduler =
std::move(
FrameScheduler::Builder(
Expand Down Expand Up @@ -391,7 +391,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoSchedulerOnlySingleLossFits) {
RegularQuicPacketBuilder builder(
conn.udpSendPacketLen,
std::move(longHeader),
conn.ackStates.handshakeAckState.largestAckedByPeer.value_or(0));
conn.ackStates.handshakeAckState->largestAckedByPeer.value_or(0));
builder.encodePacketHeader();
PacketBuilderWrapper builderWrapper(builder, 13);
CryptoStreamScheduler scheduler(
Expand Down Expand Up @@ -419,8 +419,8 @@ TEST_F(QuicPacketSchedulerTest, CryptoWritePartialLossBuffer) {
RegularQuicPacketBuilder builder(
25,
std::move(longHeader),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(
conn.ackStates.initialAckState.nextPacketNum));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(
conn.ackStates.initialAckState->nextPacketNum));
FrameScheduler cryptoOnlyScheduler =
std::move(
FrameScheduler::Builder(
Expand Down Expand Up @@ -712,7 +712,7 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) {
RegularQuicPacketBuilder builder(
conn.udpSendPacketLen,
std::move(header),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));
auto result = cloningScheduler.scheduleFramesForPacket(
std::move(builder), kDefaultUDPSendPacketLen);
EXPECT_TRUE(result.packetEvent.has_value() && result.packet.has_value());
Expand Down Expand Up @@ -767,8 +767,8 @@ TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasHandshakeDataAndAcks) {
WriteCryptoFrame(0, 4));

// Make it look like we received some acks from the peer.
conn.ackStates.handshakeAckState.acks.insert(10);
conn.ackStates.handshakeAckState.largestRecvdPacketTime = Clock::now();
conn.ackStates.handshakeAckState->acks.insert(10);
conn.ackStates.handshakeAckState->largestRecvdPacketTime = Clock::now();

// Create cloning scheduler.
CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0);
Expand Down Expand Up @@ -1184,19 +1184,19 @@ class AckSchedulingTest : public TestWithParam<PacketNumberSpace> {};
TEST_F(QuicPacketSchedulerTest, AckStateHasAcksToSchedule) {
QuicClientConnectionState conn(
FizzClientQuicHandshakeContext::Builder().build());
EXPECT_FALSE(hasAcksToSchedule(conn.ackStates.initialAckState));
EXPECT_FALSE(hasAcksToSchedule(conn.ackStates.handshakeAckState));
EXPECT_FALSE(hasAcksToSchedule(*conn.ackStates.initialAckState));
EXPECT_FALSE(hasAcksToSchedule(*conn.ackStates.handshakeAckState));
EXPECT_FALSE(hasAcksToSchedule(conn.ackStates.appDataAckState));

conn.ackStates.initialAckState.acks.insert(0, 100);
EXPECT_TRUE(hasAcksToSchedule(conn.ackStates.initialAckState));
conn.ackStates.initialAckState->acks.insert(0, 100);
EXPECT_TRUE(hasAcksToSchedule(*conn.ackStates.initialAckState));

conn.ackStates.handshakeAckState.acks.insert(0, 100);
conn.ackStates.handshakeAckState.largestAckScheduled = 200;
EXPECT_FALSE(hasAcksToSchedule(conn.ackStates.handshakeAckState));
conn.ackStates.handshakeAckState->acks.insert(0, 100);
conn.ackStates.handshakeAckState->largestAckScheduled = 200;
EXPECT_FALSE(hasAcksToSchedule(*conn.ackStates.handshakeAckState));

conn.ackStates.handshakeAckState.largestAckScheduled = folly::none;
EXPECT_TRUE(hasAcksToSchedule(conn.ackStates.handshakeAckState));
conn.ackStates.handshakeAckState->largestAckScheduled = folly::none;
EXPECT_TRUE(hasAcksToSchedule(*conn.ackStates.handshakeAckState));
}

TEST_F(QuicPacketSchedulerTest, AckSchedulerHasAcksToSchedule) {
Expand All @@ -1212,30 +1212,30 @@ TEST_F(QuicPacketSchedulerTest, AckSchedulerHasAcksToSchedule) {
EXPECT_FALSE(handshakeAckScheduler.hasPendingAcks());
EXPECT_FALSE(appDataAckScheduler.hasPendingAcks());

conn.ackStates.initialAckState.acks.insert(0, 100);
conn.ackStates.initialAckState->acks.insert(0, 100);
EXPECT_TRUE(initialAckScheduler.hasPendingAcks());

conn.ackStates.handshakeAckState.acks.insert(0, 100);
conn.ackStates.handshakeAckState.largestAckScheduled = 200;
conn.ackStates.handshakeAckState->acks.insert(0, 100);
conn.ackStates.handshakeAckState->largestAckScheduled = 200;
EXPECT_FALSE(handshakeAckScheduler.hasPendingAcks());

conn.ackStates.handshakeAckState.largestAckScheduled = folly::none;
conn.ackStates.handshakeAckState->largestAckScheduled = folly::none;
EXPECT_TRUE(handshakeAckScheduler.hasPendingAcks());
}

TEST_F(QuicPacketSchedulerTest, LargestAckToSend) {
QuicClientConnectionState conn(
FizzClientQuicHandshakeContext::Builder().build());
EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.initialAckState));
EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.handshakeAckState));
EXPECT_EQ(folly::none, largestAckToSend(*conn.ackStates.initialAckState));
EXPECT_EQ(folly::none, largestAckToSend(*conn.ackStates.handshakeAckState));
EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.appDataAckState));

conn.ackStates.initialAckState.acks.insert(0, 50);
conn.ackStates.handshakeAckState.acks.insert(0, 50);
conn.ackStates.handshakeAckState.acks.insert(75, 150);
conn.ackStates.initialAckState->acks.insert(0, 50);
conn.ackStates.handshakeAckState->acks.insert(0, 50);
conn.ackStates.handshakeAckState->acks.insert(75, 150);

EXPECT_EQ(50, *largestAckToSend(conn.ackStates.initialAckState));
EXPECT_EQ(150, *largestAckToSend(conn.ackStates.handshakeAckState));
EXPECT_EQ(50, *largestAckToSend(*conn.ackStates.initialAckState));
EXPECT_EQ(150, *largestAckToSend(*conn.ackStates.handshakeAckState));
EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.appDataAckState));
}

Expand All @@ -1253,18 +1253,18 @@ TEST_F(QuicPacketSchedulerTest, NeedsToSendAckWithoutAcksAvailable) {
EXPECT_FALSE(handshakeAckScheduler.hasPendingAcks());
EXPECT_FALSE(appDataAckScheduler.hasPendingAcks());

conn.ackStates.initialAckState.needsToSendAckImmediately = true;
conn.ackStates.handshakeAckState.needsToSendAckImmediately = true;
conn.ackStates.initialAckState->needsToSendAckImmediately = true;
conn.ackStates.handshakeAckState->needsToSendAckImmediately = true;
conn.ackStates.appDataAckState.needsToSendAckImmediately = true;

conn.ackStates.initialAckState.acks.insert(0, 100);
conn.ackStates.initialAckState->acks.insert(0, 100);
EXPECT_TRUE(initialAckScheduler.hasPendingAcks());

conn.ackStates.handshakeAckState.acks.insert(0, 100);
conn.ackStates.handshakeAckState.largestAckScheduled = 200;
conn.ackStates.handshakeAckState->acks.insert(0, 100);
conn.ackStates.handshakeAckState->largestAckScheduled = 200;
EXPECT_FALSE(handshakeAckScheduler.hasPendingAcks());

conn.ackStates.handshakeAckState.largestAckScheduled = folly::none;
conn.ackStates.handshakeAckState->largestAckScheduled = folly::none;
EXPECT_TRUE(handshakeAckScheduler.hasPendingAcks());
}

Expand Down Expand Up @@ -2380,7 +2380,7 @@ TEST_F(QuicPacketSchedulerTest, ImmediateAckFrameSchedulerOnRequest) {
RegularQuicPacketBuilder builder(
conn.udpSendPacketLen,
std::move(longHeader),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));

FrameScheduler immediateAckOnlyScheduler =
std::move(
Expand Down Expand Up @@ -2416,7 +2416,7 @@ TEST_F(QuicPacketSchedulerTest, ImmediateAckFrameSchedulerNotRequested) {
RegularQuicPacketBuilder builder(
conn.udpSendPacketLen,
std::move(longHeader),
conn.ackStates.initialAckState.largestAckedByPeer.value_or(0));
conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));

FrameScheduler immediateAckOnlyScheduler =
std::move(
Expand Down
2 changes: 1 addition & 1 deletion quic/api/test/QuicTransportBaseTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ TEST_P(QuicTransportImplTestBase, WriteAckPacketUnsetsLooper) {
pktHasCryptoData,
Clock::now());
ASSERT_TRUE(transport->transportConn->ackStates.initialAckState
.needsToSendAckImmediately);
->needsToSendAckImmediately);
// Trigger the loop callback. This will trigger writes and we assume this will
// write the acks since we have nothing else to write.
transport->writeLooper()->runLoopCallback();
Expand Down
Loading

0 comments on commit 1275798

Please sign in to comment.