Use a deterministic executor in TestMount

Summary:
As a prerequisite to running inode unloading code in a background
queue, use a deterministic executor in TestMount to make sequencing in
unit tests reliable.

Reviewed By: simpkins

Differential Revision: D9323878

fbshipit-source-id: 0b85632c1637a8cf83d6f238675e5b6bbb6923c7

eden/fs/inodes/EdenMount.cpp

@@ -167,8 +167,8 @@ EdenMount::EdenMount(
     std::unique_ptr<ClientConfig> config,
     std::unique_ptr<ObjectStore> objectStore,
     std::shared_ptr<ServerState> serverState)
-    : serverState_(std::move(serverState)),
-      config_(std::move(config)),
+    : config_(std::move(config)),
+      serverState_(std::move(serverState)),
       inodeMap_{new InodeMap(this)},
       dispatcher_{new EdenDispatcher(this)},
       objectStore_(std::move(objectStore)),
@@ -747,7 +747,7 @@ void EdenMount::fuseInitSuccessful(
             ));
       })
       .onError([this](folly::exception_wrapper&& ew) {
-        XLOG(ERR) << "session complete with err" << ew;
+        XLOG(ERR) << "session complete with err: " << ew.what();
         fuseCompletionPromise_.setException(std::move(ew));
       });
 }

eden/fs/inodes/EdenMount.h

@@ -567,12 +567,26 @@ class EdenMount {
 
   static constexpr int kMaxSymlinkChainDepth = 40; // max depth of symlink chain
 
+  const std::unique_ptr<const ClientConfig> config_;
+
+  /**
+   * A promise associated with the future returned from
+   * EdenMount::getFuseCompletionFuture() that completes when the
+   * fuseChannel has no work remaining and can be torn down.
+   * The future yields the underlying fuseDevice descriptor; it can
+   * be passed on during graceful restart or simply closed if we're
+   * unmounting and shutting down completely.  In the unmount scenario
+   * the device should be closed prior to calling EdenMount::shutdown()
+   * so that the subsequent privilegedFuseUnmount() call won't block
+   * waiting on us for a response.
+   */
+  folly::Promise<TakeoverData::MountInfo> fuseCompletionPromise_;
+
   /**
    * Eden server state shared across multiple mount points.
    */
   std::shared_ptr<ServerState> serverState_;
 
-  const std::unique_ptr<const ClientConfig> config_;
   std::unique_ptr<InodeMap> inodeMap_;
   std::unique_ptr<EdenDispatcher> dispatcher_;
   std::unique_ptr<ObjectStore> objectStore_;
@@ -645,19 +659,6 @@ class EdenMount {
    */
   std::atomic<State> state_{State::UNINITIALIZED};
 
-  /**
-   * A promise associated with the future returned from
-   * EdenMount::getFuseCompletionFuture() that completes when the
-   * fuseChannel has no work remaining and can be torn down.
-   * The future yields the underlying fuseDevice descriptor; it can
-   * be passed on during graceful restart or simply closed if we're
-   * unmounting and shutting down completely.  In the unmount scenario
-   * the device should be closed prior to calling EdenMount::shutdown()
-   * so that the subsequent privilegedFuseUnmount() call won't block
-   * waiting on us for a response.
-   */
-  folly::Promise<TakeoverData::MountInfo> fuseCompletionPromise_;
-
   /**
    * uid and gid that we'll set as the owners in the stat information
    * returned via initStatData().

eden/fs/inodes/test/FuseTest.cpp

@@ -8,11 +8,13 @@
  *
  */
 #include <gtest/gtest.h>
+#include <chrono>
 #include "eden/fs/testharness/FakeFuse.h"
 #include "eden/fs/testharness/FakeTreeBuilder.h"
 #include "eden/fs/testharness/TestMount.h"
 #include "eden/fs/utils/UnboundedQueueExecutor.h"
 
+#include <folly/executors/ManualExecutor.h>
 #include <folly/io/async/EventBase.h>
 #include <folly/io/async/ScopedEventBaseThread.h>
 #include <folly/logging/xlog.h>
@@ -25,6 +27,17 @@ using folly::ScopedEventBaseThread;
 using folly::Unit;
 using std::make_shared;
 
+namespace {
+
+/**
+ * The FUSE tests wait for work to finish on a thread pool. 250ms is too short
+ * for the test to reliably test under heavy system load (such as when stress
+ * testing), so wait for 10 seconds.
+ */
+constexpr std::chrono::seconds kWaitTimeout = 10s;
+
+} // namespace
+
 TEST(FuseTest, initMount) {
   auto builder1 = FakeTreeBuilder();
   builder1.setFile("src/main.c", "int main() { return 0; }\n");
@@ -55,13 +68,30 @@ TEST(FuseTest, initMount) {
       sizeof(fuse_out_header) + sizeof(fuse_init_out), response.header.len);
 
   // Wait for the mount to complete
-  std::move(initFuture).get(250ms);
+  testMount.drainServerExecutor();
+  std::move(initFuture).get(kWaitTimeout);
 
   // Close the FakeFuse device, and ensure that the mount's FUSE completion
   // future is then signalled.
   fuse->close();
-  auto mountInfo =
-      testMount.getEdenMount()->getFuseCompletionFuture().get(100ms);
+
+  auto fuseCompletionFuture =
+      testMount.getEdenMount()->getFuseCompletionFuture();
+
+  // TestMount has a manual executor, but the fuse channel thread enqueues
+  // the work. Wait for the future to complete, driving the ManualExecutor
+  // all the while.
+  // TODO: It might be worth moving this logic into a TestMount::waitFuture
+  // method.
+  auto deadline = std::chrono::steady_clock::now() + kWaitTimeout;
+  do {
+    if (std::chrono::steady_clock::now() > deadline) {
+      FAIL() << "fuse completion future not ready within timeout";
+    }
+    testMount.drainServerExecutor();
+  } while (!fuseCompletionFuture.isReady());
+
+  auto mountInfo = std::move(fuseCompletionFuture.value());
 
   // Since we closed the FUSE device from the "kernel" side the returned
   // MountInfo should not contain a valid FUSE device any more.
@@ -107,7 +137,7 @@ TEST(FuseTest, destroyWithInitRace) {
   auto initFuture = Future<Unit>::makeEmpty();
   auto completionFuture = Future<TakeoverData::MountInfo>::makeEmpty();
   {
-    // Create the TestMountj
+    // Create the TestMount.
     TestMount testMount{builder1};
     testMount.registerFakeFuse(fuse);
 

eden/fs/testharness/TestMount.cpp

@@ -10,6 +10,7 @@
 #include "TestMount.h"
 
 #include <folly/FileUtil.h>
+#include <folly/executors/ManualExecutor.h>
 #include <folly/experimental/TestUtil.h>
 #include <folly/io/IOBuf.h>
 #include <folly/logging/xlog.h>
@@ -70,7 +71,9 @@ bool TestMountFile::operator==(const TestMountFile& other) const {
       type == other.type;
 }
 
-TestMount::TestMount() : privHelper_{make_shared<FakePrivHelper>()} {
+TestMount::TestMount()
+    : privHelper_{make_shared<FakePrivHelper>()},
+      serverExecutor_{make_shared<folly::ManualExecutor>()} {
   // Initialize the temporary directory.
   // This sets both testDir_, config_, localStore_, and backingStore_
   initTestDirectory();
@@ -78,8 +81,7 @@ TestMount::TestMount() : privHelper_{make_shared<FakePrivHelper>()} {
   serverState_ = {make_shared<ServerState>(
       UserInfo::lookup(),
       privHelper_,
-      make_shared<UnboundedQueueExecutor>(
-          FLAGS_num_eden_test_threads, "EdenCPUThread"),
+      make_shared<UnboundedQueueExecutor>(serverExecutor_),
       clock_,
       make_shared<EdenConfig>(
           /*userName=*/folly::StringPiece{"bob"},
@@ -110,6 +112,11 @@ TestMount::~TestMount() {
   // keeps the EdenMount alive, causing the test to leak.
   // Manually release the futures in FakeBackingStore.
   backingStore_->discardOutstandingRequests();
+
+  // Make sure the server executor has nothing left to run.
+  serverExecutor_->drain();
+
+  CHECK_EQ(0, serverExecutor_->clear());
 }
 
 void TestMount::initialize(
@@ -287,6 +294,10 @@ bool TestMount::hasMetadata(InodeNumber ino) const {
   return edenMount_->getInodeMetadataTable()->getOptional(ino).has_value();
 }
 
+size_t TestMount::drainServerExecutor() {
+  return serverExecutor_->drain();
+}
+
 void TestMount::setInitialCommit(Hash commitHash) {
   // Write the commit hash to the snapshot file
   auto snapshotPath = config_->getSnapshotPath();

eden/fs/testharness/TestMount.h

@@ -26,6 +26,7 @@ namespace folly {
 template <typename T>
 class Future;
 struct Unit;
+class ManualExecutor;
 } // namespace folly
 
 namespace facebook {
@@ -275,6 +276,15 @@ class TestMount {
    */
   bool hasMetadata(InodeNumber inodeNumber) const;
 
+  /**
+   * Returns number of functions executed.
+   */
+  size_t drainServerExecutor();
+
+  std::shared_ptr<folly::ManualExecutor> getServerExecutor() const {
+    return serverExecutor_;
+  }
+
  private:
   void initTestDirectory();
   void setInitialCommit(Hash commitHash);
@@ -313,6 +323,11 @@ class TestMount {
   std::shared_ptr<FakeClock> clock_ = std::make_shared<FakeClock>();
   std::shared_ptr<FakePrivHelper> privHelper_;
 
+  // The ManualExecutor must be destroyed prior to the EdenMount. Otherwise,
+  // when clearing its queue, it will deallocate functions with captured values
+  // that still reference the EdenMount (or its owned objects).
+  std::shared_ptr<folly::ManualExecutor> serverExecutor_;
+
   std::shared_ptr<ServerState> serverState_;
 };
 } // namespace eden

eden/fs/utils/UnboundedQueueExecutor.cpp

@@ -27,7 +27,7 @@ UnboundedQueueExecutor::UnboundedQueueExecutor(
           std::make_unique<folly::NamedThreadFactory>(threadNamePrefix))} {}
 
 UnboundedQueueExecutor::UnboundedQueueExecutor(
-    std::unique_ptr<folly::ManualExecutor> executor)
+    std::shared_ptr<folly::ManualExecutor> executor)
     : executor_{std::move(executor)} {}
 
 } // namespace eden

eden/fs/utils/UnboundedQueueExecutor.h

@@ -42,7 +42,7 @@ class UnboundedQueueExecutor : public folly::Executor {
    * Used primarily for tests.
    */
   explicit UnboundedQueueExecutor(
-      std::unique_ptr<folly::ManualExecutor> executor);
+      std::shared_ptr<folly::ManualExecutor> executor);
 
   UnboundedQueueExecutor(const UnboundedQueueExecutor&) = delete;
   UnboundedQueueExecutor& operator=(const UnboundedQueueExecutor&) = delete;
@@ -54,7 +54,7 @@ class UnboundedQueueExecutor : public folly::Executor {
   }
 
  private:
-  std::unique_ptr<folly::Executor> executor_;
+  std::shared_ptr<folly::Executor> executor_;
 };
 
 } // namespace eden