From ad3dd0ec01d43a4dad1c23e8c629bd025dfd8af5 Mon Sep 17 00:00:00 2001 From: Brandon Schlinker Date: Thu, 21 Sep 2023 07:57:58 -0700 Subject: [PATCH] Cleanup and modularize receive path, improve timestamp support [7/x] Summary: This diff: - Adds `QuicAsyncUDPSocketWrapperImpl` and changes existing instantiatons of `QuicAsyncUDPSocketWrapper` to instead instantiate `QuicAsyncUDPSocketWrapperImpl`. In follow up diffs, pure virtual functions will be added to `QuicAsyncUDPSocketWrapper` and implemented in `QuicAsyncUDPSocketWrapperImpl`. See D48717388 for more information. -- This diff is part of a larger stack focused on the following: - **Cleaning up client and server UDP packet receive paths while improving testability.** We currently have multiple receive paths for client and server. Capabilities vary significantly and there are few tests. For instance: - The server receive path supports socket RX timestamps, abet incorrectly in that it does not store timestamp per packet. In comparison, the client receive path does not currently support socket RX timestamps, although the code in `QuicClientTransport::recvmsg` and `QuicClientTransport::recvmmsg` makes reference to socket RX timestamps, making it confusing to understand the capabilities available when tracing through the code. This complicates the tests in `QuicTypedTransportTests`, as we have to disable test logic that depends on socket RX timestamps for client tests. - The client currently has three receive paths, and none of them are well tested. - **Modularize and abstract components in the receive path.** This will make it easier to mock/fake the UDP socket and network layers. - `QuicClientTransport` and `QuicServerTransport` currently contain UDP socket handling logic that operates over lower layer primitives such `cmsg` and `io_vec` (see `QuicClientTransport::recvmmsg` and `...::recvmsg` as examples). - Because this UDP socket handling logic is inside of the mvfst transport implementations, it is difficult to test this logic in isolation and mock/fake the underlying socket and network layers. For instance, injecting a user space network emulator that operates at the socket layer would require faking `folly::AsyncUDPSocket`, which is non-trivial given that `AsyncUDPSocket` does not abstract away intricacies arising from the aforementioned lower layer primitives. - By shifting this logic into an intermediate layer between the transport and the underlying UDP socket, it will be easier to mock out the UDP socket layer when testing functionality at higher layers, and inject fake components when we want to emulate the network between a mvfst client and server. It will also be easier for us to have unit tests focused on testing interactions between the UDP socket implementation and this intermediate layer. - **Improving receive path timestamping.** We only record a single timestamp per `NetworkData` at the moment, but (1) it is possible for a `NetworkData` to have multiple packets, each with their own timestamps, and (2) we should be able to record both userspace and socket timestamps. Reviewed By: jbeshay, mjoras Differential Revision: D48717592 fbshipit-source-id: e21368f5c1f3b37608fc1c88617e96b93a02f6e0 --- quic/api/QuicBatchWriterFactory.cpp | 2 +- quic/api/test/IoBufQuicBatchTest.cpp | 2 +- quic/api/test/QuicBatchWriterTest.cpp | 12 ++++++------ quic/api/test/QuicStreamAsyncTransportTest.cpp | 2 +- quic/client/connector/QuicConnector.cpp | 2 +- quic/client/test/ClientStateMachineTest.cpp | 2 +- quic/client/test/QuicConnectorTest.cpp | 2 +- quic/common/QuicAsyncUDPSocketWrapper.h | 6 ++++++ quic/common/testutil/MockAsyncUDPSocket.h | 2 +- quic/fizz/client/test/QuicClientTransportTest.cpp | 4 ++-- quic/samples/echo/EchoClient.h | 2 +- quic/server/QuicReusePortUDPSocketFactory.h | 2 +- quic/server/QuicServerPacketRouter.cpp | 2 +- quic/server/QuicSharedUDPSocketFactory.h | 2 +- .../async_tran/test/QuicAsyncTransportServerTest.cpp | 2 +- quic/server/test/QuicServerTest.cpp | 8 ++++---- quic/tools/tperf/tperf.cpp | 2 +- 17 files changed, 31 insertions(+), 25 deletions(-) diff --git a/quic/api/QuicBatchWriterFactory.cpp b/quic/api/QuicBatchWriterFactory.cpp index 39ddaa9b2..422c82700 100644 --- a/quic/api/QuicBatchWriterFactory.cpp +++ b/quic/api/QuicBatchWriterFactory.cpp @@ -97,7 +97,7 @@ class ThreadLocalBatchWriterCache : public folly::AsyncTimeout { if (evb && evb->getBackingEventBase() && !socket_) { auto fd = writer->getAndResetFd(); if (fd >= 0) { - socket_ = std::make_unique( + socket_ = std::make_unique( evb->getBackingEventBase()); socket_->setFD( quic::toNetworkFdType(fd), diff --git a/quic/api/test/IoBufQuicBatchTest.cpp b/quic/api/test/IoBufQuicBatchTest.cpp index ccbb44708..dfccabe39 100644 --- a/quic/api/test/IoBufQuicBatchTest.cpp +++ b/quic/api/test/IoBufQuicBatchTest.cpp @@ -21,7 +21,7 @@ namespace quic { namespace testing { void RunTest(int numBatch) { folly::EventBase evb; - QuicAsyncUDPSocketWrapper sock(&evb); + QuicAsyncUDPSocketWrapperImpl sock(&evb); auto batchWriter = BatchWriterPtr(new test::TestPacketBatchWriter(numBatch)); folly::SocketAddress peerAddress{"127.0.0.1", 1234}; diff --git a/quic/api/test/QuicBatchWriterTest.cpp b/quic/api/test/QuicBatchWriterTest.cpp index 8dc9f3379..30664a030 100644 --- a/quic/api/test/QuicBatchWriterTest.cpp +++ b/quic/api/test/QuicBatchWriterTest.cpp @@ -63,7 +63,7 @@ TEST_P(QuicBatchWriterTest, TestBatchingNone) { TEST_P(QuicBatchWriterTest, TestBatchingGSOBase) { bool useThreadLocal = GetParam(); folly::EventBase evb; - QuicAsyncUDPSocketWrapper sock(&evb); + QuicAsyncUDPSocketWrapperImpl sock(&evb); sock.setReuseAddr(false); sock.bind(folly::SocketAddress("127.0.0.1", 0)); gsoSupported_ = sock.getGSO() >= 0; @@ -93,7 +93,7 @@ TEST_P(QuicBatchWriterTest, TestBatchingGSOBase) { TEST_P(QuicBatchWriterTest, TestBatchingGSOLastSmallPacket) { bool useThreadLocal = GetParam(); folly::EventBase evb; - QuicAsyncUDPSocketWrapper sock(&evb); + QuicAsyncUDPSocketWrapperImpl sock(&evb); sock.setReuseAddr(false); sock.bind(folly::SocketAddress("127.0.0.1", 0)); gsoSupported_ = sock.getGSO() >= 0; @@ -135,7 +135,7 @@ TEST_P(QuicBatchWriterTest, TestBatchingGSOLastSmallPacket) { TEST_P(QuicBatchWriterTest, TestBatchingGSOLastBigPacket) { bool useThreadLocal = GetParam(); folly::EventBase evb; - QuicAsyncUDPSocketWrapper sock(&evb); + QuicAsyncUDPSocketWrapperImpl sock(&evb); sock.setReuseAddr(false); sock.bind(folly::SocketAddress("127.0.0.1", 0)); gsoSupported_ = sock.getGSO() >= 0; @@ -172,7 +172,7 @@ TEST_P(QuicBatchWriterTest, TestBatchingGSOLastBigPacket) { TEST_P(QuicBatchWriterTest, TestBatchingGSOBatchNum) { bool useThreadLocal = GetParam(); folly::EventBase evb; - QuicAsyncUDPSocketWrapper sock(&evb); + QuicAsyncUDPSocketWrapperImpl sock(&evb); sock.setReuseAddr(false); sock.bind(folly::SocketAddress("127.0.0.1", 0)); gsoSupported_ = sock.getGSO() >= 0; @@ -255,7 +255,7 @@ TEST_P(QuicBatchWriterTest, TestBatchingSendmmsg) { TEST_P(QuicBatchWriterTest, TestBatchingSendmmsgGSOBatchNum) { bool useThreadLocal = GetParam(); folly::EventBase evb; - QuicAsyncUDPSocketWrapper sock(&evb); + QuicAsyncUDPSocketWrapperImpl sock(&evb); sock.setReuseAddr(false); sock.bind(folly::SocketAddress("127.0.0.1", 0)); gsoSupported_ = sock.getGSO() >= 0; @@ -301,7 +301,7 @@ TEST_P(QuicBatchWriterTest, TestBatchingSendmmsgGSOBatchNum) { TEST_P(QuicBatchWriterTest, TestBatchingSendmmsgGSOBatcBigSmallPacket) { bool useThreadLocal = GetParam(); folly::EventBase evb; - QuicAsyncUDPSocketWrapper sock(&evb); + QuicAsyncUDPSocketWrapperImpl sock(&evb); sock.setReuseAddr(false); sock.bind(folly::SocketAddress("127.0.0.1", 0)); gsoSupported_ = sock.getGSO() >= 0; diff --git a/quic/api/test/QuicStreamAsyncTransportTest.cpp b/quic/api/test/QuicStreamAsyncTransportTest.cpp index ffa7a86d3..90ea64e6f 100644 --- a/quic/api/test/QuicStreamAsyncTransportTest.cpp +++ b/quic/api/test/QuicStreamAsyncTransportTest.cpp @@ -156,7 +156,7 @@ class QuicStreamAsyncTransportTest : public Test { .WillOnce(Invoke([&p = promise]() mutable { p.setValue(); })); clientEvb_.runInLoop([&]() { - auto sock = std::make_unique(&clientEvb_); + auto sock = std::make_unique(&clientEvb_); auto fizzClientContext = FizzClientQuicHandshakeContext::Builder() .setCertificateVerifier(test::createTestCertificateVerifier()) diff --git a/quic/client/connector/QuicConnector.cpp b/quic/client/connector/QuicConnector.cpp index b58596040..b0535f56c 100644 --- a/quic/client/connector/QuicConnector.cpp +++ b/quic/client/connector/QuicConnector.cpp @@ -55,7 +55,7 @@ void QuicConnector::connect( return; } - auto sock = std::make_unique(eventBase); + auto sock = std::make_unique(eventBase); quicClient_ = quic::QuicClientTransport::newClient( eventBase, std::move(sock), diff --git a/quic/client/test/ClientStateMachineTest.cpp b/quic/client/test/ClientStateMachineTest.cpp index 6192612d8..b28384e35 100644 --- a/quic/client/test/ClientStateMachineTest.cpp +++ b/quic/client/test/ClientStateMachineTest.cpp @@ -107,7 +107,7 @@ TEST_F(ClientStateMachineTest, PreserveHappyeyabllsDuringUndo) { client_->clientConnectionId = ConnectionId::createRandom(8); client_->happyEyeballsState.finished = true; client_->happyEyeballsState.secondSocket = - std::make_unique(&evb); + std::make_unique(&evb); auto newConn = undoAllClientStateForRetry(std::move(client_)); EXPECT_TRUE(newConn->happyEyeballsState.finished); EXPECT_NE(nullptr, newConn->happyEyeballsState.secondSocket); diff --git a/quic/client/test/QuicConnectorTest.cpp b/quic/client/test/QuicConnectorTest.cpp index fd2ba4d0d..8db3cd6dd 100644 --- a/quic/client/test/QuicConnectorTest.cpp +++ b/quic/client/test/QuicConnectorTest.cpp @@ -32,7 +32,7 @@ class QuicConnectorTest : public Test { auto verifier = createTestCertificateVerifier(); auto clientCtx = std::make_shared(); auto pskCache = std::make_shared(); - auto sock = std::make_unique(&eventBase_); + auto sock = std::make_unique(&eventBase_); auto fizzClientContext = FizzClientQuicHandshakeContext::Builder() .setFizzClientContext(clientCtx) .setCertificateVerifier(verifier) diff --git a/quic/common/QuicAsyncUDPSocketWrapper.h b/quic/common/QuicAsyncUDPSocketWrapper.h index 923517a93..2f141914d 100644 --- a/quic/common/QuicAsyncUDPSocketWrapper.h +++ b/quic/common/QuicAsyncUDPSocketWrapper.h @@ -32,6 +32,12 @@ class QuicAsyncUDPSocketWrapper : public QuicAsyncUDPSocketType { using ErrMessageCallback = QuicAsyncUDPSocketType::ErrMessageCallback; }; +class QuicAsyncUDPSocketWrapperImpl : public QuicAsyncUDPSocketWrapper { + public: + using QuicAsyncUDPSocketWrapper::QuicAsyncUDPSocketWrapper; + ~QuicAsyncUDPSocketWrapperImpl() override = default; +}; + int getSocketFd(const QuicAsyncUDPSocketWrapper& s); NetworkFdType toNetworkFdType(int fd); diff --git a/quic/common/testutil/MockAsyncUDPSocket.h b/quic/common/testutil/MockAsyncUDPSocket.h index 42c991edf..3a7096e99 100644 --- a/quic/common/testutil/MockAsyncUDPSocket.h +++ b/quic/common/testutil/MockAsyncUDPSocket.h @@ -13,6 +13,6 @@ namespace quic::test { using MockAsyncUDPSocket = - folly::test::MockAsyncUDPSocketT; + folly::test::MockAsyncUDPSocketT; } // namespace quic::test diff --git a/quic/fizz/client/test/QuicClientTransportTest.cpp b/quic/fizz/client/test/QuicClientTransportTest.cpp index 60fb08b0f..dc23b89ae 100644 --- a/quic/fizz/client/test/QuicClientTransportTest.cpp +++ b/quic/fizz/client/test/QuicClientTransportTest.cpp @@ -89,7 +89,7 @@ class QuicClientTransportIntegrationTest : public TestWithParam { std::shared_ptr createClient() { pskCache_ = std::make_shared(); - auto sock = std::make_unique(&eventbase_); + auto sock = std::make_unique(&eventbase_); auto fizzClientContext = FizzClientQuicHandshakeContext::Builder() .setFizzClientContext(clientCtx) .setCertificateVerifier(verifier) @@ -5795,7 +5795,7 @@ TEST(AsyncUDPSocketTest, CloseMultipleTimes) { }; EventBase evb; - QuicAsyncUDPSocketWrapper socket(&evb); + QuicAsyncUDPSocketWrapperImpl socket(&evb); TransportSettings transportSettings; EmptyErrMessageCallback errMessageCallback; EmptyReadCallback readCallback; diff --git a/quic/samples/echo/EchoClient.h b/quic/samples/echo/EchoClient.h index 307fde9e1..f29e33d1a 100644 --- a/quic/samples/echo/EchoClient.h +++ b/quic/samples/echo/EchoClient.h @@ -196,7 +196,7 @@ class EchoClient : public quic::QuicSocket::ConnectionSetupCallback, folly::SocketAddress addr(host_.c_str(), port_); evb->runInEventBaseThreadAndWait([&] { - auto sock = std::make_unique(evb); + auto sock = std::make_unique(evb); auto fizzClientContext = FizzClientQuicHandshakeContext::Builder() .setCertificateVerifier(test::createTestCertificateVerifier()) diff --git a/quic/server/QuicReusePortUDPSocketFactory.h b/quic/server/QuicReusePortUDPSocketFactory.h index bfcabde83..a0f5f6492 100644 --- a/quic/server/QuicReusePortUDPSocketFactory.h +++ b/quic/server/QuicReusePortUDPSocketFactory.h @@ -19,7 +19,7 @@ class QuicReusePortUDPSocketFactory : public QuicUDPSocketFactory { std::unique_ptr make(folly::EventBase* evb, int) override { - auto sock = std::make_unique(evb); + auto sock = std::make_unique(evb); sock->setReusePort(reusePort_); sock->setReuseAddr(reuseAddr_); return sock; diff --git a/quic/server/QuicServerPacketRouter.cpp b/quic/server/QuicServerPacketRouter.cpp index 9da961994..be71095b2 100644 --- a/quic/server/QuicServerPacketRouter.cpp +++ b/quic/server/QuicServerPacketRouter.cpp @@ -172,7 +172,7 @@ void TakeoverPacketHandler::forwardPacket(Buf writeBuffer) { std::unique_ptr TakeoverPacketHandler::makeSocket( folly::EventBase* evb) { - return std::make_unique(evb); + return std::make_unique(evb); } void TakeoverPacketHandler::processForwardedPacket( diff --git a/quic/server/QuicSharedUDPSocketFactory.h b/quic/server/QuicSharedUDPSocketFactory.h index ba0a7c496..4a4862aeb 100644 --- a/quic/server/QuicSharedUDPSocketFactory.h +++ b/quic/server/QuicSharedUDPSocketFactory.h @@ -18,7 +18,7 @@ class QuicSharedUDPSocketFactory : public QuicUDPSocketFactory { std::unique_ptr make(folly::EventBase* evb, int fd) override { - auto sock = std::make_unique(evb); + auto sock = std::make_unique(evb); if (fd != -1) { sock->setFD( folly::NetworkSocket::fromFd(fd), diff --git a/quic/server/async_tran/test/QuicAsyncTransportServerTest.cpp b/quic/server/async_tran/test/QuicAsyncTransportServerTest.cpp index e82ded8ec..f9540da19 100644 --- a/quic/server/async_tran/test/QuicAsyncTransportServerTest.cpp +++ b/quic/server/async_tran/test/QuicAsyncTransportServerTest.cpp @@ -84,7 +84,7 @@ class QuicAsyncTransportServerTest : public Test { EXPECT_CALL(clientWriteCB_, writeSuccess_()).WillOnce(Return()); clientEvb_.runInEventBaseThreadAndWait([&]() { - auto sock = std::make_unique(&clientEvb_); + auto sock = std::make_unique(&clientEvb_); auto fizzClientContext = FizzClientQuicHandshakeContext::Builder() .setCertificateVerifier(test::createTestCertificateVerifier()) diff --git a/quic/server/test/QuicServerTest.cpp b/quic/server/test/QuicServerTest.cpp index 95557c9b2..9f7409560 100644 --- a/quic/server/test/QuicServerTest.cpp +++ b/quic/server/test/QuicServerTest.cpp @@ -2192,7 +2192,7 @@ class QuicServerTest : public Test { folly::SocketAddress addr2("::1", 0); std::unique_ptr client; evbThread_.getEventBase()->runInEventBaseThreadAndWait([&] { - client = std::make_unique( + client = std::make_unique( evbThread_.getEventBase()); client->bind(addr2); }); @@ -2476,7 +2476,7 @@ class QuicServerTakeoverTest : public Test { folly::SocketAddress clientAddr("::1", 0); std::unique_ptr client; evbThread_.getEventBase()->runInEventBaseThreadAndWait([&] { - client = std::make_unique( + client = std::make_unique( evbThread_.getEventBase()); client->bind(clientAddr); }); @@ -2624,7 +2624,7 @@ struct UDPReader : public QuicAsyncUDPSocketWrapper::ReadCallback { void start(EventBase* evb, SocketAddress addr) { evb_ = evb; evb_->runInEventBaseThreadAndWait([&] { - client = std::make_unique(evb_); + client = std::make_unique(evb_); client->bind(addr); client->resumeRead(this); }); @@ -3231,7 +3231,7 @@ class ServerTransportParameters : public testing::Test { .build(); auto client = std::make_shared( &evb_, - std::make_unique(&evb_), + std::make_unique(&evb_), std::move(fizzClientContext)); client->addNewPeerAddress(server_->getAddress()); client->setHostname("::1"); diff --git a/quic/tools/tperf/tperf.cpp b/quic/tools/tperf/tperf.cpp index 8eccadac2..03d79518e 100644 --- a/quic/tools/tperf/tperf.cpp +++ b/quic/tools/tperf/tperf.cpp @@ -648,7 +648,7 @@ class TPerfClient : public quic::QuicSocket::ConnectionSetupCallback, void start() { folly::SocketAddress addr(host_.c_str(), port_); - auto sock = std::make_unique(&eventBase_); + auto sock = std::make_unique(&eventBase_); auto fizzClientContext = FizzClientQuicHandshakeContext::Builder() .setCertificateVerifier(test::createTestCertificateVerifier())