diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index af1d81d24..726623b88 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -804,9 +805,9 @@ void updateConnection( break; } case QuicWriteFrame::Type::ImmediateAckFrame: { - // do not mark immediate acks as retranmittable. // turn off the immediate ack pending event. conn.pendingEvents.requestImmediateAck = false; + retransmittable = true; break; } default: @@ -1511,54 +1512,69 @@ uint64_t writeProbingDataToSocket( scheduler, connection, "CloningScheduler", aead.getCipherOverhead()); auto writeLoopBeginTime = Clock::now(); - auto written = writeConnectionDataToSocket( - sock, - connection, - srcConnId, - dstConnId, - builder, - pnSpace, - cloningScheduler, - connection.transportSettings.enableWritableBytesLimit - ? probePacketWritableBytes - : unlimitedWritableBytes, - probesToSend, - aead, - headerCipher, - version, - writeLoopBeginTime, - token) - .packetsWritten; - if (probesToSend && !written) { - // Fall back to send a ping: - connection.pendingEvents.sendPing = true; - auto pingScheduler = - std::move(FrameScheduler::Builder( - connection, encryptionLevel, pnSpace, "PingScheduler") - .pingFrames()) - .build(); - written += writeConnectionDataToSocket( - sock, - connection, - srcConnId, - dstConnId, - builder, - pnSpace, - pingScheduler, - connection.transportSettings.enableWritableBytesLimit - ? probePacketWritableBytes - : unlimitedWritableBytes, - probesToSend - written, - aead, - headerCipher, - version, - writeLoopBeginTime) - .packetsWritten; + // If we have the ability to draw an ACK for AppData, let's send a probe that + // is just an IMMEDIATE_ACK. Increase the number of probes to do so. + uint8_t dataProbesToSend = probesToSend; + if (probesToSend && canSendAckControlFrames(connection) && + encryptionLevel == EncryptionLevel::AppData) { + probesToSend = std::max(probesToSend, kPacketToSendForPTO); + dataProbesToSend = probesToSend - 1; } - VLOG_IF(10, written > 0) + auto probesWritten = writeConnectionDataToSocket( + sock, + connection, + srcConnId, + dstConnId, + builder, + pnSpace, + cloningScheduler, + connection.transportSettings.enableWritableBytesLimit + ? probePacketWritableBytes + : unlimitedWritableBytes, + dataProbesToSend, + aead, + headerCipher, + version, + writeLoopBeginTime, + token) + .packetsWritten; + if (probesWritten < probesToSend) { + // If we can use an IMMEDIATE_ACK, that's better than a PING. + auto probeSchedulerBuilder = FrameScheduler::Builder( + connection, encryptionLevel, pnSpace, "ProbeScheduler"); + // Might as well include some ACKs. + probeSchedulerBuilder.ackFrames(); + if (canSendAckControlFrames(connection) && + encryptionLevel == EncryptionLevel::AppData) { + requestPeerImmediateAck(connection); + probeSchedulerBuilder.immediateAckFrames(); + } else { + connection.pendingEvents.sendPing = true; + probeSchedulerBuilder.pingFrames(); + } + auto probeScheduler = std::move(probeSchedulerBuilder).build(); + probesWritten += writeConnectionDataToSocket( + sock, + connection, + srcConnId, + dstConnId, + builder, + pnSpace, + probeScheduler, + connection.transportSettings.enableWritableBytesLimit + ? probePacketWritableBytes + : unlimitedWritableBytes, + probesToSend - probesWritten, + aead, + headerCipher, + version, + writeLoopBeginTime) + .packetsWritten; + } + VLOG_IF(10, probesWritten > 0) << nodeToString(connection.nodeType) << " writing probes using scheduler=CloningScheduler " << connection; - return written; + return probesWritten; } WriteDataReason shouldWriteData(/*const*/ QuicConnectionStateBase& conn) { diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 335c08376..f0869adcf 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -3096,6 +3096,60 @@ TEST_F(QuicTransportFunctionsTest, WriteProbingOldData) { EXPECT_TRUE(folly::IOBufEqualTo()(pktBodyCaptured, secondBodyCaptured)); } +TEST_F(QuicTransportFunctionsTest, WriteProbingOldDataAckFreq) { + auto conn = createConn(); + conn->congestionController.reset(); + conn->peerMinAckDelay = 1ms; + EventBase evb; + auto socket = + std::make_unique>(&evb); + auto rawSocket = socket.get(); + EXPECT_CALL(*rawSocket, write(_, _)).WillRepeatedly(Return(100)); + auto capturingAead = std::make_unique(); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + auto buf = folly::IOBuf::copyBuffer("Where you wanna go"); + writeDataToQuicStream(*stream, buf->clone(), true); + + folly::IOBuf pktBodyCaptured; + EXPECT_CALL(*capturingAead, _inplaceEncrypt(_, _, _)) + .WillRepeatedly(Invoke([&](auto& buf, auto, auto) { + if (buf) { + pktBodyCaptured.prependChain(buf->clone()); + return buf->clone(); + } else { + return folly::IOBuf::create(0); + } + })); + EXPECT_EQ( + 2, + writeProbingDataToSocketForTest( + *rawSocket, *conn, 1, *aead, *headerCipher, getVersion(*conn))); + + auto immAck = + getFirstFrameInOutstandingPackets( + conn->outstandings.packets, QuicWriteFrame::Type::ImmediateAckFrame) + .asImmediateAckFrame(); + EXPECT_TRUE(immAck); + // Now we have no new data, let's probe again, and verify the same old data + // is sent. + folly::IOBuf secondBodyCaptured; + EXPECT_CALL(*capturingAead, _inplaceEncrypt(_, _, _)) + .WillRepeatedly(Invoke([&](auto& buf, auto, auto) { + if (buf) { + secondBodyCaptured.prependChain(buf->clone()); + return buf->clone(); + } else { + return folly::IOBuf::create(0); + } + })); + EXPECT_EQ( + 2, + writeProbingDataToSocketForTest( + *rawSocket, *conn, 1, *aead, *headerCipher, getVersion(*conn))); + // Verify two pacekts have the same body + EXPECT_TRUE(folly::IOBufEqualTo()(pktBodyCaptured, secondBodyCaptured)); +} + TEST_F(QuicTransportFunctionsTest, WriteProbingCryptoData) { QuicServerConnectionState conn( FizzServerQuicHandshakeContext::Builder().build()); @@ -3179,7 +3233,7 @@ TEST_F(QuicTransportFunctionsTest, WriteableBytesLimitedProbingCryptoData) { writeCryptoDataProbesToSocketForTest( *rawSocket, conn, probesToSend, *aead, *headerCipher, getVersion(conn)); - EXPECT_EQ(conn.numProbesWritableBytesLimited, 1); + EXPECT_EQ(conn.numProbesWritableBytesLimited, 2); EXPECT_LT(currentPacketSeqNum, conn.ackStates.initialAckState->nextPacketNum); EXPECT_FALSE(conn.outstandings.packets.empty()); EXPECT_TRUE(conn.pendingEvents.setLossDetectionAlarm); @@ -3237,6 +3291,34 @@ TEST_F(QuicTransportFunctionsTest, ProbingFallbackToPing) { EXPECT_EQ(1, conn->outstandings.packets.size()); } +TEST_F(QuicTransportFunctionsTest, ProbingFallbackToImmediateAck) { + auto conn = createConn(); + conn->peerMinAckDelay = 1ms; + EventBase evb; + auto socket = + std::make_unique>(&evb); + auto rawSocket = socket.get(); + EXPECT_CALL(*rawSocket, write(_, _)) + .Times(1) + .WillOnce(Invoke([&](const SocketAddress&, + const std::unique_ptr& iobuf) { + return iobuf->computeChainDataLength(); + })); + uint8_t probesToSend = 1; + EXPECT_EQ( + 1, + writeProbingDataToSocketForTest( + *rawSocket, + *conn, + probesToSend, + *aead, + *headerCipher, + getVersion(*conn))); + // Immediate Ack is the only non-retransmittable packet that will go into OP + // list + EXPECT_EQ(1, conn->outstandings.packets.size()); +} + TEST_F(QuicTransportFunctionsTest, TestCryptoWritingIsHandshakeInOutstanding) { auto conn = createConn(); auto cryptoStream = &conn->cryptoState->initialStream; @@ -4136,6 +4218,60 @@ TEST_F(QuicTransportFunctionsTest, ProbeWriteNewFunctionalFrames) { conn->outstandings.packets[1].packet.frames[0].type()); } +TEST_F(QuicTransportFunctionsTest, ProbeWriteNewFunctionalFramesAckFreq) { + auto conn = createConn(); + conn->udpSendPacketLen = 1200; + conn->peerMinAckDelay = 1ms; + EventBase evb; + auto sock = std::make_unique>(&evb); + auto rawSocket = sock.get(); + + EXPECT_CALL(*rawSocket, write(_, _)) + .WillRepeatedly(Invoke([&](const SocketAddress&, + const std::unique_ptr& iobuf) { + return iobuf->computeChainDataLength(); + })); + + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + auto buf = folly::IOBuf::copyBuffer("Drug facts"); + writeDataToQuicStream(*stream, buf->clone(), true); + writeQuicDataToSocket( + *rawSocket, + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + *aead, + *headerCipher, + getVersion(*conn), + conn->transportSettings.writeConnectionDataPacketsLimit); + ASSERT_EQ(1, stream->retransmissionBuffer.size()); + + conn->pendingEvents.numProbePackets[PacketNumberSpace::AppData] = 1; + conn->flowControlState.windowSize *= 2; + conn->flowControlState.timeOfLastFlowControlUpdate = Clock::now() - 20s; + maybeSendConnWindowUpdate(*conn, Clock::now()); + writeQuicDataToSocket( + *rawSocket, + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + *aead, + *headerCipher, + getVersion(*conn), + 1 /* limit to 1 packet */); + EXPECT_EQ(3, conn->outstandings.packets.size()); + auto packet = stripPaddingFrames(conn->outstandings.packets[1].packet); + EXPECT_EQ(1, packet.frames.size()); + EXPECT_EQ( + QuicWriteFrame::Type::MaxDataFrame, + conn->outstandings.packets[1].packet.frames[0].type()); + packet = stripPaddingFrames(conn->outstandings.packets[2].packet); + EXPECT_EQ(1, packet.frames.size()); + EXPECT_EQ( + QuicWriteFrame::Type::ImmediateAckFrame, + conn->outstandings.packets[2].packet.frames[0].type()); +} + TEST_F(QuicTransportFunctionsTest, WriteWithInplaceBuilder) { auto conn = createConn(); conn->transportSettings.dataPathType = DataPathType::ContinuousMemory;