Commit 550b907b authored by Hans Fugal's avatar Hans Fugal Committed by Andrii Grynenko

Go back to a regular mutex

Summary:
D1428504 changed the `detail::State` mutex to be a recursive mutex to get around deadlock in our project, where a promise was being freed within a callback, racing with the mechanics happening while fulfilling the promise in the first place. At least, that's what seemed to be happening. I couldn't easily pull it into gdb because it's a python test.

I made my own test to repro, and it did in fact deadlock:

TEST(Future, DISABLED_promiseDestructedDuringCallback) {
auto p = folly::make_unique<Promise<void>>();
p->getFuture().then([&](Try<void>&&) {
// shouldn't deadlock.
p.reset();
});
p->setValue();
}

However, although this code fixes our project it does not fix this code, which still fails (not with a deadlock, but with a promiseAlreadyFulfilled exception). So whatever our project is doing is a bit more intricate.
I'm not convinced that even allowing this is ok - so I suspect out project is doing something bad. However, I also think it's probably bad to hold the lock while doing the callback so I am presenting this as a fix/compromise.

Test Plan:
unit tests

Reviewed By: hannesr@fb.com

Subscribers: net-systems@, fugalh, exa, njormrod, fsilva, davejwatson

FB internal diff: D1604829

Blame Revision: D1428504
parent 58132d07
......@@ -149,7 +149,7 @@ class State {
private:
void maybeCallback() {
std::lock_guard<decltype(mutex_)> lock(mutex_);
std::unique_lock<decltype(mutex_)> lock(mutex_);
if (!calledBack_ &&
value_ && callback_ && isActive()) {
// TODO(5306911) we should probably try/catch here
......@@ -159,8 +159,9 @@ class State {
executor_->add([cb, val]() mutable { (*cb)(std::move(**val)); });
calledBack_ = true;
} else {
callback_(std::move(*value_));
calledBack_ = true;
lock.unlock();
callback_(std::move(*value_));
}
}
}
......@@ -191,7 +192,7 @@ class State {
// 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::recursive_mutex mutex_;
std::mutex mutex_;
};
template <typename... Ts>
......
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