diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index 422c3d8d4..0fcedcfad 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -3618,6 +3618,9 @@ QuicSocket::WriteResult QuicTransportBase::setDSRPacketizationRequestSender( return folly::makeUnexpected(LocalErrorCode::STREAM_CLOSED); } stream->dsrSender = std::move(sender); + // Default to disabling opportunistic ACKing for DSR since it causes extra + // writes and spurious losses. + conn_->transportSettings.opportunisticAcking = false; // Fow now, no appLimited or appIdle update here since we are not writing // either BufferMetas yet. The first BufferMeta write will update it. } catch (const QuicTransportException& ex) { diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 312c516fc..ec9669d36 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -177,7 +177,6 @@ WriteQuicDataResult writeQuicDataToSocketImpl( PacketNumberSpace::AppData, exceptCryptoStream ? "FrameSchedulerWithoutCrypto" : "FrameScheduler") .streamFrames() - .ackFrames() .resetFrames() .windowUpdateFrames() .blockedFrames() @@ -185,6 +184,13 @@ WriteQuicDataResult writeQuicDataToSocketImpl( .pingFrames() .datagramFrames() .immediateAckFrames(); + // Only add ACK frames if we need to send an ACK, or if the write reason isn't + // just streams. + if (connection.transportSettings.opportunisticAcking || + toWriteAppDataAcks(connection) || + (hasNonAckDataToWrite(connection) != WriteDataReason::STREAM)) { + schedulerBuilder.ackFrames(); + } if (!exceptCryptoStream) { schedulerBuilder.cryptoFrames(); } diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index a9a3b013c..8a907b73e 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -2025,6 +2025,43 @@ TEST_F(QuicTransportTest, WritePendingAckIfHavingData) { EXPECT_EQ(0, ackState.numNonRxPacketsRecvd); } +TEST_F(QuicTransportTest, NoWritePendingAckIfHavingData) { + auto& conn = transport_->getConnectionState(); + conn.transportSettings.opportunisticAcking = false; + auto streamId = transport_->createBidirectionalStream().value(); + auto buf = buildRandomInputData(20); + PacketNum start = 10; + PacketNum end = 15; + addAckStatesWithCurrentTimestamps(conn.ackStates.appDataAckState, start, end); + conn.ackStates.appDataAckState.needsToSendAckImmediately = false; + conn.ackStates.appDataAckState.numNonRxPacketsRecvd = 3; + EXPECT_CALL(*socket_, write(_, _)).WillOnce(Invoke(bufLength)); + // We should write acks if there is data pending + transport_->writeChain(streamId, buf->clone(), true); + loopForWrites(); + EXPECT_EQ(conn.outstandings.packets.size(), 1); + auto& packet = + getFirstOutstandingPacket(conn, PacketNumberSpace::AppData)->packet; + EXPECT_GE(packet.frames.size(), 1); + + bool ackFound = false; + for (auto& frame : packet.frames) { + auto ackFrame = frame.asWriteAckFrame(); + if (!ackFrame) { + continue; + } + ackFound = true; + } + EXPECT_FALSE(ackFound); + EXPECT_EQ(conn.ackStates.appDataAckState.largestAckScheduled, folly::none); + + auto pnSpace = packet.header.getPacketNumberSpace(); + auto ackState = getAckState(conn, pnSpace); + EXPECT_EQ(ackState.largestAckScheduled, folly::none); + EXPECT_FALSE(ackState.needsToSendAckImmediately); + EXPECT_EQ(3, ackState.numNonRxPacketsRecvd); +} + TEST_F(QuicTransportTest, RstStream) { auto streamId = transport_->createBidirectionalStream().value(); EXPECT_CALL(*socket_, write(_, _)).WillOnce(Invoke(bufLength)); diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 4d3c72aff..aacb798d8 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -274,6 +274,8 @@ struct TransportSettings { // How many times we will a schedule a stream to packets before moving onto // the next one in the queue. Only relevant for incremental priority. uint64_t priorityQueueWritesPerStream{1}; + // Whether to include ACKs whenever we have data to write and packets to ACK. + bool opportunisticAcking{true}; // Local configuration for ACK receive timestamps. //