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
};
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>
template <class T>
Future<T>::~Future() {
+ detach();
+}
+
+template <class T>
+void Future<T>::detach() {
if (state_) {
- setCallback_([](Try<T>&&) {}); // detach
+ state_->detachFuture();
+ state_ = nullptr;
}
}
void Future<T>::setCallback_(F&& func) {
throwIfInvalid();
state_->setCallback(std::move(func));
- state_ = nullptr;
}
template <class T>
explicit
Future(statePtr obj) : state_(obj) {}
+ void detach();
+
void throwIfInvalid() const;
friend class Promise<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>
template <class T>
void Promise<T>::throwIfFulfilled() {
if (!state_)
+ throw NoState();
+ if (state_->ready())
throw PromiseAlreadySatisfied();
}
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;
}
}
template <class T>
template <class E>
void Promise<T>::setException(E const& e) {
- throwIfFulfilled();
setException(std::make_exception_ptr<E>(e));
}
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>
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>
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)));
}
}}
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();
};
}}
#pragma once
#include <atomic>
+#include <mutex>
#include <stdexcept>
#include <vector>
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;
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) {
}
}
+ // 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>
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());
projects / folly.git / commitdiff