Move the delayed destruction from Handshake to QuicConnectionStateBase

Summary: Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/88

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

 ---
## Proxygen Canary
Traffic Canary: https://our.intern.facebook.com/intern/traffic/canary?fbid=224323975233396
* elb.prod.ham3c01 - binary_asan - 2020-02-05 02:00 - https://fburl.com/dyndash/u2q12hwq
* elb.prod.mia3c02 - binary - 2020-01-31 09:40 - https://fburl.com/dyndash/vmv34rpa
* elb.prod.otp1c01 - binary - 2020-02-05 02:26 - https://fburl.com/dyndash/0zttm61b
* flb.prod.fath4c02 - binary - 2020-02-05 02:26 - https://fburl.com/dyndash/6o1nqsti
* flb.prod.fgye3c01 - binary - 2020-01-31 09:40 - https://fburl.com/dyndash/nu3i5ahw
* olb.prod.rfrc0c01.p2 - binary - 2020-01-31 09:40 - https://fburl.com/dyndash/c1o6hpqw
* olb.prod.rftw0c01.p2 - binary - 2020-02-05 02:26 - https://fburl.com/dyndash/xg6qbyru
* slb.prod_regional.rcln0c00 - binary - 2020-02-05 02:26 - https://fburl.com/dyndash/e4qkbzcz
* slb.prod_regional.rfrc0c00 - binary - 2020-01-31 09:40 - https://fburl.com/dyndash/j0yxofty
* slb.prod_regional.rrva0c00 - binary_asan - 2020-02-05 02:00 - https://fburl.com/dyndash/4hsg02uj
* slb.regional.rfrc0c01.p2 - binary - 2020-01-31 09:40 - https://fburl.com/dyndash/1njxzbgf
* slb.regional.rvll0c01.p2 - binary - 2020-02-05 02:26 - https://fburl.com/dyndash/056xdmzn
 ---

Reviewed By: lnicco

Differential Revision: D19551142

Pulled By: mjoras

fbshipit-source-id: b0d14146d14384b8c37887b3e9d8fed7d6181d88

quic/api/QuicTransportBase.h

@@ -8,9 +8,6 @@
 
 #pragma once
 
-#include <folly/ExceptionWrapper.h>
-#include <folly/io/async/AsyncUDPSocket.h>
-#include <folly/io/async/HHWheelTimer.h>
 #include <quic/QuicConstants.h>
 #include <quic/QuicException.h>
 #include <quic/api/QuicSocket.h>
@@ -22,6 +19,10 @@
 #include <quic/congestion_control/QuicCubic.h>
 #include <quic/state/StateData.h>
 
+#include <folly/ExceptionWrapper.h>
+#include <folly/io/async/AsyncUDPSocket.h>
+#include <folly/io/async/HHWheelTimer.h>
+
 namespace quic {
 
 enum class CloseState { OPEN, GRACEFUL_CLOSING, CLOSED };
@@ -561,7 +562,9 @@ class QuicTransportBase : public QuicSocket {
   std::unique_ptr<folly::AsyncUDPSocket> socket_;
   ConnectionCallback* connCallback_{nullptr};
 
-  std::unique_ptr<QuicConnectionStateBase> conn_;
+  std::
+      unique_ptr<QuicConnectionStateBase, folly::DelayedDestruction::Destructor>
+          conn_;
 
   struct ReadCallbackData {
     ReadCallback* readCb;

quic/api/test/QuicTransportBaseTest.cpp

@@ -136,7 +136,7 @@ class TestQuicTransport
     conn->clientConnectionId = ConnectionId({10, 9, 8, 7});
     conn->version = QuicVersion::MVFST;
     transportConn = conn.get();
-    conn_ = std::move(conn);
+    conn_.reset(conn.release());
     aead = test::createNoOpAead();
     headerCipher = test::createNoOpHeaderCipher();
     connIdAlgo_ = std::make_unique<DefaultConnectionIdAlgo>();

quic/api/test/QuicTransportTest.cpp

@@ -42,7 +42,7 @@ class TestQuicTransport
       ConnectionCallback& cb)
       : QuicTransportBase(evb, std::move(socket)) {
     setConnectionCallback(&cb);
-    conn_ = std::make_unique<QuicServerConnectionState>();
+    conn_.reset(new QuicServerConnectionState());
     conn_->clientConnectionId = ConnectionId({9, 8, 7, 6});
     conn_->serverConnectionId = ConnectionId({1, 2, 3, 4});
     conn_->version = QuicVersion::MVFST;

quic/client/QuicClientTransport.cpp

@@ -54,7 +54,7 @@ QuicClientTransport::QuicClientTransport(
   auto tempConn =
       std::make_unique<QuicClientConnectionState>(std::move(handshakeFactory));
   clientConn_ = tempConn.get();
-  conn_ = std::move(tempConn);
+  conn_.reset(tempConn.release());
   std::vector<uint8_t> connIdData(
       std::max(kMinInitialDestinationConnIdLength, connectionIdSize));
   folly::Random::secureRandom(connIdData.data(), connIdData.size());
@@ -208,7 +208,7 @@ void QuicClientTransport::processPacketData(
     auto tempConn = undoAllClientStateForRetry(std::move(uniqueClient));
 
     clientConn_ = tempConn.get();
-    conn_ = std::move(tempConn);
+    conn_.reset(tempConn.release());
 
     clientConn_->retryToken = longHeader->getToken();
 

quic/client/state/ClientStateMachine.h

@@ -60,7 +60,7 @@ struct QuicClientConnectionState : public QuicConnectionStateBase {
     auto tmpClientHandshake =
         handshakeFactory->makeClientHandshake(*cryptoState);
     clientHandshakeLayer = tmpClientHandshake.get();
-    handshakeLayer.reset(tmpClientHandshake.release());
+    handshakeLayer = std::move(tmpClientHandshake);
     // We shouldn't normally need to set this until we're starting the
     // transport, however writing unit tests is much easier if we set this here.
     updateFlowControlStateWithSettings(flowControlState, transportSettings);

quic/client/test/QuicClientTransportTest.cpp

@@ -1354,8 +1354,6 @@ class QuicClientTransportTest : public Test {
         new FakeOneRttHandshakeLayer(*client->getNonConstConn().cryptoState);
     client->getNonConstConn().clientHandshakeLayer = mockClientHandshake;
     client->getNonConstConn().handshakeLayer.reset(mockClientHandshake);
-    handshakeDG = std::make_unique<DelayedDestruction::DestructorGuard>(
-        mockClientHandshake);
     setFakeHandshakeCiphers();
     // Allow ignoring path mtu for testing negotiation.
     client->getNonConstConn().transportSettings.canIgnorePathMTU = true;
@@ -1690,7 +1688,6 @@ class QuicClientTransportTest : public Test {
   std::unique_ptr<folly::EventBase> eventbase_;
   SocketAddress serverAddr{"127.0.0.1", 443};
   AsyncUDPSocket::ReadCallback* networkReadCallback{nullptr};
-  std::unique_ptr<DelayedDestruction::DestructorGuard> handshakeDG;
   FakeOneRttHandshakeLayer* mockClientHandshake;
   std::shared_ptr<TestingQuicClientTransport> client;
   PacketNum initialPacketNum{0}, handshakePacketNum{0}, appDataPacketNum{0};

quic/handshake/HandshakeLayer.h

@@ -8,8 +8,6 @@
 
 #pragma once
 
-#include <folly/io/async/DelayedDestruction.h>
-
 #include <quic/QuicConstants.h>
 #include <quic/codec/PacketNumberCipher.h>
 #include <quic/codec/Types.h>
@@ -20,13 +18,12 @@ constexpr folly::StringPiece kQuicKeyLabel = "quic key";
 constexpr folly::StringPiece kQuicIVLabel = "quic iv";
 constexpr folly::StringPiece kQuicPNLabel = "quic hp";
 
-class Handshake : public folly::DelayedDestruction {
+class Handshake {
  public:
+  virtual ~Handshake() = default;
+
   virtual const folly::Optional<std::string>& getApplicationProtocol()
       const = 0;
-
- protected:
-  virtual ~Handshake() = default;
 };
 
 constexpr folly::StringPiece kQuicDraft17Salt =

quic/server/QuicServerTransport.cpp

@@ -24,7 +24,7 @@ QuicServerTransport::QuicServerTransport(
   auto tempConn = std::make_unique<QuicServerConnectionState>();
   tempConn->serverAddr = socket_->address();
   serverConn_ = tempConn.get();
-  conn_ = std::move(tempConn);
+  conn_.reset(tempConn.release());
   // TODO: generate this when we can encode the packet sequence number
   // correctly.
   // conn_->nextSequenceNum = folly::Random::secureRandom<PacketNum>();

quic/server/handshake/ServerHandshake.cpp

@@ -8,14 +8,20 @@
 
 #include <quic/server/handshake/ServerHandshake.h>
 
-#include <fizz/protocol/Protocol.h>
 #include <quic/fizz/handshake/FizzBridge.h>
 #include <quic/fizz/handshake/FizzCryptoFactory.h>
 #include <quic/state/QuicStreamFunctions.h>
 
+#include <fizz/protocol/Protocol.h>
+
 namespace quic {
-ServerHandshake::ServerHandshake(QuicCryptoState& cryptoState)
-    : actionGuard_(nullptr), cryptoState_(cryptoState), visitor_(*this) {}
+ServerHandshake::ServerHandshake(
+    QuicConnectionStateBase* conn,
+    QuicCryptoState& cryptoState)
+    : conn_(conn),
+      actionGuard_(nullptr),
+      cryptoState_(cryptoState),
+      visitor_(*this) {}
 
 void ServerHandshake::accept(
     std::shared_ptr<ServerTransportParametersExtension> transportParams) {
@@ -238,7 +244,7 @@ void ServerHandshake::addProcessingActions(fizz::server::AsyncActions actions) {
         "Processing action while pending", TransportErrorCode::INTERNAL_ERROR));
     return;
   }
-  actionGuard_ = folly::DelayedDestruction::DestructorGuard(this);
+  actionGuard_ = folly::DelayedDestruction::DestructorGuard(conn_);
   startActions(std::move(actions));
 }
 
@@ -257,7 +263,7 @@ void ServerHandshake::processActions(
     fizz::server::ServerStateMachine::CompletedActions actions) {
   // This extra DestructorGuard is needed due to the gap between clearing
   // actionGuard_ and potentially processing another action.
-  folly::DelayedDestruction::DestructorGuard dg(this);
+  folly::DelayedDestruction::DestructorGuard dg(conn_);
 
   for (auto& action : actions) {
     switch (action.type()) {
@@ -307,7 +313,7 @@ void ServerHandshake::processPendingEvents() {
     return;
   }
 
-  folly::DelayedDestruction::DestructorGuard dg(this);
+  folly::DelayedDestruction::DestructorGuard dg(conn_);
   inProcessPendingEvents_ = true;
   SCOPE_EXIT {
     inProcessPendingEvents_ = false;
@@ -316,7 +322,7 @@ void ServerHandshake::processPendingEvents() {
   while (!actionGuard_ && !error_) {
     folly::Optional<fizz::server::ServerStateMachine::ProcessingActions>
         actions;
-    actionGuard_ = folly::DelayedDestruction::DestructorGuard(this);
+    actionGuard_ = folly::DelayedDestruction::DestructorGuard(conn_);
     if (!waitForData_) {
       switch (state_.readRecordLayer()->getEncryptionLevel()) {
         case fizz::EncryptionLevel::Plaintext:

quic/server/handshake/ServerHandshake.h

@@ -26,6 +26,8 @@
 
 namespace quic {
 
+// struct QuicConnectionStateBase;
+
 /**
  * ServerHandshake abstracts details of the TLS 1.3 fizz crypto handshake. The
  * TLS handshake can be async, so ServerHandshake provides an API to deal with
@@ -69,7 +71,9 @@ class ServerHandshake : public Handshake {
    */
   enum class Phase { Handshake, KeysDerived, Established };
 
-  explicit ServerHandshake(QuicCryptoState& cryptoState);
+  explicit ServerHandshake(
+      QuicConnectionStateBase* conn,
+      QuicCryptoState& cryptoState);
 
   /**
    * Starts accepting the TLS connection.
@@ -247,6 +251,7 @@ class ServerHandshake : public Handshake {
 
   fizz::server::State state_;
   fizz::server::ServerStateMachine machine_;
+  QuicConnectionStateBase* conn_;
   folly::DelayedDestruction::DestructorGuard actionGuard_;
   folly::Executor* executor_;
   std::shared_ptr<const fizz::server::FizzServerContext> context_;

quic/server/handshake/test/ServerHandshakeTest.cpp

@@ -48,10 +48,8 @@ class MockServerHandshakeCallback : public ServerHandshake::HandshakeCallback {
   GMOCK_METHOD0_(, noexcept, , onCryptoEventAvailable, void());
 };
 
-class TestingServerHandshake : public ServerHandshake {
- public:
-  explicit TestingServerHandshake(QuicCryptoState& cryptoState)
-      : ServerHandshake(cryptoState) {}
+struct TestingServerConnectionState : public QuicServerConnectionState {
+  explicit TestingServerConnectionState() : QuicServerConnectionState() {}
 
   uint32_t getDestructorGuardCount() const {
     return folly::DelayedDestruction::getDestructorGuardCount();
@@ -74,14 +72,15 @@ class ServerHandshakeTest : public Test {
 
   void SetUp() override {
     folly::ssl::init();
-    cryptoState = std::make_unique<QuicCryptoState>();
+    conn.reset(new TestingServerConnectionState());
+    cryptoState = conn->cryptoState.get();
     clientCtx = std::make_shared<fizz::client::FizzClientContext>();
     clientCtx->setOmitEarlyRecordLayer(true);
     clientCtx->setFactory(std::make_shared<QuicFizzFactory>());
     clientCtx->setClock(std::make_shared<fizz::test::MockClock>());
     serverCtx = quic::test::createServerCtx();
     setupClientAndServerContext();
-    handshake.reset(new TestingServerHandshake(*cryptoState));
+    handshake = conn->serverHandshakeLayer;
     hostname = kTestHostname.str();
     verifier = std::make_shared<fizz::test::MockCertificateVerifier>();
 
@@ -320,8 +319,12 @@ class ServerHandshakeTest : public Test {
   std::unique_ptr<DelayedHolder, folly::DelayedDestruction::Destructor> dg;
 
   folly::EventBase evb;
-  std::unique_ptr<TestingServerHandshake> handshake;
-  std::unique_ptr<QuicCryptoState> cryptoState;
+  std::unique_ptr<
+      TestingServerConnectionState,
+      folly::DelayedDestruction::Destructor>
+      conn{nullptr};
+  ServerHandshake* handshake;
+  QuicCryptoState* cryptoState;
 
   fizz::client::State clientState;
   std::unique_ptr<fizz::client::FizzClient<
@@ -612,7 +615,7 @@ TEST_F(ServerHandshakeHRRTest, TestAsyncCancel) {
 
   handshake->cancel();
   // Let's destroy the crypto state to make sure it is not referenced.
-  cryptoState.reset();
+  conn->cryptoState.reset();
 
   promise.setValue();
   evb.loop();
@@ -641,12 +644,12 @@ TEST_F(ServerHandshakeAsyncTest, TestAsyncCancel) {
 
   handshake->cancel();
   // Let's destroy the crypto state to make sure it is not referenced.
-  cryptoState.reset();
+  conn->cryptoState.reset();
 
   promise.setValue();
   evb.loop();
 
-  EXPECT_EQ(handshake->getDestructorGuardCount(), 0);
+  EXPECT_EQ(conn->getDestructorGuardCount(), 0);
 }
 
 class ServerHandshakeAsyncErrorTest : public ServerHandshakePskTest {
@@ -685,7 +688,7 @@ TEST_F(ServerHandshakeAsyncErrorTest, TestCancelOnAsyncError) {
       .WillRepeatedly(Invoke([&] {
         handshake->cancel();
         // Let's destroy the crypto state to make sure it is not referenced.
-        cryptoState.reset();
+        conn->cryptoState.reset();
       }));
   promise.setValue();
   evb.loop();
@@ -696,7 +699,7 @@ TEST_F(ServerHandshakeAsyncErrorTest, TestCancelWhileWaitingAsyncError) {
   clientServerRound();
   handshake->cancel();
   // Let's destroy the crypto state to make sure it is not referenced.
-  cryptoState.reset();
+  conn->cryptoState.reset();
 
   promise.setValue();
   evb.loop();

quic/server/state/ServerStateMachine.h

@@ -133,7 +133,7 @@ struct QuicServerConnectionState : public QuicConnectionStateBase {
                                                   QuicVersion::QUIC_DRAFT,
                                                   QuicVersion::QUIC_DRAFT_23}};
     originalVersion = QuicVersion::MVFST;
-    serverHandshakeLayer = new ServerHandshake(*cryptoState);
+    serverHandshakeLayer = new ServerHandshake(this, *cryptoState);
     handshakeLayer.reset(serverHandshakeLayer);
     // We shouldn't normally need to set this until we're starting the
     // transport, however writing unit tests is much easier if we set this here.

quic/server/test/QuicServerTransportTest.cpp

@@ -44,7 +44,7 @@ class FakeServerHandshake : public ServerHandshake {
       bool chloSync = false,
       bool cfinSync = false,
       folly::Optional<uint64_t> clientActiveConnectionIdLimit = folly::none)
-      : ServerHandshake(*conn.cryptoState),
+      : ServerHandshake(&conn, *conn.cryptoState),
         conn_(conn),
         chloSync_(chloSync),
         cfinSync_(cfinSync),

quic/state/StateData.h

@@ -8,10 +8,6 @@
 
 #pragma once
 
-#include <folly/Optional.h>
-#include <folly/io/IOBuf.h>
-#include <folly/io/async/AsyncUDPSocket.h>
-#include <folly/io/async/HHWheelTimer.h>
 #include <quic/QuicConstants.h>
 #include <quic/codec/ConnectionIdAlgo.h>
 #include <quic/codec/QuicReadCodec.h>
@@ -25,6 +21,13 @@
 #include <quic/state/QuicTransportStatsCallback.h>
 #include <quic/state/StreamData.h>
 #include <quic/state/TransportSettings.h>
+
+#include <folly/Optional.h>
+#include <folly/io/IOBuf.h>
+#include <folly/io/async/AsyncUDPSocket.h>
+#include <folly/io/async/DelayedDestruction.h>
+#include <folly/io/async/HHWheelTimer.h>
+
 #include <chrono>
 #include <list>
 #include <numeric>
@@ -489,7 +492,7 @@ class CongestionControllerFactory;
 class LoopDetectorCallback;
 class PendingPathRateLimiter;
 
-struct QuicConnectionStateBase {
+struct QuicConnectionStateBase : public folly::DelayedDestruction {
   virtual ~QuicConnectionStateBase() = default;
 
   explicit QuicConnectionStateBase(QuicNodeType type) : nodeType(type) {}
@@ -497,8 +500,7 @@ struct QuicConnectionStateBase {
   // Type of node owning this connection (client or server).
   QuicNodeType nodeType;
 
-  std::unique_ptr<Handshake, folly::DelayedDestruction::Destructor>
-      handshakeLayer;
+  std::unique_ptr<Handshake> handshakeLayer;
 
   // Crypto stream
   std::unique_ptr<QuicCryptoState> cryptoState;