From: Hans Fugal <fugalh@fb.com>
Date: Mon, 30 Jun 2014 20:38:41 +0000 (-0700)
Subject: (wangle) Future/Promise detachment
X-Git-Tag: v0.22.0~476
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=03f412789821e5c1892dea9c887e0ecd63e1c705;p=folly.git

(wangle) Future/Promise detachment

Summary:
Bring a bit more sanity to the lifetime. Now Future and Promise detach by calling the respective methods on State, and they do it during their destruction only. State manages its own lifetime.

Besides being cleaner, this also sets the stage for cancellation (where we'll need Future to hang on to its reference to State after setting the callback), and for folding in Later (we will have a bool for whether the future is hot and hold off executing the callback if it isn't).

Also cleans up various things I noticed while auditing the code for usages of `state_`.

Test Plan:
All the unit tests still pass.
Ran with ASAN (and found I had introduced a bug, then fixed it. yay)

Reviewed By: hannesr@fb.com

Subscribers: jsedgwick, net-systems@, fugalh, exa

FB internal diff: D1412038

Tasks: 4618297
---

diff --git a/folly/wangle/Future-inl.h b/folly/wangle/Future-inl.h
index c3002f48..1e17e7cb 100644
--- a/folly/wangle/Future-inl.h
+++ b/folly/wangle/Future-inl.h
@@ -32,8 +32,8 @@ struct isFuture<Future<T> > {
 };
 
 template <class T>
-Future<T>::Future(Future<T>&& other) noexcept : state_(other.state_) {
-  other.state_ = nullptr;
+Future<T>::Future(Future<T>&& other) noexcept : state_(nullptr) {
+  *this = std::move(other);
 }
 
 template <class T>
@@ -44,8 +44,14 @@ Future<T>& Future<T>::operator=(Future<T>&& other) {
 
 template <class T>
 Future<T>::~Future() {
+  detach();
+}
+
+template <class T>
+void Future<T>::detach() {
   if (state_) {
-    setCallback_([](Try<T>&&) {}); // detach
+    state_->detachFuture();
+    state_ = nullptr;
   }
 }
 
@@ -60,7 +66,6 @@ template <class F>
 void Future<T>::setCallback_(F&& func) {
   throwIfInvalid();
   state_->setCallback(std::move(func));
-  state_ = nullptr;
 }
 
 template <class T>
diff --git a/folly/wangle/Future.h b/folly/wangle/Future.h
index fcec4353..e468eebe 100644
--- a/folly/wangle/Future.h
+++ b/folly/wangle/Future.h
@@ -189,6 +189,8 @@ class Future {
   explicit
   Future(statePtr obj) : state_(obj) {}
 
+  void detach();
+
   void throwIfInvalid() const;
 
   friend class Promise<T>;
diff --git a/folly/wangle/Promise-inl.h b/folly/wangle/Promise-inl.h
index 190d9cd0..ef6bdb41 100644
--- a/folly/wangle/Promise-inl.h
+++ b/folly/wangle/Promise-inl.h
@@ -29,9 +29,8 @@ Promise<T>::Promise() : retrieved_(false), state_(new detail::State<T>())
 {}
 
 template <class T>
-Promise<T>::Promise(Promise<T>&& other) :
-retrieved_(other.retrieved_), state_(other.state_) {
-  other.state_ = nullptr;
+Promise<T>::Promise(Promise<T>&& other) : state_(nullptr) {
+  *this = std::move(other);
 }
 
 template <class T>
@@ -44,6 +43,8 @@ Promise<T>& Promise<T>::operator=(Promise<T>&& other) {
 template <class T>
 void Promise<T>::throwIfFulfilled() {
   if (!state_)
+    throw NoState();
+  if (state_->ready())
     throw PromiseAlreadySatisfied();
 }
 
@@ -55,8 +56,16 @@ void Promise<T>::throwIfRetrieved() {
 
 template <class T>
 Promise<T>::~Promise() {
+  detach();
+}
+
+template <class T>
+void Promise<T>::detach() {
   if (state_) {
-    setException(BrokenPromise());
+    if (!retrieved_)
+      state_->detachFuture();
+    state_->detachPromise();
+    state_ = nullptr;
   }
 }
 
@@ -71,7 +80,6 @@ Future<T> Promise<T>::getFuture() {
 template <class T>
 template <class E>
 void Promise<T>::setException(E const& e) {
-  throwIfFulfilled();
   setException(std::make_exception_ptr<E>(e));
 }
 
@@ -79,20 +87,12 @@ template <class T>
 void Promise<T>::setException(std::exception_ptr const& e) {
   throwIfFulfilled();
   state_->setException(e);
-  if (!retrieved_) {
-    delete state_;
-  }
-  state_ = nullptr;
 }
 
 template <class T>
 void Promise<T>::fulfilTry(Try<T>&& t) {
   throwIfFulfilled();
   state_->fulfil(std::move(t));
-  if (!retrieved_) {
-    delete state_;
-  }
-  state_ = nullptr;
 }
 
 template <class T>
@@ -101,12 +101,7 @@ void Promise<T>::setValue(M&& v) {
   static_assert(!std::is_same<T, void>::value,
                 "Use setValue() instead");
 
-  throwIfFulfilled();
-  state_->fulfil(Try<T>(std::forward<M>(v)));
-  if (!retrieved_) {
-    delete state_;
-  }
-  state_ = nullptr;
+  fulfilTry(Try<T>(std::forward<M>(v)));
 }
 
 template <class T>
@@ -114,47 +109,14 @@ void Promise<T>::setValue() {
   static_assert(std::is_same<T, void>::value,
                 "Use setValue(value) instead");
 
-  throwIfFulfilled();
-  state_->fulfil(Try<void>());
-  if (!retrieved_) {
-    delete state_;
-  }
-  state_ = nullptr;
+  fulfilTry(Try<void>());
 }
 
 template <class T>
 template <class F>
 void Promise<T>::fulfil(F&& func) {
-  fulfilHelper(std::forward<F>(func));
-}
-
-template <class T>
-template <class F>
-typename std::enable_if<
-  std::is_convertible<typename std::result_of<F()>::type, T>::value &&
-  !std::is_same<T, void>::value>::type
-inline Promise<T>::fulfilHelper(F&& func) {
-  throwIfFulfilled();
-  try {
-    setValue(func());
-  } catch (...) {
-    setException(std::current_exception());
-  }
-}
-
-template <class T>
-template <class F>
-typename std::enable_if<
-  std::is_same<typename std::result_of<F()>::type, void>::value &&
-  std::is_same<T, void>::value>::type
-inline Promise<T>::fulfilHelper(F&& func) {
   throwIfFulfilled();
-  try {
-    func();
-    setValue();
-  } catch (...) {
-    setException(std::current_exception());
-  }
+  fulfilTry(makeTryFunction(std::forward<F>(func)));
 }
 
 }}
diff --git a/folly/wangle/Promise.h b/folly/wangle/Promise.h
index c47cc98e..62e6f6d8 100644
--- a/folly/wangle/Promise.h
+++ b/folly/wangle/Promise.h
@@ -86,18 +86,7 @@ private:
 
   void throwIfFulfilled();
   void throwIfRetrieved();
-
-  template <class F>
-  typename std::enable_if<
-    std::is_convertible<typename std::result_of<F()>::type, T>::value &&
-    !std::is_same<T, void>::value>::type
-  fulfilHelper(F&& func);
-
-  template <class F>
-  typename std::enable_if<
-    std::is_same<typename std::result_of<F()>::type, void>::value &&
-    std::is_same<T, void>::value>::type
-  fulfilHelper(F&& func);
+  void detach();
 };
 
 }}
diff --git a/folly/wangle/detail/State.h b/folly/wangle/detail/State.h
index bf2a3434..39b6a7f2 100644
--- a/folly/wangle/detail/State.h
+++ b/folly/wangle/detail/State.h
@@ -17,6 +17,7 @@
 #pragma once
 
 #include <atomic>
+#include <mutex>
 #include <stdexcept>
 #include <vector>
 
@@ -32,7 +33,14 @@ namespace folly { namespace wangle { namespace detail {
 template<typename T>
 class State {
  public:
+  // This must be heap-constructed. There's probably a way to enforce that in
+  // code but since this is just internal detail code and I don't know how
+  // off-hand, I'm punting.
   State() = default;
+  ~State() {
+    assert(calledBack_);
+    assert(detached_ == 2);
+  }
 
   // not copyable
   State(State const&) = delete;
@@ -48,29 +56,32 @@ class State {
 
   template <typename F>
   void setCallback(F func) {
-    if (callback_) {
-      throw std::logic_error("setCallback called twice");
-    }
+    {
+      std::lock_guard<std::mutex> lock(mutex_);
 
-    callback_ = std::move(func);
+      if (callback_) {
+        throw std::logic_error("setCallback called twice");
+      }
 
-    if (shouldContinue_.test_and_set()) {
-      callback_(std::move(*value_));
-      delete this;
+      callback_ = std::move(func);
     }
+
+    maybeCallback();
   }
 
   void fulfil(Try<T>&& t) {
-    if (value_.hasValue()) {
-      throw std::logic_error("fulfil called twice");
-    }
+    {
+      std::lock_guard<std::mutex> lock(mutex_);
 
-    value_ = std::move(t);
+      if (ready()) {
+        throw std::logic_error("fulfil called twice");
+      }
 
-    if (shouldContinue_.test_and_set()) {
-      callback_(std::move(*value_));
-      delete this;
+      value_ = std::move(t);
+      assert(ready());
     }
+
+    maybeCallback();
   }
 
   void setException(std::exception_ptr const& e) {
@@ -93,10 +104,57 @@ class State {
     }
   }
 
+  // Called by a destructing Future
+  void detachFuture() {
+    if (!callback_) {
+      setCallback([](Try<T>&&) {});
+    }
+    detachOne();
+  }
+
+  // Called by a destructing Promise
+  void detachPromise() {
+    if (!ready()) {
+      setException(BrokenPromise());
+    }
+    detachOne();
+  }
+
  private:
-  std::atomic_flag shouldContinue_ = ATOMIC_FLAG_INIT;
+  void maybeCallback() {
+    std::lock_guard<std::mutex> lock(mutex_);
+    if (value_ && callback_) {
+      // TODO we should probably try/catch here
+      callback_(std::move(*value_));
+      calledBack_ = true;
+    }
+  }
+
+  void detachOne() {
+    bool shouldDelete;
+    {
+      std::lock_guard<std::mutex> lock(mutex_);
+      detached_++;
+      assert(detached_ == 1 || detached_ == 2);
+      shouldDelete = (detached_ == 2);
+    }
+
+    if (shouldDelete) {
+      // we should have already executed the callback with the value
+      assert(calledBack_);
+      delete this;
+    }
+  }
+
   folly::Optional<Try<T>> value_;
   std::function<void(Try<T>&&)> callback_;
+  bool calledBack_ = false;
+  unsigned char detached_ = 0;
+
+  // this lock isn't meant to protect all accesses to members, only the ones
+  // that need to be threadsafe: the act of setting value_ and callback_, and
+  // seeing if they are set and whether we should then continue.
+  std::mutex mutex_;
 };
 
 template <typename... Ts>
diff --git a/folly/wangle/test/FutureTest.cpp b/folly/wangle/test/FutureTest.cpp
index 97dd82a8..cbf76a15 100644
--- a/folly/wangle/test/FutureTest.cpp
+++ b/folly/wangle/test/FutureTest.cpp
@@ -276,20 +276,21 @@ TEST(Promise, fulfil) {
 
 TEST(Future, finish) {
   auto x = std::make_shared<int>(0);
-  Promise<int> p;
-  auto f = p.getFuture().then([x](Try<int>&& t) { *x = t.value(); });
-
-  // The callback hasn't executed
-  EXPECT_EQ(0, *x);
+  {
+    Promise<int> p;
+    auto f = p.getFuture().then([x](Try<int>&& t) { *x = t.value(); });
 
-  // The callback has a reference to x
-  EXPECT_EQ(2, x.use_count());
+    // The callback hasn't executed
+    EXPECT_EQ(0, *x);
 
-  p.setValue(42);
+    // The callback has a reference to x
+    EXPECT_EQ(2, x.use_count());
 
-  // the callback has executed
-  EXPECT_EQ(42, *x);
+    p.setValue(42);
 
+    // the callback has executed
+    EXPECT_EQ(42, *x);
+  }
   // the callback has been destructed
   // and has released its reference to x
   EXPECT_EQ(1, x.use_count());