Make collectAll, collectAllRange, collectAllWindowed consistently rethrow...

Make collectAll, collectAllRange, collectAllWindowed consistently rethrow exception from first failure

Summary:
The void-returning variants of collectAllRange() and collectAllWindowed() were
previously failing with the error from the first task to fail whereas the other
variants of collectAll() were failing with an arbitrary error (typically first
by argument order) in the case of multiple errors.

This was problematic because after the first task failed it requests cancellation
of the other tasks, which then often will complete with an 'OperationCancelled'
error. This mean that, unless the original error occurred on the first failed
task in argument order, the error that caused the cancellation would be discarded
and instead, the collectAll() operation would result in an 'OperationCancelled'
error.

This change updates those algorithms to now consistently fail with the error
from the first task to fail (in time) as this error will typically be the most
useful in determining the root cause of the failure.

Also it now discards any errors that occur in child tasks if they fail after
cancellation was requested on the parent task. In this case the collectAll()
will complete with an 'OperationCancelled' error.

Also made a few drive-by changes to avoid rethrowing exceptions where possible
by using `co_yield co_error(e)`.

Reviewed By: yfeldblum

Differential Revision: D21681812

fbshipit-source-id: dfe15d0672dcbdeb4e156962cdffe767ddf91a0b

folly/experimental/coro/Collect.h

@@ -82,9 +82,8 @@ using range_reference_t = iterator_reference_t<range_iterator_t<Range>>;
 // request cancellation of any outstanding tasks and the whole collectAll()
 // operation will complete with an exception once all of the operations
 // have completed.  Any partial results will be discarded. If multiple
-// operations fail with an exception then one of the exceptions will be rethrown
-// to the caller (which one is unspecified) and the other exceptions are
-// discarded.
+// operations fail with an exception then the exception from the first task
+// to fail will be rethrown and subsequent errors are discarded.
 //
 // If you need to know which operation failed or you want to handle partial
 // failures then you can use the folly::coro::collectAllTry() instead which
@@ -148,7 +147,7 @@ auto collectAllTry(SemiAwaitables&&... awaitables)
         remove_cvref_t<SemiAwaitables>>...>>;
 
 ////////////////////////////////////////////////////////////////////////
-// rangeCollectAll(RangeOf<SemiAwaitable<T>>&&)
+// collectAllRange(RangeOf<SemiAwaitable<T>>&&)
 //   -> SemiAwaitable<std::vector<T>>
 //
 // The collectAllRange() function can be used to concurrently await a collection
@@ -158,8 +157,8 @@ auto collectAllTry(SemiAwaitables&&... awaitables)
 // If any of the operations fail with an exception then requests cancellation of
 // any outstanding operations and the entire operation fails with an exception,
 // discarding any partial results. If more than one operation fails with an
-// exception then the exception from the first failed operation in the input
-// range is rethrown. Other results and exceptions are discarded.
+// exception then the exception from task that failed first (in time) is
+// rethrown. Other results and exceptions are discarded.
 //
 // If you need to be able to distinguish which operation failed or handle
 // partial failures then use collectAllTryRange() instead.
@@ -242,13 +241,13 @@ auto collectAllTryRange(std::vector<SemiAwaitable> awaitables)
 // If any of the input awaitables fail with an exception then requests
 // cancellation of any incomplete operations and fails the whole
 // operation with an exception. If multiple input awaitables fail with
-// an exception then one of these exceptions will be rethrown and the rest
-// of the results will be discarded.
+// an exception then the exeception from the first task to fail (in time)
+// will be rethrown and the rest of the results will be discarded.
 //
 // If there is an exception thrown while iterating over the input-range then
 // it will still guarantee that any prior awaitables in the input-range will
 // run to completion before completing the collectAllWindowed() operation with
-// an exception.
+// the exception thrown during iteration.
 //
 // The resulting std::vector will contain the results in the corresponding
 // order of their respective awaitables in the input range.

folly/experimental/coro/test/CollectTest.cpp

@@ -144,13 +144,21 @@ struct ErrorA : std::exception {};
 struct ErrorB : std::exception {};
 struct ErrorC : std::exception {};
 
-TEST_F(CollectAllTest, ThrowsOneOfMultipleErrors) {
+TEST_F(CollectAllTest, ThrowsFirstError) {
   bool caughtException = false;
   folly::coro::blockingWait([&]() -> folly::coro::Task<void> {
     try {
       bool throwError = true;
+      // Child tasks are started in-order.
+      // The first task will reschedule itself onto the executor.
+      // The second task will fail immediately and will be the first
+      // task to fail.
+      // Then the third and first tasks will fail.
+      // As the second task failed first we should see its exception
+      // propagate out of collectAll().
       auto [x, y, z] = co_await folly::coro::collectAll(
           [&]() -> folly::coro::Task<int> {
+            co_await folly::coro::co_reschedule_on_current_executor;
             if (throwError) {
               throw ErrorA{};
             }
@@ -172,12 +180,8 @@ TEST_F(CollectAllTest, ThrowsOneOfMultipleErrors) {
       (void)y;
       (void)z;
       CHECK(false);
-    } catch (const ErrorA&) {
-      caughtException = true;
     } catch (const ErrorB&) {
       caughtException = true;
-    } catch (const ErrorC&) {
-      caughtException = true;
     }
   }());
   CHECK(caughtException);
@@ -693,6 +697,32 @@ TEST_F(CollectAllRangeTest, SubtasksCancelledWhenASubtaskFails) {
   }());
 }
 
+TEST_F(CollectAllRangeTest, FailsWithErrorOfFirstTaskToFailWhenMultipleErrors) {
+  using namespace std::chrono_literals;
+
+  folly::coro::blockingWait([]() -> folly::coro::Task<void> {
+    try {
+      co_await folly::coro::collectAllRange(
+          []() -> folly::coro::Generator<folly::coro::Task<void>&&> {
+            co_yield folly::coro::sleep(1s);
+            co_yield[]()->folly::coro::Task<> {
+              co_await folly::coro::sleep(1s);
+              throw ErrorA{};
+            }
+            ();
+            co_yield[]()->folly::coro::Task<> {
+              co_await folly::coro::co_reschedule_on_current_executor;
+              throw ErrorB{};
+            }
+            ();
+            co_yield folly::coro::sleep(2s);
+          }());
+      CHECK(false);
+    } catch (const ErrorB&) {
+    }
+  }());
+}
+
 TEST_F(CollectAllRangeTest, SubtasksCancelledWhenParentTaskCancelled) {
   using namespace std::chrono_literals;
 
@@ -1119,7 +1149,7 @@ TEST_F(CollectAllWindowedTest, VectorOfValueTask) {
   }
 }
 
-TEST_F(CollectAllWindowedTest, PartialFailure) {
+TEST_F(CollectAllWindowedTest, MultipleFailuresPropagatesFirstError) {
   try {
     [[maybe_unused]] auto results =
         folly::coro::blockingWait(folly::coro::collectAllWindowed(
@@ -1142,8 +1172,6 @@ TEST_F(CollectAllWindowedTest, PartialFailure) {
             }(),
             5));
     CHECK(false); // Should have thrown.
-  } catch (ErrorA) {
-    // Expected.
   } catch (ErrorB) {
     // Expected.
   }