From 1d36d4649eb1f80a7741a9b71902dbf5e9b6dc61 Mon Sep 17 00:00:00 2001
From: Yedidya Feldblum <yfeldblum@fb.com>
Date: Thu, 29 Dec 2016 17:25:26 -0800
Subject: [PATCH] Let Future::then call callbacks outside of the catch handler

Summary:
[Folly] Let `Future::then` call callbacks outside of the catch handler.

And `Future::onError`.

This makes the behavior of calls to `Future` callbacks with respect to currently-handled ("active") exceptions consistent - there will not be an active exception by the time the `Future` callback is called. (Unless `Future::then` or `Future::onError`, etc., is itself called with an active exception. Or unless the `Promise` is fulfilled, outside of the `Future` implementation code, with an active exception.)

This will affect any code which tries to call `std::current_exception()` or `throw;` from within a `Future` callback, such as an `onError` handler. That code will crash. (It was incorrect anyway, and relied on misusing Folly Futures.)

Reviewed By: ericniebler

Differential Revision: D4372173

fbshipit-source-id: 600b22e4db63c98358de29a6abcee807fbc53b0f
---
 folly/futures/Future-inl.h | 80 +++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 31 deletions(-)

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index 52b650c0..2d19c67d 100644
--- a/folly/futures/Future-inl.h
+++ b/folly/futures/Future-inl.h
@@ -205,20 +205,26 @@ Future<T>::thenImplementation(F&& func, detail::argResult<isTry, F, Args...>) {
 
   setCallback_([ funcm = std::forward<F>(func), pm = std::move(p) ](
       Try<T> && t) mutable {
-    if (!isTry && t.hasException()) {
-      pm.setException(std::move(t.exception()));
-    } else {
-      try {
-        auto f2 = funcm(t.template get<isTry, Args>()...);
-        // that didn't throw, now we can steal p
-        f2.setCallback_([p = std::move(pm)](Try<B> && b) mutable {
-          p.setTry(std::move(b));
-        });
-      } catch (const std::exception& e) {
-        pm.setException(exception_wrapper(std::current_exception(), e));
-      } catch (...) {
-        pm.setException(exception_wrapper(std::current_exception()));
+    auto ew = [&] {
+      if (!isTry && t.hasException()) {
+        return std::move(t.exception());
+      } else {
+        try {
+          auto f2 = funcm(t.template get<isTry, Args>()...);
+          // that didn't throw, now we can steal p
+          f2.setCallback_([p = std::move(pm)](Try<B> && b) mutable {
+            p.setTry(std::move(b));
+          });
+          return exception_wrapper();
+        } catch (const std::exception& e) {
+          return exception_wrapper(std::current_exception(), e);
+        } catch (...) {
+          return exception_wrapper(std::current_exception());
+        }
       }
+    }();
+    if (ew) {
+      pm.setException(std::move(ew));
     }
   });
 
@@ -301,15 +307,21 @@ Future<T>::onError(F&& func) {
   setCallback_([ pm = std::move(p), funcm = std::forward<F>(func) ](
       Try<T> && t) mutable {
     if (!t.template withException<Exn>([&](Exn& e) {
-          try {
-            auto f2 = funcm(e);
-            f2.setCallback_([pm = std::move(pm)](Try<T> && t2) mutable {
-              pm.setTry(std::move(t2));
-            });
-          } catch (const std::exception& e2) {
-            pm.setException(exception_wrapper(std::current_exception(), e2));
-          } catch (...) {
-            pm.setException(exception_wrapper(std::current_exception()));
+          auto ew = [&] {
+            try {
+              auto f2 = funcm(e);
+              f2.setCallback_([pm = std::move(pm)](Try<T> && t2) mutable {
+                pm.setTry(std::move(t2));
+              });
+              return exception_wrapper();
+            } catch (const std::exception& e2) {
+              return exception_wrapper(std::current_exception(), e2);
+            } catch (...) {
+              return exception_wrapper(std::current_exception());
+            }
+          }();
+          if (ew) {
+            pm.setException(std::move(ew));
           }
         })) {
       pm.setTry(std::move(t));
@@ -350,15 +362,21 @@ Future<T>::onError(F&& func) {
   setCallback_(
       [ pm = std::move(p), funcm = std::forward<F>(func) ](Try<T> t) mutable {
         if (t.hasException()) {
-          try {
-            auto f2 = funcm(std::move(t.exception()));
-            f2.setCallback_([pm = std::move(pm)](Try<T> t2) mutable {
-              pm.setTry(std::move(t2));
-            });
-          } catch (const std::exception& e2) {
-            pm.setException(exception_wrapper(std::current_exception(), e2));
-          } catch (...) {
-            pm.setException(exception_wrapper(std::current_exception()));
+          auto ew = [&] {
+            try {
+              auto f2 = funcm(std::move(t.exception()));
+              f2.setCallback_([pm = std::move(pm)](Try<T> t2) mutable {
+                pm.setTry(std::move(t2));
+              });
+              return exception_wrapper();
+            } catch (const std::exception& e2) {
+              return exception_wrapper(std::current_exception(), e2);
+            } catch (...) {
+              return exception_wrapper(std::current_exception());
+            }
+          }();
+          if (ew) {
+            pm.setException(std::move(ew));
           }
         } else {
           pm.setTry(std::move(t));
-- 
2.34.1