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

diff --git a/folly/wangle/detail/State.h b/folly/wangle/detail/State.h
index c0d921f62f86c8ee494a4e8aa14ca4a8060023d6..c06e223667d6db983dc814ed43f86f00dba18bcc 100644 (file)
--- a/folly/wangle/detail/State.h
+++ b/folly/wangle/detail/State.h
@@ -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>