From: Hans Fugal <fugalh@fb.com>
Date: Tue, 21 Apr 2015 18:19:04 +0000 (-0700)
Subject: Assume exception when Executor::add throws
X-Git-Tag: v0.36.0~20
X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=df50885c695dc72d0897291c2deecd6c91f0395b;p=folly.git

Assume exception when Executor::add throws

Summary:
Rather than crashing spectacularly, if `Executor::add` throws (e.g. because the queue is full), then discard the result we got and assume the exception the executor threw instead.

Alternatively, we could pass this exceptional Try to the callback (without an executor, as it is here), but not perturb `result_`. This would mean two different world views in these two code snippets:

auto f1 = makeFuture(42).via(&crappyExecutor);
f1.value(); // 42 (no callback happened)
f1.then(...); // would see the executor's exception. Would also be ill-advised to do this after value()

auto f2 = makeFuture(42).via(&crappyExecutor)
.then([](int x) { return x * 2; }); // skipped
f2.value(); // throws executor's exception

It feels rude to throw away the result, but it feels too potentially dangerous to allow this split view of the world.

Test Plan: modified unit

Reviewed By: jsedgwick@fb.com

Subscribers: exa, folly-diffs@, jsedgwick, yfeldblum, chalfant

FB internal diff: D2007729

Tasks: 5306911

Signature: t1:2007729:1429627114:b627ce758ce9231298f1b28e203ccc1ee415ed9a
---

diff --git a/folly/futures/detail/Core.h b/folly/futures/detail/Core.h
index 037ad138..f9d18679 100644
--- a/folly/futures/detail/Core.h
+++ b/folly/futures/detail/Core.h
@@ -278,18 +278,21 @@ class Core {
   }
 
   void doCallback() {
-    // TODO(5306911) we should probably try/catch around the callback
-
     RequestContext::setContext(context_);
 
     // TODO(6115514) semantic race on reading executor_ and setExecutor()
     Executor* x = executor_;
     if (x) {
       ++attached_; // keep Core alive until executor did its thing
-      x->add([this]() mutable {
-        SCOPE_EXIT { detachOne(); };
+      try {
+        x->add([this]() mutable {
+          SCOPE_EXIT { detachOne(); };
+          callback_(std::move(*result_));
+        });
+      } catch (...) {
+        result_ = Try<T>(exception_wrapper(std::current_exception()));
         callback_(std::move(*result_));
-      });
+      }
     } else {
       callback_(std::move(*result_));
     }
diff --git a/folly/futures/test/ExecutorTest.cpp b/folly/futures/test/ExecutorTest.cpp
index 832e135c..e477f5cc 100644
--- a/folly/futures/test/ExecutorTest.cpp
+++ b/folly/futures/test/ExecutorTest.cpp
@@ -178,14 +178,10 @@ class CrappyExecutor : public Executor {
 
 TEST(Executor, CrappyExecutor) {
   CrappyExecutor x;
-  try {
-    auto f = Future<void>().via(&x).then([](){
-      return;
-    });
-    f.value();
-    EXPECT_TRUE(false);
-  } catch(...) {
-    // via() should throw
-    return;
-  }
+  bool flag = false;
+  auto f = folly::via(&x).onError([&](std::runtime_error& e) {
+    EXPECT_STREQ("bad", e.what());
+    flag = true;
+  });
+  EXPECT_TRUE(flag);
 }