From 4762a35e70fb08622baed5d30157c99c215f4f15 Mon Sep 17 00:00:00 2001
From: Yinghai Lu <yinghai@fb.com>
Date: Fri, 12 May 2017 10:21:06 -0700
Subject: [PATCH] Fix broken promise error in folly::retrying

Summary: This diff shows an issue in `folly::retrying`. When the future generation function throws an exception and `folly::retrying` is nested in another functor that returns Future, it will throw `broken promise` instead of the actual exception message, which can be very generic and confusing. Fix is to capture the exception so that exact error message can be propagated up.

Reviewed By: yfeldblum

Differential Revision: D5050690

fbshipit-source-id: 5b9b24977788f60aa778bb8e9cdf4281ea9a0023
---
 folly/futures/Future-inl.h          |  2 +-
 folly/futures/test/RetryingTest.cpp | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/folly/futures/Future-inl.h b/folly/futures/Future-inl.h
index 0e6c7e31..c4f9ca2f 100644
--- a/folly/futures/Future-inl.h
+++ b/folly/futures/Future-inl.h
@@ -1233,7 +1233,7 @@ template <class Policy, class FF, class Prom>
 void retryingImpl(size_t k, Policy&& p, FF&& ff, Prom prom) {
   using F = typename std::result_of<FF(size_t)>::type;
   using T = typename F::value_type;
-  auto f = ff(k++);
+  auto f = makeFutureWith([&] { return ff(k++); });
   f.then([
     k,
     prom = std::move(prom),
diff --git a/folly/futures/test/RetryingTest.cpp b/folly/futures/test/RetryingTest.cpp
index 2ec569fa..59eb3e01 100644
--- a/folly/futures/test/RetryingTest.cpp
+++ b/folly/futures/test/RetryingTest.cpp
@@ -75,6 +75,27 @@ TEST(RetryingTest, basic) {
   EXPECT_EQ(2, r.value());
 }
 
+TEST(RetryingTest, future_factory_throws) {
+  struct ReturnedException : exception {};
+  struct ThrownException : exception {};
+  auto result = futures::retrying(
+                    [](size_t n, const exception_wrapper&) { return n < 2; },
+                    [](size_t n) {
+                      switch (n) {
+                        case 0:
+                          return makeFuture<size_t>(
+                              make_exception_wrapper<ReturnedException>());
+                        case 1:
+                          throw ThrownException();
+                        default:
+                          return makeFuture(n);
+                      }
+                    })
+                    .wait()
+                    .getTry();
+  EXPECT_THROW(result.throwIfFailed(), ThrownException);
+}
+
 TEST(RetryingTest, policy_future) {
   atomic<size_t> sleeps {0};
   auto r = futures::retrying(
-- 
2.34.1