From 206d0ced8571c9bf74f46db45ae25da329344c0f Mon Sep 17 00:00:00 2001
From: Xavier Deguillard <xavierd@fb.com>
Date: Mon, 22 Feb 2021 22:38:29 -0800
Subject: [PATCH] nfs: fix RpcServer lifetime

Summary:
Take 2 of fixing the lifetime issues of the RpcServer, this time, the
RpcAcceptCallback needs to live for longer than RpcServer itself. This is due
to the acceptStopped callback being called after RpcServer is being destroyed.
Since the RpcServer needs to be destroyed in the EventBase thread, the
acceptStopped callback is delayed, causing it to be invoked after the RpcServer
has freed its memory.

To solve this, we simply need to make the RpcAcceptCallback a delayed
destructor, and hold a reference to itself that goes away once the
acceptStopped callback is called.

Reviewed By: kmancini

Differential Revision: D26556644

fbshipit-source-id: 32cd80f1664649c1ad96f88c4eec6dd2ed6e8c12
---
 eden/fs/nfs/rpc/Server.cpp | 19 ++++++++++---
 eden/fs/nfs/rpc/Server.h   | 57 +++++++++++++++++++++++---------------
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/eden/fs/nfs/rpc/Server.cpp b/eden/fs/nfs/rpc/Server.cpp
index 4a43ffbaa6..14acf7bb29 100644
--- a/eden/fs/nfs/rpc/Server.cpp
+++ b/eden/fs/nfs/rpc/Server.cpp
@@ -181,7 +181,7 @@ class RpcTcpHandler : public folly::DelayedDestruction {
   }
 
   std::shared_ptr<RpcServerProcessor> proc_;
-  std::unique_ptr<AsyncSocket, ReleasableDestructor> sock_;
+  AsyncSocket::UniquePtr sock_;
   std::unique_ptr<Reader> reader_;
   Writer writer_;
   folly::IOBufQueue readBuf_;
@@ -268,6 +268,17 @@ void RpcServer::RpcAcceptCallback::connectionAccepted(
   handler->setup();
 }
 
+void RpcServer::RpcAcceptCallback::acceptError(
+    const std::exception& ex) noexcept {
+  XLOG(ERR) << "acceptError: " << folly::exceptionStr(ex);
+}
+
+void RpcServer::RpcAcceptCallback::acceptStopped() noexcept {
+  // We won't ever be accepting any connection, it is now safe to delete
+  // ourself, release the guard.
+  guard_.reset();
+}
+
 auth_stat RpcServerProcessor::checkAuthentication(
     const call_body& /*call_body*/) {
   // Completely ignore authentication.
@@ -289,13 +300,13 @@ RpcServer::RpcServer(
     std::shared_ptr<RpcServerProcessor> proc,
     folly::EventBase* evb)
     : evb_(evb),
-      acceptCb_(proc, evb_),
-      serverSocket_(AsyncServerSocket::newSocket(evb_)) {
+      acceptCb_(new RpcServer::RpcAcceptCallback(proc, evb_)),
+      serverSocket_(new AsyncServerSocket(evb_)) {
   // Ask kernel to assign us a port on the loopback interface
   serverSocket_->bind(SocketAddress("127.0.0.1", 0));
   serverSocket_->listen(1024);
 
-  serverSocket_->addAcceptCallback(&acceptCb_, evb_);
+  serverSocket_->addAcceptCallback(acceptCb_.get(), evb_);
   serverSocket_->startAccepting();
 }
 
diff --git a/eden/fs/nfs/rpc/Server.h b/eden/fs/nfs/rpc/Server.h
index b055bdbaac..c7bf2ec7e7 100644
--- a/eden/fs/nfs/rpc/Server.h
+++ b/eden/fs/nfs/rpc/Server.h
@@ -11,7 +11,6 @@
 
 #include <folly/SocketAddress.h>
 #include <folly/io/async/AsyncServerSocket.h>
-#include <folly/logging/xlog.h>
 #include <folly/net/NetworkSocket.h>
 #include "eden/fs/nfs/portmap/PortmapClient.h"
 #include "eden/fs/nfs/rpc/Rpc.h"
@@ -32,26 +31,6 @@ class RpcServerProcessor {
 };
 
 class RpcServer {
-  class RpcAcceptCallback : public folly::AsyncServerSocket::AcceptCallback {
-   public:
-    explicit RpcAcceptCallback(
-        std::shared_ptr<RpcServerProcessor> proc,
-        folly::EventBase* evb)
-        : evb_(evb), proc_(proc) {}
-
-    void connectionAccepted(
-        folly::NetworkSocket fd,
-        const folly::SocketAddress& clientAddr) noexcept override;
-
-    void acceptError(const std::exception& ex) noexcept override {
-      XLOG(ERR) << "acceptError: " << folly::exceptionStr(ex);
-    }
-
-   private:
-    folly::EventBase* evb_;
-    std::shared_ptr<RpcServerProcessor> proc_;
-  };
-
  public:
   RpcServer(std::shared_ptr<RpcServerProcessor> proc, folly::EventBase* evb);
   ~RpcServer();
@@ -71,9 +50,41 @@ class RpcServer {
   uint16_t getPort() const;
 
  private:
+  class RpcAcceptCallback : public folly::AsyncServerSocket::AcceptCallback,
+                            public folly::DelayedDestruction {
+   public:
+    using UniquePtr = std::
+        unique_ptr<RpcAcceptCallback, folly::DelayedDestruction::Destructor>;
+
+    explicit RpcAcceptCallback(
+        std::shared_ptr<RpcServerProcessor> proc,
+        folly::EventBase* evb)
+        : evb_(evb), proc_(proc), guard_(this) {}
+
+   private:
+    void connectionAccepted(
+        folly::NetworkSocket fd,
+        const folly::SocketAddress& clientAddr) noexcept override;
+
+    void acceptError(const std::exception& ex) noexcept override;
+
+    void acceptStopped() noexcept override;
+
+    ~RpcAcceptCallback() override = default;
+
+    folly::EventBase* evb_;
+    std::shared_ptr<RpcServerProcessor> proc_;
+
+    /**
+     * Hold a guard to ourself to avoid being deleted until the callback is
+     * removed from the AsyncServerSocket.
+     */
+    std::optional<folly::DelayedDestruction::DestructorGuard> guard_;
+  };
+
   folly::EventBase* evb_;
-  RpcAcceptCallback acceptCb_;
-  std::shared_ptr<folly::AsyncServerSocket> serverSocket_;
+  RpcAcceptCallback::UniquePtr acceptCb_;
+  folly::AsyncServerSocket::UniquePtr serverSocket_;
   PortmapClient portMap_;
   std::vector<PortmapMapping> mappedPorts_;
   std::shared_ptr<RpcServerProcessor> proc_;