Commit 03f41278 authored by Hans Fugal's avatar Hans Fugal Committed by Tudor Bosman

(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
parent 7609a2fb
...@@ -32,8 +32,8 @@ struct isFuture<Future<T> > { ...@@ -32,8 +32,8 @@ struct isFuture<Future<T> > {
}; };
template <class T> template <class T>
Future<T>::Future(Future<T>&& other) noexcept : state_(other.state_) { Future<T>::Future(Future<T>&& other) noexcept : state_(nullptr) {
other.state_ = nullptr; *this = std::move(other);
} }
template <class T> template <class T>
...@@ -44,8 +44,14 @@ Future<T>& Future<T>::operator=(Future<T>&& other) { ...@@ -44,8 +44,14 @@ Future<T>& Future<T>::operator=(Future<T>&& other) {
template <class T> template <class T>
Future<T>::~Future() { Future<T>::~Future() {
detach();
}
template <class T>
void Future<T>::detach() {
if (state_) { if (state_) {
setCallback_([](Try<T>&&) {}); // detach state_->detachFuture();
state_ = nullptr;
} }
} }
...@@ -60,7 +66,6 @@ template <class F> ...@@ -60,7 +66,6 @@ template <class F>
void Future<T>::setCallback_(F&& func) { void Future<T>::setCallback_(F&& func) {
throwIfInvalid(); throwIfInvalid();
state_->setCallback(std::move(func)); state_->setCallback(std::move(func));
state_ = nullptr;
} }
template <class T> template <class T>
......
...@@ -189,6 +189,8 @@ class Future { ...@@ -189,6 +189,8 @@ class Future {
explicit explicit
Future(statePtr obj) : state_(obj) {} Future(statePtr obj) : state_(obj) {}
void detach();
void throwIfInvalid() const; void throwIfInvalid() const;
friend class Promise<T>; friend class Promise<T>;
......
...@@ -29,9 +29,8 @@ Promise<T>::Promise() : retrieved_(false), state_(new detail::State<T>()) ...@@ -29,9 +29,8 @@ Promise<T>::Promise() : retrieved_(false), state_(new detail::State<T>())
{} {}
template <class T> template <class T>
Promise<T>::Promise(Promise<T>&& other) : Promise<T>::Promise(Promise<T>&& other) : state_(nullptr) {
retrieved_(other.retrieved_), state_(other.state_) { *this = std::move(other);
other.state_ = nullptr;
} }
template <class T> template <class T>
...@@ -44,6 +43,8 @@ Promise<T>& Promise<T>::operator=(Promise<T>&& other) { ...@@ -44,6 +43,8 @@ Promise<T>& Promise<T>::operator=(Promise<T>&& other) {
template <class T> template <class T>
void Promise<T>::throwIfFulfilled() { void Promise<T>::throwIfFulfilled() {
if (!state_) if (!state_)
throw NoState();
if (state_->ready())
throw PromiseAlreadySatisfied(); throw PromiseAlreadySatisfied();
} }
...@@ -55,8 +56,16 @@ void Promise<T>::throwIfRetrieved() { ...@@ -55,8 +56,16 @@ void Promise<T>::throwIfRetrieved() {
template <class T> template <class T>
Promise<T>::~Promise() { Promise<T>::~Promise() {
detach();
}
template <class T>
void Promise<T>::detach() {
if (state_) { if (state_) {
setException(BrokenPromise()); if (!retrieved_)
state_->detachFuture();
state_->detachPromise();
state_ = nullptr;
} }
} }
...@@ -71,7 +80,6 @@ Future<T> Promise<T>::getFuture() { ...@@ -71,7 +80,6 @@ Future<T> Promise<T>::getFuture() {
template <class T> template <class T>
template <class E> template <class E>
void Promise<T>::setException(E const& e) { void Promise<T>::setException(E const& e) {
throwIfFulfilled();
setException(std::make_exception_ptr<E>(e)); setException(std::make_exception_ptr<E>(e));
} }
...@@ -79,20 +87,12 @@ template <class T> ...@@ -79,20 +87,12 @@ template <class T>
void Promise<T>::setException(std::exception_ptr const& e) { void Promise<T>::setException(std::exception_ptr const& e) {
throwIfFulfilled(); throwIfFulfilled();
state_->setException(e); state_->setException(e);
if (!retrieved_) {
delete state_;
}
state_ = nullptr;
} }
template <class T> template <class T>
void Promise<T>::fulfilTry(Try<T>&& t) { void Promise<T>::fulfilTry(Try<T>&& t) {
throwIfFulfilled(); throwIfFulfilled();
state_->fulfil(std::move(t)); state_->fulfil(std::move(t));
if (!retrieved_) {
delete state_;
}
state_ = nullptr;
} }
template <class T> template <class T>
...@@ -101,12 +101,7 @@ void Promise<T>::setValue(M&& v) { ...@@ -101,12 +101,7 @@ void Promise<T>::setValue(M&& v) {
static_assert(!std::is_same<T, void>::value, static_assert(!std::is_same<T, void>::value,
"Use setValue() instead"); "Use setValue() instead");
throwIfFulfilled(); fulfilTry(Try<T>(std::forward<M>(v)));
state_->fulfil(Try<T>(std::forward<M>(v)));
if (!retrieved_) {
delete state_;
}
state_ = nullptr;
} }
template <class T> template <class T>
...@@ -114,47 +109,14 @@ void Promise<T>::setValue() { ...@@ -114,47 +109,14 @@ void Promise<T>::setValue() {
static_assert(std::is_same<T, void>::value, static_assert(std::is_same<T, void>::value,
"Use setValue(value) instead"); "Use setValue(value) instead");
throwIfFulfilled(); fulfilTry(Try<void>());
state_->fulfil(Try<void>());
if (!retrieved_) {
delete state_;
}
state_ = nullptr;
} }
template <class T> template <class T>
template <class F> template <class F>
void Promise<T>::fulfil(F&& func) { 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(); throwIfFulfilled();
try { fulfilTry(makeTryFunction(std::forward<F>(func)));
func();
setValue();
} catch (...) {
setException(std::current_exception());
}
} }
}} }}
...@@ -86,18 +86,7 @@ private: ...@@ -86,18 +86,7 @@ private:
void throwIfFulfilled(); void throwIfFulfilled();
void throwIfRetrieved(); void throwIfRetrieved();
void detach();
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);
}; };
}} }}
......
...@@ -17,6 +17,7 @@ ...@@ -17,6 +17,7 @@
#pragma once #pragma once
#include <atomic> #include <atomic>
#include <mutex>
#include <stdexcept> #include <stdexcept>
#include <vector> #include <vector>
...@@ -32,7 +33,14 @@ namespace folly { namespace wangle { namespace detail { ...@@ -32,7 +33,14 @@ namespace folly { namespace wangle { namespace detail {
template<typename T> template<typename T>
class State { class State {
public: 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() = default;
~State() {
assert(calledBack_);
assert(detached_ == 2);
}
// not copyable // not copyable
State(State const&) = delete; State(State const&) = delete;
...@@ -48,29 +56,32 @@ class State { ...@@ -48,29 +56,32 @@ class State {
template <typename F> template <typename F>
void setCallback(F func) { 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(func);
callback_(std::move(*value_));
delete this;
} }
maybeCallback();
} }
void fulfil(Try<T>&& t) { 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()) { value_ = std::move(t);
callback_(std::move(*value_)); assert(ready());
delete this;
} }
maybeCallback();
} }
void setException(std::exception_ptr const& e) { void setException(std::exception_ptr const& e) {
...@@ -93,10 +104,57 @@ class State { ...@@ -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: 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_; folly::Optional<Try<T>> value_;
std::function<void(Try<T>&&)> callback_; 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> template <typename... Ts>
......
...@@ -276,20 +276,21 @@ TEST(Promise, fulfil) { ...@@ -276,20 +276,21 @@ TEST(Promise, fulfil) {
TEST(Future, finish) { TEST(Future, finish) {
auto x = std::make_shared<int>(0); auto x = std::make_shared<int>(0);
Promise<int> p; {
auto f = p.getFuture().then([x](Try<int>&& t) { *x = t.value(); }); Promise<int> p;
auto f = p.getFuture().then([x](Try<int>&& t) { *x = t.value(); });
// The callback hasn't executed
EXPECT_EQ(0, *x);
// The callback has a reference to x // The callback hasn't executed
EXPECT_EQ(2, x.use_count()); EXPECT_EQ(0, *x);
p.setValue(42); // The callback has a reference to x
EXPECT_EQ(2, x.use_count());
// the callback has executed p.setValue(42);
EXPECT_EQ(42, *x);
// the callback has executed
EXPECT_EQ(42, *x);
}
// the callback has been destructed // the callback has been destructed
// and has released its reference to x // and has released its reference to x
EXPECT_EQ(1, x.use_count()); EXPECT_EQ(1, x.use_count());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment