Use RAII std::lock_guard pattern in Futures FSM and Core

Summary: [Folly] Use RAII `std::lock_guard` pattern in Futures `FSM` and `Core`.

Reviewed By: LeeHowes

Differential Revision: D7722808

fbshipit-source-id: e8a0d0ec1fee05cd723411f7b07f520dd56cdf22

folly/futures/detail/Core.h

@@ -59,6 +59,19 @@ enum class State : uint8_t {
   Done,
 };
 
+/// SpinLock is and must stay a 1-byte object because of how Core is laid out.
+struct SpinLock : private MicroSpinLock {
+  SpinLock() : MicroSpinLock{0} {}
+
+  void lock() {
+    if (!MicroSpinLock::try_lock()) {
+      MicroSpinLock::lock();
+    }
+  }
+  using MicroSpinLock::unlock;
+};
+static_assert(sizeof(SpinLock) == 1, "missized");
+
 /// The shared state object for Future and Promise.
 /// Some methods must only be called by either the Future thread or the
 /// Promise thread. The Future thread is the thread that currently "owns" the
@@ -242,35 +255,26 @@ class Core final {
 
   /// Call only from Future thread
   void raise(exception_wrapper e) {
-    if (!interruptLock_.try_lock()) {
-      interruptLock_.lock();
-    }
+    std::lock_guard<SpinLock> lock(interruptLock_);
     if (!interrupt_ && !hasResult()) {
       interrupt_ = std::make_unique<exception_wrapper>(std::move(e));
       if (interruptHandler_) {
         interruptHandler_(*interrupt_);
       }
     }
-    interruptLock_.unlock();
   }
 
   std::function<void(exception_wrapper const&)> getInterruptHandler() {
     if (!interruptHandlerSet_.load(std::memory_order_acquire)) {
       return nullptr;
     }
-    if (!interruptLock_.try_lock()) {
-      interruptLock_.lock();
-    }
-    auto handler = interruptHandler_;
-    interruptLock_.unlock();
-    return handler;
+    std::lock_guard<SpinLock> lock(interruptLock_);
+    return interruptHandler_;
   }
 
   /// Call only from Promise thread
   void setInterruptHandler(std::function<void(exception_wrapper const&)> fn) {
-    if (!interruptLock_.try_lock()) {
-      interruptLock_.lock();
-    }
+    std::lock_guard<SpinLock> lock(interruptLock_);
     if (!hasResult()) {
       if (interrupt_) {
         fn(*interrupt_);
@@ -278,7 +282,6 @@ class Core final {
         setInterruptHandlerNoLock(std::move(fn));
       }
     }
-    interruptLock_.unlock();
   }
 
   void setInterruptHandlerNoLock(
@@ -406,12 +409,12 @@ class Core final {
   // place result_ next to increase the likelihood that the value will be
   // contained entirely in one cache line
   folly::Optional<Try<T>> result_;
-  FSM<State> fsm_;
+  FSM<State, SpinLock> fsm_;
   std::atomic<unsigned char> attached_;
   std::atomic<unsigned char> callbackReferences_{0};
   std::atomic<bool> active_ {true};
   std::atomic<bool> interruptHandlerSet_ {false};
-  folly::MicroSpinLock interruptLock_ {0};
+  SpinLock interruptLock_;
   int8_t priority_ {-1};
   Executor* executor_ {nullptr};
   std::shared_ptr<RequestContext> context_ {nullptr};

folly/futures/detail/FSM.h

@@ -28,14 +28,10 @@ namespace detail {
 /// Finite State Machine helper base class.
 /// Inherit from this.
 /// For best results, use an "enum class" for Enum.
-template <class Enum>
+template <class Enum, class Mutex>
 class FSM {
  private:
-  // I am not templatizing this because folly::MicroSpinLock needs to be
-  // zero-initialized (or call init) which isn't generic enough for something
-  // that behaves like std::mutex. :(
-  using Mutex = folly::MicroSpinLock;
-  Mutex mutex_ {0};
+  Mutex mutex_;
 
   // This might not be necessary for all Enum types, e.g. anything
   // that is atomically updated in practice on this CPU and there's no risk
@@ -55,16 +51,12 @@ class FSM {
   /// @returns true on success, false and action unexecuted otherwise
   template <class F>
   bool updateState(Enum A, Enum B, F const& action) {
-    if (!mutex_.try_lock()) {
-      mutex_.lock();
-    }
+    std::lock_guard<Mutex> lock(mutex_);
     if (state_.load(std::memory_order_acquire) != A) {
-      mutex_.unlock();
       return false;
     }
     action();
     state_.store(B, std::memory_order_release);
-    mutex_.unlock();
     return true;
   }
 

folly/futures/test/CoreTest.cpp

@@ -26,7 +26,8 @@ TEST(Core, size) {
     typename std::aligned_storage<lambdaBufSize>::type lambdaBuf_;
     folly::Optional<Try<Unit>> result_;
     std::function<void(Try<Unit>&&)> callback_;
-    futures::detail::FSM<futures::detail::State> fsm_;
+    futures::detail::FSM<futures::detail::State, futures::detail::SpinLock>
+        fsm_;
     std::atomic<unsigned char> attached_;
     std::atomic<bool> active_;
     std::atomic<bool> interruptHandlerSet_;

folly/futures/test/FSMTest.cpp

@@ -22,7 +22,7 @@ using namespace folly::futures::detail;
 enum class State { A, B };
 
 TEST(FSM, example) {
-  FSM<State> fsm(State::A);
+  FSM<State, std::mutex> fsm(State::A);
   int count = 0;
   int unprotectedCount = 0;
 
@@ -56,7 +56,7 @@ TEST(FSM, example) {
 
 TEST(FSM, magicMacrosExample) {
   struct MyFSM {
-    FSM<State> fsm_;
+    FSM<State, std::mutex> fsm_;
     int count = 0;
     int unprotectedCount = 0;
     MyFSM() : fsm_(State::A) {}
@@ -84,37 +84,37 @@ TEST(FSM, magicMacrosExample) {
 
 
 TEST(FSM, ctor) {
-  FSM<State> fsm(State::A);
+  FSM<State, std::mutex> fsm(State::A);
   EXPECT_EQ(State::A, fsm.getState());
 }
 
 TEST(FSM, update) {
-  FSM<State> fsm(State::A);
+  FSM<State, std::mutex> fsm(State::A);
   EXPECT_TRUE(fsm.updateState(State::A, State::B, []{}));
   EXPECT_EQ(State::B, fsm.getState());
 }
 
 TEST(FSM, badUpdate) {
-  FSM<State> fsm(State::A);
+  FSM<State, std::mutex> fsm(State::A);
   EXPECT_FALSE(fsm.updateState(State::B, State::A, []{}));
 }
 
 TEST(FSM, actionOnUpdate) {
-  FSM<State> fsm(State::A);
+  FSM<State, std::mutex> fsm(State::A);
   int count = 0;
   fsm.updateState(State::A, State::B, [&]{ count++; });
   EXPECT_EQ(1, count);
 }
 
 TEST(FSM, noActionOnBadUpdate) {
-  FSM<State> fsm(State::A);
+  FSM<State, std::mutex> fsm(State::A);
   int count = 0;
   fsm.updateState(State::B, State::A, [&]{ count++; });
   EXPECT_EQ(0, count);
 }
 
 TEST(FSM, stateTransitionAfterAction) {
-  FSM<State> fsm(State::A);
+  FSM<State, std::mutex> fsm(State::A);
   fsm.updateState(State::A, State::B,
                   [&]{ EXPECT_EQ(State::A, fsm.getState()); });
 }