Make collect* return SemiFuture

Summary:
Migration from Future-returning executor-erasing collectX forms to SemiFuture-returning forms, that are less risky in particular with coroutines.

Earlier changes added collectXSemiFuture and collectXUnsafe as a migration path. This diff changes the behaviour of collectX to return SemiFuture.

Reviewed By: yfeldblum

Differential Revision: D20260537

fbshipit-source-id: 9e01536e4d9af68ff429f9fcd10aea4881d62041

folly/futures/Future-inl.h

@@ -1374,7 +1374,7 @@ void stealDeferredExecutors(
 
 template <typename... Fs>
 SemiFuture<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>>
-collectAllSemiFuture(Fs&&... fs) {
+collectAll(Fs&&... fs) {
   using Result = std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>;
   struct Context {
     ~Context() {
@@ -1409,9 +1409,9 @@ collectAllSemiFuture(Fs&&... fs) {
 }
 
 template <typename... Fs>
-Future<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>> collectAll(
-    Fs&&... fs) {
-  return collectAllSemiFuture(std::forward<Fs>(fs)...).toUnsafeFuture();
+SemiFuture<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>>
+collectAllSemiFuture(Fs&&... fs) {
+  return collectAll(std::forward<Fs>(fs)...);
 }
 
 template <typename... Fs>
@@ -1425,7 +1425,7 @@ collectAllUnsafe(Fs&&... fs) {
 template <class InputIterator>
 SemiFuture<std::vector<
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
-collectAllSemiFuture(InputIterator first, InputIterator last) {
+collectAll(InputIterator first, InputIterator last) {
   using F = typename std::iterator_traits<InputIterator>::value_type;
   using T = typename F::value_type;
 
@@ -1473,23 +1473,22 @@ template <class InputIterator>
 Future<std::vector<
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
 collectAllUnsafe(InputIterator first, InputIterator last) {
-  return collectAllSemiFuture(first, last).toUnsafeFuture();
+  return collectAll(first, last).toUnsafeFuture();
 }
 
 template <class InputIterator>
-Future<std::vector<
+SemiFuture<std::vector<
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
-collectAll(InputIterator first, InputIterator last) {
-  return collectAllSemiFuture(first, last).toUnsafeFuture();
+collectAllSemiFuture(InputIterator first, InputIterator last) {
+  return collectAll(first, last);
 }
 
 // collect (iterator)
 
-// TODO(T26439406): Make return SemiFuture
 template <class InputIterator>
 SemiFuture<std::vector<
     typename std::iterator_traits<InputIterator>::value_type::value_type>>
-collectSemiFuture(InputIterator first, InputIterator last) {
+collect(InputIterator first, InputIterator last) {
   using F = typename std::iterator_traits<InputIterator>::value_type;
   using T = typename F::value_type;
 
@@ -1546,22 +1545,21 @@ template <class InputIterator>
 Future<std::vector<
     typename std::iterator_traits<InputIterator>::value_type::value_type>>
 collectUnsafe(InputIterator first, InputIterator last) {
-  return collectSemiFuture(first, last).toUnsafeFuture();
+  return collect(first, last).toUnsafeFuture();
 }
 
 template <class InputIterator>
-Future<std::vector<
+SemiFuture<std::vector<
     typename std::iterator_traits<InputIterator>::value_type::value_type>>
-collect(InputIterator first, InputIterator last) {
-  return collectSemiFuture(first, last).toUnsafeFuture();
+collectSemiFuture(InputIterator first, InputIterator last) {
+  return collect(first, last);
 }
 
 // collect (variadic)
 
-// TODO(T26439406): Make return SemiFuture
 template <typename... Fs>
-SemiFuture<std::tuple<typename remove_cvref_t<Fs>::value_type...>>
-collectSemiFuture(Fs&&... fs) {
+SemiFuture<std::tuple<typename remove_cvref_t<Fs>::value_type...>> collect(
+    Fs&&... fs) {
   using Result = std::tuple<typename remove_cvref_t<Fs>::value_type...>;
   struct Context {
     ~Context() {
@@ -1605,15 +1603,15 @@ collectSemiFuture(Fs&&... fs) {
 }
 
 template <typename... Fs>
-Future<std::tuple<typename remove_cvref_t<Fs>::value_type...>> collect(
-    Fs&&... fs) {
-  return collectSemiFuture(std::forward<Fs>(fs)...).toUnsafeFuture();
+SemiFuture<std::tuple<typename remove_cvref_t<Fs>::value_type...>>
+collectSemiFuture(Fs&&... fs) {
+  return collect(std::forward<Fs>(fs)...);
 }
 
 template <typename... Fs>
 Future<std::tuple<typename remove_cvref_t<Fs>::value_type...>> collectUnsafe(
     Fs&&... fs) {
-  return collectSemiFuture(std::forward<Fs>(fs)...).toUnsafeFuture();
+  return collect(std::forward<Fs>(fs)...).toUnsafeFuture();
 }
 
 template <class Collection>
@@ -1626,11 +1624,11 @@ auto collectSemiFuture(Collection&& c)
 
 // TODO(T26439406): Make return SemiFuture
 template <class InputIterator>
-Future<std::pair<
+SemiFuture<std::pair<
     size_t,
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
-collectAny(InputIterator first, InputIterator last) {
-  return collectAnySemiFuture(first, last).via(&InlineExecutor::instance());
+collectAnySemiFuture(InputIterator first, InputIterator last) {
+  return collectAny(first, last);
 }
 
 template <class InputIterator>
@@ -1638,14 +1636,14 @@ Future<std::pair<
     size_t,
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
 collectAnyUnsafe(InputIterator first, InputIterator last) {
-  return collectAnySemiFuture(first, last).via(&InlineExecutor::instance());
+  return collectAny(first, last).toUnsafeFuture();
 }
 
 template <class InputIterator>
 SemiFuture<std::pair<
     size_t,
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
-collectAnySemiFuture(InputIterator first, InputIterator last) {
+collectAny(InputIterator first, InputIterator last) {
   using F = typename std::iterator_traits<InputIterator>::value_type;
   using T = typename F::value_type;
 

folly/futures/Future.h

@@ -2352,7 +2352,12 @@ auto via(Executor::KeepAlive<>, Func&& func) -> Future<
   follow with `via(executor)` because it will complete in whichever thread the
   last Future completes in.
 
-  The return type for Future<T> input is a Future<std::vector<Try<T>>>
+  The return type for Future<T> input is a SemiFuture<std::vector<Try<T>>>
+  for collectX and collectXSemiFuture.
+
+  collectXUnsafe returns an inline Future that erases the executor from the
+  incoming Futures/SemiFutures. collectXUnsafe should be phased out and
+  replaced with collectX(...).via(e) where e is a valid non-inline executor.
   */
 template <class InputIterator>
 SemiFuture<std::vector<
@@ -2366,11 +2371,13 @@ auto collectAllSemiFuture(Collection&& c)
   return collectAllSemiFuture(c.begin(), c.end());
 }
 
+// Unsafe variant, see above comment for details
 template <class InputIterator>
 Future<std::vector<
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
 collectAllUnsafe(InputIterator first, InputIterator last);
 
+// Unsafe variant sugar, see above comment for details
 template <class Collection>
 auto collectAllUnsafe(Collection&& c)
     -> decltype(collectAllUnsafe(c.begin(), c.end())) {
@@ -2378,7 +2385,7 @@ auto collectAllUnsafe(Collection&& c)
 }
 
 template <class InputIterator>
-Future<std::vector<
+SemiFuture<std::vector<
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
 collectAll(InputIterator first, InputIterator last);
 
@@ -2389,51 +2396,56 @@ auto collectAll(Collection&& c) -> decltype(collectAll(c.begin(), c.end())) {
 
 /// This version takes a varying number of Futures instead of an iterator.
 /// The return type for (Future<T1>, Future<T2>, ...) input
-/// is a Future<std::tuple<Try<T1>, Try<T2>, ...>>.
+/// is a SemiFuture<std::tuple<Try<T1>, Try<T2>, ...>>.
 /// The Futures are moved in, so your copies are invalid.
 template <typename... Fs>
 SemiFuture<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>>
 collectAllSemiFuture(Fs&&... fs);
 
+// Unsafe variant of collectAll, see coment above for details. Returns
+// a Future<std::tuple<Try<T1>, Try<T2>, ...>> on the Inline executor.
 template <typename... Fs>
 Future<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>>
 collectAllUnsafe(Fs&&... fs);
 
 template <typename... Fs>
-Future<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>> collectAll(
-    Fs&&... fs);
+SemiFuture<std::tuple<Try<typename remove_cvref_t<Fs>::value_type>...>>
+collectAll(Fs&&... fs);
 
 /// Like collectAll, but will short circuit on the first exception. Thus, the
-/// type of the returned Future is std::vector<T> instead of
+/// type of the returned SemiFuture is std::vector<T> instead of
 /// std::vector<Try<T>>
 template <class InputIterator>
-Future<std::vector<
+SemiFuture<std::vector<
     typename std::iterator_traits<InputIterator>::value_type::value_type>>
-collectUnsafe(InputIterator first, InputIterator last);
+collect(InputIterator first, InputIterator last);
 
 /// Sugar for the most common case
 template <class Collection>
-auto collectUnsafe(Collection&& c)
-    -> decltype(collectUnsafe(c.begin(), c.end())) {
-  return collectUnsafe(c.begin(), c.end());
+auto collect(Collection&& c) -> decltype(collect(c.begin(), c.end())) {
+  return collect(c.begin(), c.end());
 }
 
+// Unsafe variant of collect. Returns a Future<std::vector<T>> that
+// completes inline.
 template <class InputIterator>
 Future<std::vector<
     typename std::iterator_traits<InputIterator>::value_type::value_type>>
-collect(InputIterator first, InputIterator last);
+collectUnsafe(InputIterator first, InputIterator last);
 
-/// Sugar for the most common case
+/// Sugar for the most common unsafe case. Returns a Future<std::vector<T>>
+// that completes inline.
 template <class Collection>
-auto collect(Collection&& c) -> decltype(collect(c.begin(), c.end())) {
-  return collect(c.begin(), c.end());
+auto collectUnsafe(Collection&& c)
+    -> decltype(collectUnsafe(c.begin(), c.end())) {
+  return collectUnsafe(c.begin(), c.end());
 }
 
 /// Like collectAll, but will short circuit on the first exception. Thus, the
-/// type of the returned Future is std::tuple<T1, T2, ...> instead of
+/// type of the returned SemiFuture is std::tuple<T1, T2, ...> instead of
 /// std::tuple<Try<T1>, Try<T2>, ...>
 template <typename... Fs>
-Future<std::tuple<typename remove_cvref_t<Fs>::value_type...>> collect(
+SemiFuture<std::tuple<typename remove_cvref_t<Fs>::value_type...>> collect(
     Fs&&... fs);
 
 /** The result is a pair of the index of the first Future to complete and
@@ -2443,10 +2455,11 @@ Future<std::tuple<typename remove_cvref_t<Fs>::value_type...>> collect(
   This function is thread-safe for Futures running on different threads.
   */
 template <class InputIterator>
-Future<std::pair<
+SemiFuture<std::pair<
     size_t,
     Try<typename std::iterator_traits<InputIterator>::value_type::value_type>>>
 collectAny(InputIterator first, InputIterator last);
+// Unsafe variant of collectAny, Returns a Future that completes inline.
 template <class InputIterator>
 Future<std::pair<
     size_t,
@@ -2463,6 +2476,8 @@ template <class Collection>
 auto collectAny(Collection&& c) -> decltype(collectAny(c.begin(), c.end())) {
   return collectAny(c.begin(), c.end());
 }
+// Unsafe variant of common form of collectAny, Returns a Future that completes
+// inline.
 template <class Collection>
 auto collectAnyUnsafe(Collection&& c)
     -> decltype(collectAnyUnsafe(c.begin(), c.end())) {

folly/futures/test/CollectTest.cpp

@@ -79,7 +79,7 @@ TEST(Collect, collectAll) {
     promises[3].setException(eggs);
 
     EXPECT_TRUE(allf.isReady());
-    EXPECT_FALSE(allf.getTry().hasException());
+    EXPECT_FALSE(allf.result().hasException());
 
     auto& results = allf.value();
     EXPECT_EQ(42, results[0].value());
@@ -160,7 +160,7 @@ TEST(Collect, collectAllUnsafe) {
     promises[3].setException(eggs);
 
     EXPECT_TRUE(allf.isReady());
-    EXPECT_FALSE(allf.getTry().hasException());
+    EXPECT_FALSE(allf.result().hasException());
 
     auto& results = allf.value();
     EXPECT_EQ(42, results[0].value());
@@ -531,7 +531,7 @@ TEST(Collect, collectAny) {
       futures.push_back(p.getFuture());
     }
 
-    auto anyf = collectAny(futures).thenValue(
+    auto anyf = collectAnyUnsafe(futures).thenValue(
         [](std::pair<size_t, Try<int>> p) { EXPECT_EQ(42, p.second.value()); });
 
     promises[3].setValue(42);
@@ -656,9 +656,10 @@ TEST(Collect, alreadyCompleted) {
       fs.push_back(makeFuture(i));
     }
 
-    collectAny(fs).thenValue([&](std::pair<size_t, Try<int>> p) {
-      EXPECT_EQ(p.first, p.second.value());
-    });
+    collectAny(fs).toUnsafeFuture().thenValue(
+        [&](std::pair<size_t, Try<int>> p) {
+          EXPECT_EQ(p.first, p.second.value());
+        });
   }
 }
 
@@ -974,6 +975,7 @@ TEST(Collect, collectVariadic) {
   Future<int> fi = pi.getFuture();
   bool flag = false;
   collect(std::move(fb), std::move(fi))
+      .toUnsafeFuture()
       .thenValue([&](std::tuple<bool, int> tup) {
         flag = true;
         EXPECT_EQ(std::get<0>(tup), true);
@@ -1013,7 +1015,7 @@ TEST(Collect, collectVariadicWithException) {
   EXPECT_FALSE(f.isReady());
   pi.setException(eggs);
   EXPECT_TRUE(f.isReady());
-  EXPECT_TRUE(f.getTry().hasException());
+  EXPECT_TRUE(f.result().hasException());
   EXPECT_THROW(std::move(f).get(), eggs_t);
 }
 

folly/futures/test/ReduceTest.cpp

@@ -95,14 +95,16 @@ TEST(Reduce, chain) {
   };
 
   {
-    auto f = collectAll(makeFutures(3)).reduce(0, [](int a, Try<int>&& b) {
-      return a + *b;
-    });
+    auto f = collectAll(makeFutures(3))
+                 .toUnsafeFuture()
+                 .reduce(0, [](int a, Try<int>&& b) { return a + *b; });
     EXPECT_EQ(6, std::move(f).get());
   }
   {
     auto f =
-        collect(makeFutures(3)).reduce(0, [](int a, int&& b) { return a + b; });
+        collect(makeFutures(3)).toUnsafeFuture().reduce(0, [](int a, int&& b) {
+          return a + b;
+        });
     EXPECT_EQ(6, std::move(f).get());
   }
 }

folly/futures/test/WaitTest.cpp

@@ -148,7 +148,7 @@ TEST(Wait, waitWithDuration) {
     vector<Future<bool>> v_fb;
     v_fb.push_back(makeFuture(true));
     v_fb.push_back(makeFuture(false));
-    auto f = collectAll(v_fb);
+    auto f = collectAll(v_fb).toUnsafeFuture();
     f.wait(milliseconds(1));
     EXPECT_TRUE(f.isReady());
     EXPECT_EQ(2, f.value().size());
@@ -159,7 +159,7 @@ TEST(Wait, waitWithDuration) {
     Promise<bool> p2;
     v_fb.push_back(p1.getFuture());
     v_fb.push_back(p2.getFuture());
-    auto f = collectAll(v_fb);
+    auto f = collectAll(v_fb).toUnsafeFuture();
     f.wait(milliseconds(1));
     EXPECT_FALSE(f.isReady());
     p1.setValue(true);