diff --git a/quic/api/QuicPacketScheduler-inl.h b/quic/api/QuicPacketScheduler-inl.h index 48ddcdc22..8ccf0d49e 100644 --- a/quic/api/QuicPacketScheduler-inl.h +++ b/quic/api/QuicPacketScheduler-inl.h @@ -15,30 +15,6 @@ namespace quic { template inline folly::Optional AckScheduler::writeNextAcks( - PacketBuilderInterface& builder, - AckMode mode) { - switch (mode) { - case AckMode::Immediate: { - return writeAcksImpl(builder); - } - case AckMode::Pending: { - return writeAcksIfPending(builder); - } - } - folly::assume_unreachable(); -} - -template -inline folly::Optional AckScheduler::writeAcksIfPending( - PacketBuilderInterface& builder) { - if (ackState_.needsToSendAckImmediately) { - return writeAcksImpl(builder); - } - return folly::none; -} - -template -folly::Optional AckScheduler::writeAcksImpl( PacketBuilderInterface& builder) { // Use default ack delay for long headers. Usually long headers are sent // before crypto negotiation, so the peer might not know about the ack delay diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 01c8f6a76..a725ed4ab 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -125,7 +125,6 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( // We cannot return early if the writablyBytes dropps to 0 here, since pure // acks can skip writableBytes entirely. PacketBuilderWrapper wrapper(builder, writableBytes); - auto ackMode = hasImmediateData() ? AckMode::Immediate : AckMode::Pending; bool cryptoDataWritten = false; bool rstWritten = false; if (cryptoStreamScheduler_ && cryptoStreamScheduler_->hasData()) { @@ -134,18 +133,19 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( if (rstScheduler_ && rstScheduler_->hasPendingRsts()) { rstWritten = rstScheduler_->writeRsts(wrapper); } + // TODO: Long time ago we decided RST has higher priority than Acks. Why tho? if (ackScheduler_ && ackScheduler_->hasPendingAcks()) { if (cryptoDataWritten || rstWritten) { // If packet has non ack data, it is subject to congestion control. We // need to use the wrapper/ - ackScheduler_->writeNextAcks(wrapper, ackMode); + ackScheduler_->writeNextAcks(wrapper); } else { // If we start with writing acks, we will let the ack scheduler write // up to the full packet space. If the ack bytes exceeds the writable // bytes, this will be a pure ack packet and it will skip congestion // controller. Otherwise, we will give other schedulers an opportunity to // write up to writable bytes. - ackScheduler_->writeNextAcks(builder, ackMode); + ackScheduler_->writeNextAcks(builder); } } if (windowUpdateScheduler_ && diff --git a/quic/api/QuicPacketScheduler.h b/quic/api/QuicPacketScheduler.h index cab78a73b..37f883dd5 100644 --- a/quic/api/QuicPacketScheduler.h +++ b/quic/api/QuicPacketScheduler.h @@ -60,9 +60,6 @@ class QuicPacketScheduler { virtual std::string name() const = 0; }; -// A tag to denote how we should schedule ack in this packet. -enum class AckMode { Pending, Immediate }; - class RetransmissionScheduler { public: explicit RetransmissionScheduler(const QuicConnectionStateBase& conn); @@ -198,23 +195,11 @@ class AckScheduler { template folly::Optional writeNextAcks( - PacketBuilderInterface& builder, - AckMode mode); + PacketBuilderInterface& builder); bool hasPendingAcks() const; private: - /* Write out pending acks if needsToSendAckImmeidately in the connection's - * pendingEvent is true. - */ - template - folly::Optional writeAcksIfPending( - PacketBuilderInterface& builder); - - // Write out pending acks - template - folly::Optional writeAcksImpl(PacketBuilderInterface& builder); - const QuicConnectionStateBase& conn_; const AckState& ackState_; }; diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 10fd27eca..7486d6b37 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -1856,10 +1856,11 @@ TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteCipherAndAckStateMatch) { TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteNoImmediateAcks) { auto conn = createConn(); conn->oneRttWriteCipher = test::createNoOpAead(); - conn->ackStates.initialAckState.needsToSendAckImmediately = false; - conn->ackStates.handshakeAckState.needsToSendAckImmediately = false; + conn->ackStates.appDataAckState.acks.insert(0, 100); conn->ackStates.appDataAckState.needsToSendAckImmediately = false; EXPECT_FALSE(hasAckDataToWrite(*conn)); + conn->ackStates.appDataAckState.needsToSendAckImmediately = true; + EXPECT_TRUE(hasAckDataToWrite(*conn)); } TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteNoAcksScheduled) { diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index b2685bd6e..e6f10281a 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -800,26 +800,6 @@ TEST_F(QuicTransportTest, WriteImmediateAcks) { EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(conn)); } -TEST_F(QuicTransportTest, NotWriteAcksIfNoData) { - auto& conn = transport_->getConnectionState(); - - addAckStatesWithCurrentTimestamps(conn.ackStates.appDataAckState, 0, 100); - conn.ackStates.appDataAckState.needsToSendAckImmediately = false; - conn.ackStates.appDataAckState.numNonRxPacketsRecvd = 3; - // Should not write ack blocks if there is only ack to write - EXPECT_EQ( - 0, - writeQuicDataToSocket( - *socket_, - conn, - *conn.clientConnectionId, - *conn.serverConnectionId, - *aead_, - *headerCipher_, - transport_->getVersion(), - conn.transportSettings.writeConnectionDataPacketsLimit)); -} - TEST_F(QuicTransportTest, WritePendingAckIfHavingData) { auto& conn = transport_->getConnectionState(); auto streamId = transport_->createBidirectionalStream().value();