From d31f2f7f16a540edc02aca253460ccfc8fe0b0cf Mon Sep 17 00:00:00 2001 From: Matt Joras Date: Mon, 12 Jun 2023 17:02:16 -0700 Subject: [PATCH] Fix bytesWritten return Summary: Turns out these functions have been returning 0 for bytes written for a while. Nothing was using them (yet), so there wasn't any functional breakage. Reviewed By: kvtsoy Differential Revision: D46653336 fbshipit-source-id: 765b3363c1fc0729e8239d1293ddcc4ae4bb9c79 --- quic/api/QuicTransportFunctions.cpp | 116 ++++++++++--------- quic/api/QuicTransportFunctions.h | 2 +- quic/api/test/QuicTransportFunctionsTest.cpp | 46 +++++--- 3 files changed, 96 insertions(+), 68 deletions(-) diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 95cf5f1a8..d2a053aeb 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -129,6 +129,7 @@ WriteQuicDataResult writeQuicDataToSocketImpl( WriteQuicDataResult result; auto& packetsWritten = result.packetsWritten; auto& probesWritten = result.probesWritten; + auto& bytesWritten = result.bytesWritten; auto& numProbePackets = connection.pendingEvents.numProbePackets[PacketNumberSpace::AppData]; if (numProbePackets) { @@ -149,7 +150,7 @@ WriteQuicDataResult writeQuicDataToSocketImpl( probeSchedulerBuilder.cryptoFrames(); } auto probeScheduler = std::move(probeSchedulerBuilder).build(); - probesWritten = writeProbingDataToSocket( + auto probeResult = writeProbingDataToSocket( sock, connection, srcConnId, @@ -162,6 +163,8 @@ WriteQuicDataResult writeQuicDataToSocketImpl( aead, headerCipher, version); + probesWritten = probeResult.probesWritten; + bytesWritten += probeResult.bytesWritten; // We only get one chance to write out the probes. numProbePackets = 0; packetLimit = @@ -186,21 +189,22 @@ WriteQuicDataResult writeQuicDataToSocketImpl( schedulerBuilder.cryptoFrames(); } FrameScheduler scheduler = std::move(schedulerBuilder).build(); - packetsWritten = writeConnectionDataToSocket( - sock, - connection, - srcConnId, - dstConnId, - std::move(builder), - PacketNumberSpace::AppData, - scheduler, - congestionControlWritableBytes, - packetLimit, - aead, - headerCipher, - version, - writeLoopBeginTime) - .packetsWritten; + auto connectionDataResult = writeConnectionDataToSocket( + sock, + connection, + srcConnId, + dstConnId, + std::move(builder), + PacketNumberSpace::AppData, + scheduler, + congestionControlWritableBytes, + packetLimit, + aead, + headerCipher, + version, + writeLoopBeginTime); + packetsWritten += connectionDataResult.packetsWritten; + bytesWritten += connectionDataResult.bytesWritten; VLOG_IF(10, packetsWritten || probesWritten) << nodeToString(connection.nodeType) << " written data " << (exceptCryptoStream ? "without crypto data " : "") @@ -1022,7 +1026,7 @@ WriteQuicDataResult writeCryptoAndAckDataToSocket( .numProbePackets[LongHeader::typeToPacketNumberSpace(packetType)]; if (numProbePackets && (cryptoStream.retransmissionBuffer.size() || scheduler.hasData())) { - probesWritten = writeProbingDataToSocket( + auto probeResult = writeProbingDataToSocket( sock, connection, srcConnId, @@ -1036,6 +1040,8 @@ WriteQuicDataResult writeCryptoAndAckDataToSocket( headerCipher, version, token); + probesWritten += probeResult.probesWritten; + bytesWritten += probeResult.bytesWritten; } packetLimit = probesWritten > packetLimit ? 0 : (packetLimit - probesWritten); // Only get one chance to write probes. @@ -1491,7 +1497,7 @@ WriteQuicDataResult writeConnectionDataToSocket( return {ioBufBatch.getPktSent(), 0, bytesWritten}; } -uint64_t writeProbingDataToSocket( +WriteQuicDataResult writeProbingDataToSocket( folly::AsyncUDPSocket& sock, QuicConnectionStateBase& connection, const ConnectionId& srcConnId, @@ -1519,24 +1525,25 @@ uint64_t writeProbingDataToSocket( probesToSend = std::max(probesToSend, kPacketToSendForPTO); dataProbesToSend = probesToSend - 1; } - auto probesWritten = writeConnectionDataToSocket( - sock, - connection, - srcConnId, - dstConnId, - builder, - pnSpace, - cloningScheduler, - connection.transportSettings.enableWritableBytesLimit - ? probePacketWritableBytes - : unlimitedWritableBytes, - dataProbesToSend, - aead, - headerCipher, - version, - writeLoopBeginTime, - token) - .packetsWritten; + auto cloningResult = writeConnectionDataToSocket( + sock, + connection, + srcConnId, + dstConnId, + builder, + pnSpace, + cloningScheduler, + connection.transportSettings.enableWritableBytesLimit + ? probePacketWritableBytes + : unlimitedWritableBytes, + dataProbesToSend, + aead, + headerCipher, + version, + writeLoopBeginTime, + token); + auto probesWritten = cloningResult.packetsWritten; + auto bytesWritten = cloningResult.bytesWritten; if (probesWritten < probesToSend) { // If we can use an IMMEDIATE_ACK, that's better than a PING. auto probeSchedulerBuilder = FrameScheduler::Builder( @@ -1552,28 +1559,29 @@ uint64_t writeProbingDataToSocket( 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; + auto probingResult = writeConnectionDataToSocket( + sock, + connection, + srcConnId, + dstConnId, + builder, + pnSpace, + probeScheduler, + connection.transportSettings.enableWritableBytesLimit + ? probePacketWritableBytes + : unlimitedWritableBytes, + probesToSend - probesWritten, + aead, + headerCipher, + version, + writeLoopBeginTime); + probesWritten += probingResult.packetsWritten; + bytesWritten += probingResult.bytesWritten; } VLOG_IF(10, probesWritten > 0) << nodeToString(connection.nodeType) << " writing probes using scheduler=CloningScheduler " << connection; - return probesWritten; + return {0, probesWritten, bytesWritten}; } WriteDataReason shouldWriteData(/*const*/ QuicConnectionStateBase& conn) { diff --git a/quic/api/QuicTransportFunctions.h b/quic/api/QuicTransportFunctions.h index c22865ab9..de0b65da7 100644 --- a/quic/api/QuicTransportFunctions.h +++ b/quic/api/QuicTransportFunctions.h @@ -295,7 +295,7 @@ WriteQuicDataResult writeConnectionDataToSocket( TimePoint writeLoopBeginTime, const std::string& token = std::string()); -uint64_t writeProbingDataToSocket( +WriteQuicDataResult writeProbingDataToSocket( folly::AsyncUDPSocket& sock, QuicConnectionStateBase& connection, const ConnectionId& srcConnId, diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 7a5a8ae93..42b12456c 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -43,18 +43,19 @@ uint64_t writeProbingDataToSocketForTest( .cryptoFrames()) .build(); return writeProbingDataToSocket( - sock, - conn, - *conn.clientConnectionId, - *conn.serverConnectionId, - ShortHeaderBuilder(), - EncryptionLevel::AppData, - PacketNumberSpace::AppData, - scheduler, - probesToSend, - aead, - headerCipher, - version); + sock, + conn, + *conn.clientConnectionId, + *conn.serverConnectionId, + ShortHeaderBuilder(), + EncryptionLevel::AppData, + PacketNumberSpace::AppData, + scheduler, + probesToSend, + aead, + headerCipher, + version) + .probesWritten; } void writeCryptoDataProbesToSocketForTest( @@ -2727,6 +2728,7 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketLimitTest) { conn->transportSettings.writeConnectionDataPacketsLimit); EXPECT_EQ(0, res.packetsWritten); EXPECT_EQ(0, res.probesWritten); + EXPECT_EQ(0, res.bytesWritten); // Normal limit conn->pendingEvents.numProbePackets[PacketNumberSpace::Initial] = 0; @@ -2734,11 +2736,13 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketLimitTest) { conn->pendingEvents.numProbePackets[PacketNumberSpace::AppData] = 0; conn->transportSettings.writeConnectionDataPacketsLimit = kDefaultWriteConnectionDataPacketLimit; + uint64_t actualWritten = 0; EXPECT_CALL(*rawSocket, write(_, _)) .Times(1) .WillOnce(Invoke([&](const SocketAddress&, const std::unique_ptr& iobuf) { writableBytes -= iobuf->computeChainDataLength(); + actualWritten += iobuf->computeChainDataLength(); return iobuf->computeChainDataLength(); })); EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(1); @@ -2755,9 +2759,11 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketLimitTest) { EXPECT_EQ(1, res.packetsWritten); EXPECT_EQ(0, res.probesWritten); + EXPECT_EQ(actualWritten, res.bytesWritten); // Probing can exceed packet limit. In practice we limit it to // kPacketToSendForPTO + actualWritten = 0; conn->pendingEvents.numProbePackets[PacketNumberSpace::AppData] = kDefaultWriteConnectionDataPacketLimit * 2; writeDataToQuicStream(*stream1, buf->clone(), true); @@ -2769,6 +2775,7 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketLimitTest) { .Times(kDefaultWriteConnectionDataPacketLimit * 2) .WillRepeatedly(Invoke([&](const SocketAddress&, const std::unique_ptr& iobuf) { + actualWritten += iobuf->computeChainDataLength(); return iobuf->computeChainDataLength(); })); EXPECT_CALL(*rawCongestionController, onPacketSent(_)) @@ -2787,6 +2794,7 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketLimitTest) { EXPECT_EQ(0, res.packetsWritten); EXPECT_EQ(kDefaultWriteConnectionDataPacketLimit * 2, res.probesWritten); + EXPECT_EQ(actualWritten, res.bytesWritten); } TEST_F( @@ -2934,6 +2942,7 @@ TEST_F(QuicTransportFunctionsTest, NothingWritten) { conn->transportSettings.writeConnectionDataPacketsLimit); EXPECT_EQ(0, res.packetsWritten); EXPECT_EQ(0, res.probesWritten); + EXPECT_EQ(0, res.bytesWritten); } const QuicWriteFrame& getFirstFrameInOutstandingPackets( @@ -3370,6 +3379,7 @@ TEST_F(QuicTransportFunctionsTest, NoCryptoProbeWriteIfNoProbeCredit) { EXPECT_EQ(1, res.packetsWritten); EXPECT_EQ(0, res.probesWritten); + EXPECT_EQ(conn->udpSendPacketLen, res.bytesWritten); ASSERT_EQ(1, conn->outstandings.packets.size()); EXPECT_TRUE(getFirstOutstandingPacket(*conn, PacketNumberSpace::Initial) ->metadata.isHandshake); @@ -3469,10 +3479,12 @@ TEST_F(QuicTransportFunctionsTest, WritePureAckWhenNoWritableBytes) { EXPECT_CALL(*rawCongestionController, getWritableBytes()) .WillRepeatedly(Return(0)); + uint64_t actualWritten = 0; EXPECT_CALL(*rawSocket, write(_, _)) .WillRepeatedly(Invoke([&](const SocketAddress&, const std::unique_ptr& iobuf) { EXPECT_LE(iobuf->computeChainDataLength(), 30); + actualWritten += iobuf->computeChainDataLength(); return iobuf->computeChainDataLength(); })); EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(0); @@ -3486,6 +3498,7 @@ TEST_F(QuicTransportFunctionsTest, WritePureAckWhenNoWritableBytes) { getVersion(*conn), conn->transportSettings.writeConnectionDataPacketsLimit); EXPECT_GT(res.packetsWritten, 0); + EXPECT_EQ(res.bytesWritten, actualWritten); EXPECT_EQ(0, conn->outstandings.packets.size()); } @@ -4044,7 +4057,13 @@ TEST_F(QuicTransportFunctionsTest, WriteLimitBytRttFraction) { auto buf = buildRandomInputData(2048 * 2048); writeDataToQuicStream(*stream1, buf->clone(), true); - EXPECT_CALL(*rawSocket, write(_, _)).WillRepeatedly(Return(1)); + uint64_t actualWritten = 0; + EXPECT_CALL(*rawSocket, write(_, _)) + .WillRepeatedly(Invoke([&](const SocketAddress&, + const std::unique_ptr& iobuf) { + actualWritten += iobuf->computeChainDataLength(); + return iobuf->computeChainDataLength(); + })); EXPECT_CALL(*rawCongestionController, getWritableBytes()) .WillRepeatedly(Return(50)); auto writeLoopBeginTime = Clock::now(); @@ -4060,6 +4079,7 @@ TEST_F(QuicTransportFunctionsTest, WriteLimitBytRttFraction) { writeLoopBeginTime); EXPECT_GT(1000, res.packetsWritten); + EXPECT_EQ(actualWritten, res.bytesWritten); EXPECT_EQ(res.probesWritten, 0); res = writeQuicDataToSocket(