Commit d85ae919 authored by Andrii Grynenko's avatar Andrii Grynenko Committed by Facebook Github Bot

Fix a race in thenImplementation

Summary: It's unsafe to read Executor from Core, because it can be concurrently modified by a consumer thread.

Reviewed By: yfeldblum

Differential Revision: D15117907

fbshipit-source-id: 40caf4b2878609775c1c8476336349ddf3764d4d
parent 79d908dd
...@@ -417,15 +417,15 @@ FutureBase<T>::thenImplementation(F&& func, R) { ...@@ -417,15 +417,15 @@ FutureBase<T>::thenImplementation(F&& func, R) {
// Pass through a simple future as it needs no deferral adaptation // Pass through a simple future as it needs no deferral adaptation
template <class T> template <class T>
Future<T> chainExecutor(Executor*, Future<T>&& f) { Future<T> chainExecutor(Executor::KeepAlive<>, Future<T>&& f) {
return std::move(f); return std::move(f);
} }
// Correctly chain a SemiFuture for deferral // Correctly chain a SemiFuture for deferral
template <class T> template <class T>
Future<T> chainExecutor(Executor* e, SemiFuture<T>&& f) { Future<T> chainExecutor(Executor::KeepAlive<> e, SemiFuture<T>&& f) {
if (!e) { if (!e) {
e = &InlineExecutor::instance(); e = folly::getKeepAliveToken(InlineExecutor::instance());
} }
return std::move(f).via(e); return std::move(f).via(e);
} }
...@@ -445,31 +445,30 @@ FutureBase<T>::thenImplementation(F&& func, R) { ...@@ -445,31 +445,30 @@ FutureBase<T>::thenImplementation(F&& func, R) {
// grab the Future now before we lose our handle on the Promise // grab the Future now before we lose our handle on the Promise
auto sf = p.getSemiFuture(); auto sf = p.getSemiFuture();
auto* e = this->getExecutor(); auto e = getKeepAliveToken(this->getExecutor());
sf.setExecutor(e); sf.setExecutor(e);
auto f = Future<B>(sf.core_); auto f = Future<B>(sf.core_);
sf.core_ = nullptr; sf.core_ = nullptr;
this->setCallback_( this->setCallback_([state = futures::detail::makeCoreCallbackState(
[state = futures::detail::makeCoreCallbackState( std::move(p), std::forward<F>(func)),
std::move(p), std::forward<F>(func))](Try<T>&& t) mutable { e = std::move(e)](Try<T>&& t) mutable {
if (!R::Arg::isTry() && t.hasException()) { if (!R::Arg::isTry() && t.hasException()) {
state.setException(std::move(t.exception())); state.setException(std::move(t.exception()));
} else { } else {
// Ensure that if function returned a SemiFuture we correctly chain // Ensure that if function returned a SemiFuture we correctly chain
// potential deferral. // potential deferral.
auto tf2 = detail_msvc_15_7_workaround::tryInvoke(R{}, state, t); auto tf2 = detail_msvc_15_7_workaround::tryInvoke(R{}, state, t);
if (tf2.hasException()) { if (tf2.hasException()) {
state.setException(std::move(tf2.exception())); state.setException(std::move(tf2.exception()));
} else { } else {
auto statePromise = state.stealPromise(); auto statePromise = state.stealPromise();
auto tf3 = chainExecutor( auto tf3 = chainExecutor(std::move(e), *std::move(tf2));
statePromise.core_->getExecutor(), *std::move(tf2)); std::exchange(statePromise.core_, nullptr)
std::exchange(statePromise.core_, nullptr) ->setProxy(std::exchange(tf3.core_, nullptr));
->setProxy(std::exchange(tf3.core_, nullptr)); }
} }
} });
});
return f; return f;
} }
......
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