Only execute callback if snapshot data has changed

Summary: If a callback associated with an observer depends on yet another observer, the callback may be called more than once per distinct snapshot of the associated observer. This diff fixes this problem.

Reviewed By: andriigrynenko

Differential Revision: D29313796

fbshipit-source-id: 345bfa0a1ac9dfaee806be21256088d4667aef15

folly/experimental/observer/Observer-inl.h

@@ -246,7 +246,9 @@ CallbackHandle::CallbackHandle(
     if (*rCanceled) {
       return folly::unit;
     }
-    callback(*observer);
+    auto snapshot = *observer;
+    observer_detail::ObserverManager::DependencyRecorder::
+        withDependencyRecordingDisabled([&] { callback(std::move(snapshot)); });
     return folly::unit;
   });
 }

folly/experimental/observer/detail/ObserverManager.h

@@ -125,6 +125,13 @@ class ObserverManager {
 
     static bool isActive() { return currentDependencies_; }
 
+    static void withDependencyRecordingDisabled(folly::FunctionRef<void()> f) {
+      auto* const dependencies = std::exchange(currentDependencies_, nullptr);
+      SCOPE_EXIT { currentDependencies_ = dependencies; };
+
+      f();
+    }
+
     static void markDependency(Core::Ptr dependency) {
       DCHECK(inManagerThread());
       DCHECK(currentDependencies_);

folly/experimental/observer/test/ObserverTest.cpp

@@ -376,6 +376,55 @@ TEST(Observer, SetCallback) {
   EXPECT_EQ(2, callbackCallsCount);
 }
 
+TEST(Observer, CallbackCalledOncePerSnapshot) {
+  SimpleObservable<folly::Unit> observable(folly::unit);
+  auto observer = observable.getObserver();
+
+  int value = 1;
+  SimpleObservable<int> intObservable(value);
+  auto squareObserver =
+      makeObserver([o = intObservable.getObserver()] { return **o * **o; });
+
+  folly::Baton baton;
+  size_t callbackCallsCount = 0;
+  auto callbackHandle = observer.addCallback([&](auto) {
+    // The main point of this test is that the callback depends on
+    // `squareObserver`. A refresh of `squareObserver` should not trigger the
+    // callback, since the callback is associated only with `observer`.
+    //
+    // Note that we do not guarantee that **squareObserver necessarily reflects
+    // the latest update.
+    EXPECT_GE(value * value, **squareObserver);
+
+    ++callbackCallsCount;
+    baton.post();
+  });
+
+  baton.wait();
+  baton.reset();
+  EXPECT_EQ(1, callbackCallsCount);
+
+  // Check that any second updates to squareObserver don't trigger the callback
+  // again
+  folly::observer_detail::ObserverManager::waitForAllUpdates();
+  EXPECT_EQ(1, callbackCallsCount);
+
+  value = 2;
+  intObservable.setValue(value);
+  observable.setValue(folly::Unit{});
+
+  baton.wait();
+  baton.reset();
+  EXPECT_EQ(2, callbackCallsCount);
+
+  value = 3;
+  // Updating intObservable should not trigger the callback
+  intObservable.setValue(value);
+  folly::observer_detail::ObserverManager::waitForAllUpdates();
+  EXPECT_EQ(9, **squareObserver);
+  EXPECT_EQ(2, callbackCallsCount);
+}
+
 TEST(Observer, CallbackMemoryLeak) {
   folly::observer::SimpleObservable<int> observable(42);
   auto observer = observable.getObserver();