diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index c6344b8a3..ef9372be9 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -186,11 +186,9 @@ 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. + // Only add ACK frames if we need to send an ACK. if (connection.transportSettings.opportunisticAcking || - toWriteAppDataAcks(connection) || - (hasNonAckDataToWrite(connection) != WriteDataReason::STREAM)) { + toWriteAppDataAcks(connection)) { schedulerBuilder.ackFrames(); } if (!exceptCryptoStream) { diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index d9cb81f24..8fdfc3f2a 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -2084,6 +2084,44 @@ TEST_F(QuicTransportTest, NoWritePendingAckIfHavingData) { EXPECT_EQ(3, ackState.numNonRxPacketsRecvd); } +TEST_F(QuicTransportTest, NoWritePendingAckIfHavingDataNonStream) { + auto& conn = transport_->getConnectionState(); + conn.transportSettings.opportunisticAcking = false; + auto streamId = transport_->createBidirectionalStream().value(); + 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(testing::WithArgs<1, 2>(Invoke(getTotalIovecLen))); + // We should write acks if there is data pending + conn.streamManager->queueWindowUpdate(streamId); + transport_->read(streamId, 0); + 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, none); + + auto pnSpace = packet.header.getPacketNumberSpace(); + auto ackState = getAckState(conn, pnSpace); + EXPECT_EQ(ackState.largestAckScheduled, none); + EXPECT_FALSE(ackState.needsToSendAckImmediately); + EXPECT_EQ(3, ackState.numNonRxPacketsRecvd); +} + TEST_F(QuicTransportTest, RstStream) { auto streamId = transport_->createBidirectionalStream().value(); EXPECT_CALL(*socket_, write(_, _, _))