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 037ad138540f6f586ba51a16f28c9e2c10a45a4c..f9d18679d164bd9369892885a041598a2a980ac2 100644 (file)
--- 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 832e135ce08dccfc6389b2432334862b34c73e31..e477f5cc70b1157adf0aaaf928cf74553ed75569 100644 (file)
--- 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);
 }