Fix possible lock inversion bug in ObserverCreatorContext

Reviewed By: Gownta

Differential Revision: D32197071

fbshipit-source-id: 19ead9d39777efa73750346e59dc69ef57ce4961

folly/experimental/observer/Observable-inl.h

@@ -55,8 +55,16 @@ class ObserverCreatorContext {
     // Additionally it helps avoid races between two different subscription
     // callbacks (getting new value from observable and storing it into value_
     // is not atomic).
+    //
+    // Note that state_ lock is acquired only after Traits::get. Traits::get
+    // is running application code (that may acquire locks) and so it's
+    // important to not hold state_ lock while running it to avoid possible lock
+    // inversion with another code path that needs state_ lock (e.g. get()).
+    std::lock_guard<std::mutex> updateLockGuard(updateLock_);
+    auto newValue = Traits::get(observable_);
+
     auto state = state_.lock();
-    if (!state->updateValue(Traits::get(observable_))) {
+    if (!state->updateValue(std::move(newValue))) {
       // Value didn't change, so we can skip the version update.
       return;
     }
@@ -72,6 +80,7 @@ class ObserverCreatorContext {
   }
 
  private:
+  std::mutex updateLock_;
   struct State {
     bool updateValue(std::shared_ptr<const T> newValue) {
       auto newValuePtr = newValue.get();

folly/experimental/observer/test/ObserverTest.cpp

@@ -914,3 +914,47 @@ TEST(Observer, Fibers) {
   std::move(f1).getVia(&evb);
   std::move(f2).getVia(&evb);
 }
+
+std::mutex lockingObservableLock;
+std::atomic<size_t> lockingObservableValue{0};
+folly::Function<void()> lockingObservableCallback;
+
+TEST(Observer, ObservableLockInversion) {
+  struct LockingObservable {
+    using element_type = size_t;
+
+    std::shared_ptr<const size_t> get() {
+      std::lock_guard<std::mutex> lg(lockingObservableLock);
+      return std::make_shared<const size_t>(lockingObservableValue.load());
+    }
+
+    void subscribe(folly::Function<void()> cb) {
+      lockingObservableCallback = std::move(cb);
+    }
+
+    void unsubscribe() { lockingObservableCallback = nullptr; }
+  };
+
+  auto observer =
+      folly::observer::ObserverCreator<LockingObservable>().getObserver();
+
+  EXPECT_EQ(0, **observer);
+
+  constexpr size_t kNumIters = 1000;
+
+  std::thread updater([&] {
+    for (size_t i = 1; i <= kNumIters; ++i) {
+      lockingObservableValue = i;
+      lockingObservableCallback();
+    }
+  });
+
+  while (true) {
+    std::lock_guard<std::mutex> lg(lockingObservableLock);
+    if (**makeObserver([&] { return **observer; }) == kNumIters) {
+      break;
+    }
+  }
+
+  updater.join();
+}