Don't special case stream frames for opportunistic ACKing

Summary: This was added to not bundle ACKs with stream frames. Don't special case those and also disabling opportunistic ACKing for all write reasons.

Reviewed By: jbeshay

Differential Revision: D66514392

fbshipit-source-id: f2657a5c06ea8ae37b8c8eacd04c5a3b8ac75390

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) {

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(_, _, _))