Make cycle detection FATAL instead of throw and disable it in opt mode

Summary: Cycle detection can be very expensive, so it's better to disable it in opt mode. Because of that we need to make sure we catch such cycles in dbg builds, so we have to replace exceptions with LOG(FATAL).

Reviewed By: joshkirstein

Differential Revision: D29367695

fbshipit-source-id: 9c2038eb5b42f98f4ab997f963b6a131b8d26cf9

folly/experimental/observer/detail/ObserverManager.h

@@ -18,6 +18,7 @@
 
 #include <glog/logging.h>
 
+#include <folly/Portability.h>
 #include <folly/experimental/observer/detail/Core.h>
 #include <folly/experimental/observer/detail/GraphCycleDetector.h>
 #include <folly/fibers/FiberManager.h>
@@ -132,6 +133,9 @@ class ObserverManager {
     }
 
     static void markRefreshDependency(const Core& core) {
+      if (!kIsDebug) {
+        return;
+      }
       if (!currentDependencies_) {
         return;
       }
@@ -140,12 +144,15 @@ class ObserverManager {
         bool hasCycle =
             !cycleDetector.addEdge(&currentDependencies_->core, &core);
         if (hasCycle) {
-          throw std::logic_error("Observer cycle detected.");
+          LOG(FATAL) << "Observer cycle detected.";
         }
       });
     }
 
     static void unmarkRefreshDependency(const Core& core) {
+      if (!kIsDebug) {
+        return;
+      }
       if (!currentDependencies_) {
         return;
       }

folly/experimental/observer/test/ObserverTest.cpp

@@ -152,61 +152,33 @@ TEST(Observer, NullValue) {
 }
 
 TEST(Observer, Cycle) {
-  SimpleObservable<int> observable(0);
-  auto observer = observable.getObserver();
-  folly::Optional<Observer<int>> observerB;
-
-  auto observerA = makeObserver([observer, &observerB]() {
-    auto value = **observer;
-    if (value == 1) {
-      **observerB;
+  if (!folly::kIsDebug) {
+    // Cycle detection is only available in debug builds
+    return;
   }
-    return value;
-  });
-
-  observerB = makeObserver([observerA]() { return **observerA; });
 
-  auto collectObserver = makeObserver([observer, observerA, &observerB]() {
-    auto value = **observer;
-    auto valueA = **observerA;
-    auto valueB = ***observerB;
+  EXPECT_DEATH(
+      [] {
+        SimpleObservable<bool> observable(false);
+        folly::Optional<Observer<int>> observerB;
 
-    if (value == 1) {
-      if (valueA == 0) {
-        EXPECT_EQ(0, valueB);
-      } else {
-        EXPECT_EQ(1, valueA);
-        EXPECT_EQ(0, valueB);
+        auto observerA =
+            makeObserver([observer = observable.getObserver(), &observerB]() {
+              if (**observer) {
+                return ***observerB;
               }
-    } else if (value == 2) {
-      EXPECT_EQ(value, valueA);
-      EXPECT_TRUE(valueB == 0 || valueB == 2);
-    } else {
-      EXPECT_EQ(value, valueA);
-      EXPECT_EQ(value, valueB);
-    }
-
-    return value;
+              return 42;
             });
 
-  folly::Baton<> baton;
-  auto waitingObserver = makeObserver([collectObserver, &baton]() {
-    *collectObserver;
-    baton.post();
-    return folly::Unit();
-  });
-
-  baton.reset();
-  EXPECT_EQ(0, **collectObserver);
-
-  for (size_t i = 1; i <= 3; ++i) {
-    observable.setValue(i);
+        observerB = makeObserver([observerA]() { return **observerA; });
 
-    EXPECT_TRUE(baton.try_wait_for(std::chrono::seconds{1}));
-    baton.reset();
+        EXPECT_EQ(42, **observerA);
+        EXPECT_EQ(42, ***observerB);
 
-    EXPECT_EQ(i, **collectObserver);
-  }
+        observable.setValue(true);
+        folly::observer_detail::ObserverManager::waitForAllUpdates();
+      }(),
+      "Observer cycle detected.");
 }
 
 TEST(Observer, Stress) {