diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 5125dba27..c2098d53d 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -44,33 +44,6 @@ bool hasAcksToSchedule(const AckState& ackState) { return *largestAckSend > *(ackState.largestAckScheduled); } -bool neverWrittenAcksBefore(const QuicConnectionStateBase& conn) { - return ( - !conn.ackStates.initialAckState.largestAckScheduled && - !conn.ackStates.handshakeAckState.largestAckScheduled && - !conn.ackStates.appDataAckState.largestAckScheduled); -} - -bool hasAcksToSchedule(const QuicConnectionStateBase& conn) { - bool initialSpaceHasAcks = hasAcksToSchedule(conn.ackStates.initialAckState); - bool handshakeSpaceHasAcks = - hasAcksToSchedule(conn.ackStates.handshakeAckState); - bool appDataSpaceHasAcks = hasAcksToSchedule(conn.ackStates.appDataAckState); - bool cannotWriteInitialAcks = - !conn.initialWriteCipher || !initialSpaceHasAcks; - bool cannotWriteHandshakeAcks = - !conn.handshakeWriteCipher || !handshakeSpaceHasAcks; - bool cannotWriteAppDataAcks = !conn.oneRttWriteCipher || !appDataSpaceHasAcks; - if (cannotWriteInitialAcks && cannotWriteHandshakeAcks && - cannotWriteAppDataAcks) { - return false; - } - if (neverWrittenAcksBefore(conn)) { - return true; - } - return initialSpaceHasAcks || handshakeSpaceHasAcks || appDataSpaceHasAcks; -} - folly::Optional largestAckToSend(const AckState& ackState) { if (ackState.acks.empty()) { return folly::none; diff --git a/quic/api/QuicPacketScheduler.h b/quic/api/QuicPacketScheduler.h index 928ea0cc1..e013b9edf 100644 --- a/quic/api/QuicPacketScheduler.h +++ b/quic/api/QuicPacketScheduler.h @@ -215,9 +215,7 @@ class AckScheduler { * Returns whether or not the Ack scheduler has acks to schedule. This does not * tell you when the ACKs can be written. */ -bool hasAcksToSchedule(const QuicConnectionStateBase& conn); bool hasAcksToSchedule(const AckState& ackState); -bool neverWrittenAcksBefore(const QuicConnectionStateBase& conn); /** * Returns the largest packet received which needs to be acked. diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 2a3c60740..0dcc045e4 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -56,6 +56,28 @@ std::string largestAckToSendToString( 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); +} + } // namespace namespace quic { @@ -1019,16 +1041,17 @@ bool hasAckDataToWrite(const QuicConnectionStateBase& conn) { // have an immediate need to schedule the acks then we need to wait till we // satisfy a condition where there is immediate need, so we shouldn't // consider the acks to be writable. - if (needsToSendAckImmediately(conn) && hasAcksToSchedule(conn)) { - VLOG(10) << nodeToString(conn.nodeType) - << " needs write because of acks largestAck=" - << largestAckToSendToString(conn) - << " largestSentAck=" << largestAckScheduledToString(conn) - << " ackTimeoutSet=" << conn.pendingEvents.scheduleAckTimeout - << " " << conn; - return true; - } - return false; + bool writeAcks = + (toWriteInitialAcks(conn) || toWriteHandshakeAcks(conn) || + toWriteAppDataAcks(conn)); + VLOG_IF(10, writeAcks) << nodeToString(conn.nodeType) + << " needs write because of acks largestAck=" + << largestAckToSendToString(conn) << " largestSentAck=" + << largestAckScheduledToString(conn) + << " ackTimeoutSet=" + << conn.pendingEvents.scheduleAckTimeout << " " + << conn; + return writeAcks; } bool hasNonAckDataToWrite(const QuicConnectionStateBase& conn) { diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 10fd515a0..ae5cb6664 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -666,19 +666,6 @@ TEST_F(QuicPacketSchedulerTest, AckSchedulerHasAcksToSchedule) { EXPECT_TRUE(handshakeAckScheduler.hasPendingAcks()); } -TEST_F(QuicPacketSchedulerTest, ConnHasAcksToSchedule) { - QuicClientConnectionState conn; - EXPECT_FALSE(hasAcksToSchedule(conn)); - conn.ackStates.initialAckState.acks.insert(0, 100); - EXPECT_FALSE(hasAcksToSchedule(conn)); - conn.initialWriteCipher = createNoOpAead(); - EXPECT_TRUE(hasAcksToSchedule(conn)); - conn.ackStates.initialAckState.acks.clear(); - EXPECT_FALSE(hasAcksToSchedule(conn)); - conn.oneRttWriteCipher = createNoOpAead(); - EXPECT_FALSE(hasAcksToSchedule(conn)); -} - TEST_F(QuicPacketSchedulerTest, LargestAckToSend) { QuicClientConnectionState conn; EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.initialAckState)); diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index dea8bf7a1..14c86991b 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -1382,6 +1382,19 @@ TEST_F(QuicTransportFunctionsTest, HasAckDataToWrite) { EXPECT_TRUE(hasAckDataToWrite(*conn)); } +TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteMismatch) { + // When one ack space has needsToSendAckImmediately = true and another has + // hasAckToSchedule = true, but no ack space has both of them to true, we + // should not send. + auto conn = createConn(); + EXPECT_FALSE(hasAckDataToWrite(*conn)); + conn->ackStates.initialAckState.needsToSendAckImmediately = true; + EXPECT_FALSE(hasAckDataToWrite(*conn)); + conn->ackStates.handshakeAckState.acks.insert(0, 10); + conn->handshakeWriteCipher = test::createNoOpAead(); + EXPECT_FALSE(hasAckDataToWrite(*conn)); +} + TEST_F(QuicTransportFunctionsTest, HasCryptoDataToWrite) { auto conn = createConn(); conn->cryptoState->initialStream.lossBuffer.emplace_back( diff --git a/quic/state/QuicStateFunctions.cpp b/quic/state/QuicStateFunctions.cpp index 6086f967d..c2ea9fbe7 100644 --- a/quic/state/QuicStateFunctions.cpp +++ b/quic/state/QuicStateFunctions.cpp @@ -165,12 +165,6 @@ AckStateVersion currentAckStateVersion( conn.ackStates.appDataAckState.acks.insertVersion()); } -bool needsToSendAckImmediately(const QuicConnectionStateBase& conn) noexcept { - return conn.ackStates.initialAckState.needsToSendAckImmediately || - conn.ackStates.handshakeAckState.needsToSendAckImmediately || - conn.ackStates.appDataAckState.needsToSendAckImmediately; -} - PacketNum getNextPacketNum( const QuicConnectionStateBase& conn, PacketNumberSpace pnSpace) noexcept { diff --git a/quic/state/QuicStateFunctions.h b/quic/state/QuicStateFunctions.h index 5b879a33a..387b1c512 100644 --- a/quic/state/QuicStateFunctions.h +++ b/quic/state/QuicStateFunctions.h @@ -65,8 +65,6 @@ const AckState& getAckState( AckStateVersion currentAckStateVersion( const QuicConnectionStateBase& conn) noexcept; -bool needsToSendAckImmediately(const QuicConnectionStateBase& conn) noexcept; - PacketNum getNextPacketNum( const QuicConnectionStateBase& conn, PacketNumberSpace pnSpace) noexcept;