From 612a00c3f9750b2829f008b55687d076e5244cb6 Mon Sep 17 00:00:00 2001 From: Matt Joras Date: Fri, 23 Jul 2021 14:19:55 -0700 Subject: [PATCH] Move happy eyeballs state to client state. Summary: This doesn't belong in the generic state. Untangling it is a little difficult, but I think this solution is cleaner than having it in the generic state. Reviewed By: JunqiWang Differential Revision: D29856391 fbshipit-source-id: 1042109ed29cd1d20d139e08548d187b469c8398 --- quic/api/IoBufQuicBatch.cpp | 51 +++++++++++-------- quic/api/IoBufQuicBatch.h | 5 +- quic/api/QuicTransportFunctions.cpp | 5 +- quic/api/test/IoBufQuicBatchTest.cpp | 4 +- quic/client/QuicClientTransport.cpp | 18 +++---- quic/client/state/ClientStateMachine.h | 30 +++++++++++ quic/dsr/backend/DSRPacketizer.cpp | 3 +- quic/dsr/backend/test/DSRPacketizerTest.cpp | 5 +- .../QuicHappyEyeballsFunctions.cpp | 10 ++-- .../QuicHappyEyeballsFunctions.h | 14 ++--- quic/state/StateData.h | 30 ----------- 11 files changed, 94 insertions(+), 81 deletions(-) diff --git a/quic/api/IoBufQuicBatch.cpp b/quic/api/IoBufQuicBatch.cpp index 165ba75ed..8c7a2b0c0 100644 --- a/quic/api/IoBufQuicBatch.cpp +++ b/quic/api/IoBufQuicBatch.cpp @@ -18,7 +18,7 @@ IOBufQuicBatch::IOBufQuicBatch( folly::AsyncUDPSocket& sock, const folly::SocketAddress& peerAddress, QuicTransportStatsCallback* statsCallback, - QuicConnectionStateBase::HappyEyeballsState& happyEyeballsState) + QuicClientConnectionState::HappyEyeballsState* happyEyeballsState) : batchWriter_(std::move(batchWriter)), threadLocal_(threadLocal), sock_(sock), @@ -76,38 +76,45 @@ bool IOBufQuicBatch::flushInternal() { bool written = false; folly::Optional firstSocketErrno; - if (happyEyeballsState_.shouldWriteToFirstSocket) { + if (!happyEyeballsState_ || happyEyeballsState_->shouldWriteToFirstSocket) { auto consumed = batchWriter_->write(sock_, peerAddress_); - firstSocketErrno = errno; + if (consumed < 0) { + firstSocketErrno = errno; + } written = (consumed >= 0); - happyEyeballsState_.shouldWriteToFirstSocket = - (consumed >= 0 || isRetriableError(errno)); + if (happyEyeballsState_) { + happyEyeballsState_->shouldWriteToFirstSocket = + (consumed >= 0 || isRetriableError(errno)); - if (!happyEyeballsState_.shouldWriteToFirstSocket) { - sock_.pauseRead(); + if (!happyEyeballsState_->shouldWriteToFirstSocket) { + sock_.pauseRead(); + } } } // If error occured on first socket, kick off second socket immediately - if (!written && happyEyeballsState_.connAttemptDelayTimeout && - happyEyeballsState_.connAttemptDelayTimeout->isScheduled()) { - happyEyeballsState_.connAttemptDelayTimeout->timeoutExpired(); - happyEyeballsState_.connAttemptDelayTimeout->cancelTimeout(); + if (!written && happyEyeballsState_ && + happyEyeballsState_->connAttemptDelayTimeout && + happyEyeballsState_->connAttemptDelayTimeout->isScheduled()) { + happyEyeballsState_->connAttemptDelayTimeout->timeoutExpired(); + happyEyeballsState_->connAttemptDelayTimeout->cancelTimeout(); } folly::Optional secondSocketErrno; - if (happyEyeballsState_.shouldWriteToSecondSocket) { + if (happyEyeballsState_ && happyEyeballsState_->shouldWriteToSecondSocket) { auto consumed = batchWriter_->write( - *happyEyeballsState_.secondSocket, - happyEyeballsState_.secondPeerAddress); - secondSocketErrno = errno; + *happyEyeballsState_->secondSocket, + happyEyeballsState_->secondPeerAddress); + if (consumed < 0) { + secondSocketErrno = errno; + } // written is marked true if either socket write succeeds written |= (consumed >= 0); - happyEyeballsState_.shouldWriteToSecondSocket = + happyEyeballsState_->shouldWriteToSecondSocket = (consumed >= 0 || isRetriableError(errno)); - if (!happyEyeballsState_.shouldWriteToSecondSocket) { - happyEyeballsState_.secondSocket->pauseRead(); + if (!happyEyeballsState_->shouldWriteToSecondSocket) { + happyEyeballsState_->secondSocket->pauseRead(); } } @@ -128,8 +135,12 @@ bool IOBufQuicBatch::flushInternal() { } } - if (!happyEyeballsState_.shouldWriteToFirstSocket && - !happyEyeballsState_.shouldWriteToSecondSocket) { + // If we have no happy eyeballs state, we only care if the first socket had + // an error. Otherwise we check both. + if ((!happyEyeballsState_ && firstSocketErrno.has_value() && + !isRetriableError(firstSocketErrno.value())) || + (happyEyeballsState_ && !happyEyeballsState_->shouldWriteToFirstSocket && + !happyEyeballsState_->shouldWriteToSecondSocket)) { auto firstSocketErrorMsg = firstSocketErrno.has_value() ? folly::to( folly::errnoStr(firstSocketErrno.value()), ", ") diff --git a/quic/api/IoBufQuicBatch.h b/quic/api/IoBufQuicBatch.h index 653750ee1..100357ad2 100644 --- a/quic/api/IoBufQuicBatch.h +++ b/quic/api/IoBufQuicBatch.h @@ -9,6 +9,7 @@ #pragma once #include #include +#include #include namespace quic { @@ -25,7 +26,7 @@ class IOBufQuicBatch { folly::AsyncUDPSocket& sock, const folly::SocketAddress& peerAddress, QuicTransportStatsCallback* statsCallback, - QuicConnectionStateBase::HappyEyeballsState& happyEyeballsState); + QuicClientConnectionState::HappyEyeballsState* happyEyeballsState); ~IOBufQuicBatch() = default; @@ -55,7 +56,7 @@ class IOBufQuicBatch { folly::AsyncUDPSocket& sock_; const folly::SocketAddress& peerAddress_; QuicTransportStatsCallback* statsCallback_{nullptr}; - QuicConnectionStateBase::HappyEyeballsState& happyEyeballsState_; + QuicClientConnectionState::HappyEyeballsState* happyEyeballsState_; uint64_t pktSent_{0}; }; diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index d21b62d97..57d91dc08 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -1320,13 +1320,16 @@ uint64_t writeConnectionDataToSocket( connection.transportSettings.dataPathType, connection); + auto happyEyeballsState = connection.nodeType == QuicNodeType::Server + ? nullptr + : &static_cast(connection).happyEyeballsState; IOBufQuicBatch ioBufBatch( std::move(batchWriter), connection.transportSettings.useThreadLocalBatching, sock, connection.peerAddress, connection.statsCallback, - connection.happyEyeballsState); + happyEyeballsState); if (connection.loopDetectorCallback) { connection.writeDebugState.schedulerName = scheduler.name().str(); diff --git a/quic/api/test/IoBufQuicBatchTest.cpp b/quic/api/test/IoBufQuicBatchTest.cpp index 18f18339c..ca3f88c86 100644 --- a/quic/api/test/IoBufQuicBatchTest.cpp +++ b/quic/api/test/IoBufQuicBatchTest.cpp @@ -27,7 +27,7 @@ void RunTest(int numBatch) { folly::SocketAddress peerAddress{"127.0.0.1", 1234}; QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); - QuicConnectionStateBase::HappyEyeballsState happyEyeballsState; + QuicClientConnectionState::HappyEyeballsState happyEyeballsState; IOBufQuicBatch ioBufBatch( std::move(batchWriter), @@ -35,7 +35,7 @@ void RunTest(int numBatch) { sock, peerAddress, conn.statsCallback, - happyEyeballsState); + nullptr /* happyEyeballsState */); std::string strTest("Test"); diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index 298bb2a56..13e772541 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -99,8 +99,8 @@ QuicClientTransport::~QuicClientTransport() { std::string("Closing from client destructor")), false); - if (conn_->happyEyeballsState.secondSocket) { - auto sock = std::move(conn_->happyEyeballsState.secondSocket); + if (clientConn_->happyEyeballsState.secondSocket) { + auto sock = std::move(clientConn_->happyEyeballsState.secondSocket); sock->pauseRead(); sock->close(); } @@ -213,7 +213,7 @@ void QuicClientTransport::processPacketData( if (happyEyeballsEnabled_) { happyEyeballsOnDataReceived( - *conn_, happyEyeballsConnAttemptDelayTimeout_, socket_, peer); + *clientConn_, happyEyeballsConnAttemptDelayTimeout_, socket_, peer); } // Set the destination connection ID to be the value from the source // connection id of the retry packet @@ -292,7 +292,7 @@ void QuicClientTransport::processPacketData( if (happyEyeballsEnabled_) { CHECK(socket_); happyEyeballsOnDataReceived( - *conn_, happyEyeballsConnAttemptDelayTimeout_, socket_, peer); + *clientConn_, happyEyeballsConnAttemptDelayTimeout_, socket_, peer); } LongHeader* longHeader = regularOptional->header.asLong(); @@ -1015,7 +1015,7 @@ void QuicClientTransport::errMessage( // exists, and the second socket is IPv4. Then we basically do the same // thing we would have done if we'd gotten a write error on that socket. // If both sockets are not functional we close the connection. - auto& happyEyeballsState = conn_->happyEyeballsState; + auto& happyEyeballsState = clientConn_->happyEyeballsState; if (!happyEyeballsState.finished) { if (cmsg.cmsg_level == SOL_IPV6 && happyEyeballsState.shouldWriteToFirstSocket) { @@ -1485,7 +1485,7 @@ void QuicClientTransport:: happyEyeballsConnAttemptDelayTimeoutExpired() noexcept { // Declare 0-RTT data as lost so that they will be retransmitted over the // second socket. - happyEyeballsStartSecondSocket(conn_->happyEyeballsState); + happyEyeballsStartSecondSocket(clientConn_->happyEyeballsState); // If this gets called from the write path then we haven't added the packets // to the outstanding packet list yet. runOnEvbAsync([&](auto) { markZeroRttPacketsLost(*conn_, markPacketLoss); }); @@ -1495,7 +1495,7 @@ void QuicClientTransport::start(ConnectionCallback* cb) { if (happyEyeballsEnabled_) { // TODO Supply v4 delay amount from somewhere when we want to tune this startHappyEyeballs( - *conn_, + *clientConn_, evb_, happyEyeballsCachedFamily_, happyEyeballsConnAttemptDelayTimeout_, @@ -1558,7 +1558,7 @@ void QuicClientTransport::addNewPeerAddress(folly::SocketAddress peerAddress) { conn_->udpSendPacketLen, (peerAddress.getFamily() == AF_INET6 ? kDefaultV6UDPSendPacketLen : kDefaultV4UDPSendPacketLen)); - happyEyeballsAddPeerAddress(*conn_, peerAddress); + happyEyeballsAddPeerAddress(*clientConn_, peerAddress); return; } @@ -1585,7 +1585,7 @@ void QuicClientTransport::setHappyEyeballsCachedFamily( void QuicClientTransport::addNewSocket( std::unique_ptr socket) { - happyEyeballsAddSocket(*conn_, std::move(socket)); + happyEyeballsAddSocket(*clientConn_, std::move(socket)); } void QuicClientTransport::setHostname(const std::string& hostname) { diff --git a/quic/client/state/ClientStateMachine.h b/quic/client/state/ClientStateMachine.h index 9af8294d7..a62b82414 100644 --- a/quic/client/state/ClientStateMachine.h +++ b/quic/client/state/ClientStateMachine.h @@ -71,6 +71,36 @@ struct QuicClientConnectionState : public QuicConnectionStateBase { uint64_t peerAdvertisedInitialMaxStreamsBidi{0}; uint64_t peerAdvertisedInitialMaxStreamsUni{0}; + struct HappyEyeballsState { + // Delay timer + folly::HHWheelTimer::Callback* connAttemptDelayTimeout{nullptr}; + + // IPv6 peer address + folly::SocketAddress v6PeerAddress; + + // IPv4 peer address + folly::SocketAddress v4PeerAddress; + + // The address that this socket will try to connect to after connection + // attempt delay timeout fires + folly::SocketAddress secondPeerAddress; + + // The UDP socket that will be used for the second connection attempt + std::unique_ptr secondSocket; + + // Whether should write to the first UDP socket + bool shouldWriteToFirstSocket{true}; + + // Whether should write to the second UDP socket + bool shouldWriteToSecondSocket{false}; + + // Whether HappyEyeballs has finished + // The signal of finishing is first successful decryption of a packet + bool finished{false}; + }; + + HappyEyeballsState happyEyeballsState; + // Short header packets we received but couldn't yet decrypt. std::vector pendingOneRttData; // Handshake packets we received but couldn't yet decrypt. diff --git a/quic/dsr/backend/DSRPacketizer.cpp b/quic/dsr/backend/DSRPacketizer.cpp index 02dd53e7d..642707716 100644 --- a/quic/dsr/backend/DSRPacketizer.cpp +++ b/quic/dsr/backend/DSRPacketizer.cpp @@ -104,14 +104,13 @@ size_t writePacketsGroup( auto batchWriter = BatchWriterPtr(new GSOPacketBatchWriter(kDefaultQuicMaxBatchSize)); // This doesn't matter: - QuicConnectionStateBase::HappyEyeballsState happyEyeballsState; IOBufQuicBatch ioBufBatch( std::move(batchWriter), false /* thread local batching */, sock, reqGroup[0].clientAddress, nullptr /* statsCallback */, - happyEyeballsState); + nullptr /* happyEyeballsState */); // TODO: Instead of building ciphers every time, we should cache them into a // CipherMap and look them up. CipherBuilder cipherBuilder; diff --git a/quic/dsr/backend/test/DSRPacketizerTest.cpp b/quic/dsr/backend/test/DSRPacketizerTest.cpp index bd003e61b..a3f1cb42c 100644 --- a/quic/dsr/backend/test/DSRPacketizerTest.cpp +++ b/quic/dsr/backend/test/DSRPacketizerTest.cpp @@ -42,7 +42,6 @@ class DSRPacketizerSingleWriteTest : public Test { } folly::EventBase evb; - QuicConnectionStateBase::HappyEyeballsState happyEyeballsState; folly::SocketAddress peerAddress{"127.0.0.1", 1234}; std::unique_ptr aead; std::unique_ptr headerCipher; @@ -59,7 +58,7 @@ TEST_F(DSRPacketizerSingleWriteTest, SingleWrite) { *socket, peerAddress, nullptr /* statsCallback */, - happyEyeballsState); + nullptr /* happyEyeballsState */); PacketNum packetNum = 20; PacketNum largestAckedByPeer = 0; StreamId streamId = 0; @@ -102,7 +101,7 @@ TEST_F(DSRPacketizerSingleWriteTest, NotEnoughData) { *socket, peerAddress, nullptr /* statsCallback */, - happyEyeballsState); + nullptr /* happyEyeballsState */); PacketNum packetNum = 20; PacketNum largestAckedByPeer = 0; StreamId streamId = 0; diff --git a/quic/happyeyeballs/QuicHappyEyeballsFunctions.cpp b/quic/happyeyeballs/QuicHappyEyeballsFunctions.cpp index 7f9491166..36072868f 100644 --- a/quic/happyeyeballs/QuicHappyEyeballsFunctions.cpp +++ b/quic/happyeyeballs/QuicHappyEyeballsFunctions.cpp @@ -28,7 +28,7 @@ namespace fsp = folly::portability::sockets; namespace quic { void happyEyeballsAddPeerAddress( - QuicConnectionStateBase& connection, + QuicClientConnectionState& connection, const folly::SocketAddress& peerAddress) { // TODO: Do not wait for both IPv4 and IPv6 addresses to return before // attempting connection establishment. -- RFC8305 @@ -52,13 +52,13 @@ void happyEyeballsAddPeerAddress( } void happyEyeballsAddSocket( - QuicConnectionStateBase& connection, + QuicClientConnectionState& connection, std::unique_ptr socket) { connection.happyEyeballsState.secondSocket = std::move(socket); } void startHappyEyeballs( - QuicConnectionStateBase& connection, + QuicClientConnectionState& connection, folly::EventBase* evb, sa_family_t cachedFamily, folly::HHWheelTimer::Callback& connAttemptDelayTimeout, @@ -166,14 +166,14 @@ void happyEyeballsSetUpSocket( } void happyEyeballsStartSecondSocket( - QuicConnectionStateBase::HappyEyeballsState& happyEyeballsState) { + QuicClientConnectionState::HappyEyeballsState& happyEyeballsState) { CHECK(!happyEyeballsState.finished); happyEyeballsState.shouldWriteToSecondSocket = true; } void happyEyeballsOnDataReceived( - QuicConnectionStateBase& connection, + QuicClientConnectionState& connection, folly::HHWheelTimer::Callback& connAttemptDelayTimeout, std::unique_ptr& socket, const folly::SocketAddress& peerAddress) { diff --git a/quic/happyeyeballs/QuicHappyEyeballsFunctions.h b/quic/happyeyeballs/QuicHappyEyeballsFunctions.h index ae2971995..6b10a3b8e 100644 --- a/quic/happyeyeballs/QuicHappyEyeballsFunctions.h +++ b/quic/happyeyeballs/QuicHappyEyeballsFunctions.h @@ -8,7 +8,7 @@ #pragma once -#include +#include #include #include @@ -26,15 +26,15 @@ namespace quic { struct TransportSettings; void happyEyeballsAddPeerAddress( - QuicConnectionStateBase& connection, + QuicClientConnectionState& connection, const folly::SocketAddress& peerAddress); void happyEyeballsAddSocket( - QuicConnectionStateBase& connection, + QuicClientConnectionState& connection, std::unique_ptr socket); void startHappyEyeballs( - QuicConnectionStateBase& connection, + QuicClientConnectionState& connection, folly::EventBase* evb, sa_family_t cachedFamily, folly::HHWheelTimer::Callback& connAttemptDelayTimeout, @@ -43,7 +43,7 @@ void startHappyEyeballs( folly::AsyncUDPSocket::ReadCallback* readCallback, const folly::SocketOptionMap& options); -void resetHappyEyeballs(QuicConnectionStateBase& connection); +void resetHappyEyeballs(QuicClientConnectionState& connection); void happyEyeballsSetUpSocket( folly::AsyncUDPSocket& socket, @@ -55,10 +55,10 @@ void happyEyeballsSetUpSocket( const folly::SocketOptionMap& options); void happyEyeballsStartSecondSocket( - QuicConnectionStateBase::HappyEyeballsState& happyEyeballsState); + QuicClientConnectionState::HappyEyeballsState& happyEyeballsState); void happyEyeballsOnDataReceived( - QuicConnectionStateBase& connection, + QuicClientConnectionState& connection, folly::HHWheelTimer::Callback& connAttemptDelayTimeout, std::unique_ptr& socket, const folly::SocketAddress& peerAddress); diff --git a/quic/state/StateData.h b/quic/state/StateData.h index 13d5269ba..7ed971ddf 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -685,36 +685,6 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { // Track stats for various server events QuicTransportStatsCallback* statsCallback{nullptr}; - struct HappyEyeballsState { - // Delay timer - folly::HHWheelTimer::Callback* connAttemptDelayTimeout{nullptr}; - - // IPv6 peer address - folly::SocketAddress v6PeerAddress; - - // IPv4 peer address - folly::SocketAddress v4PeerAddress; - - // The address that this socket will try to connect to after connection - // attempt delay timeout fires - folly::SocketAddress secondPeerAddress; - - // The UDP socket that will be used for the second connection attempt - std::unique_ptr secondSocket; - - // Whether should write to the first UDP socket - bool shouldWriteToFirstSocket{true}; - - // Whether should write to the second UDP socket - bool shouldWriteToSecondSocket{false}; - - // Whether HappyEyeballs has finished - // The signal of finishing is first successful decryption of a packet - bool finished{false}; - }; - - HappyEyeballsState happyEyeballsState; - // Meta state of d6d, mostly useful for analytics. D6D can operate without it. struct D6DMetaState { // Cumulative count of acked packets