From 33b7b452525858c0d45e1773cd47dd39607f3aa1 Mon Sep 17 00:00:00 2001
From: Florian Loitsch <floitsch@google.com>
Date: Thu, 18 Aug 2016 14:09:20 +0200
Subject: [PATCH] Deal with synchronous errors in Future.wait.

Synchronous errors are caught and piped into the returned future. This makes handling errors in Future.wait uniform.

Fixes #23656
BUG= http://dartbug.com/23656
R=lrn@google.com

Review URL: https://codereview.chromium.org/2252823004 .
---
 CHANGELOG.md                     |  7 ++++
 sdk/lib/async/future.dart        | 67 ++++++++++++++++++++------------
 tests/lib/async/future_test.dart | 21 ++++++++++
 3 files changed, 71 insertions(+), 24 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 92e389917b7..4d01fcad8cf 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,10 @@
+## 1.20.0
+
+### Core library changes
+* `dart:async`
+  * `Future.wait` now catches synchronous errors and returns them in the
+    returned Future.
+
 ## 1.19.0
 
 ### Language changes
diff --git a/sdk/lib/async/future.dart b/sdk/lib/async/future.dart
index b4d9c243393..c64e1c5e031 100644
--- a/sdk/lib/async/future.dart
+++ b/sdk/lib/async/future.dart
@@ -287,32 +287,51 @@ abstract class Future<T> {
       }
     }
 
-    // As each future completes, put its value into the corresponding
-    // position in the list of values.
-    for (Future future in futures) {
-      int pos = remaining++;
-      future.then((Object/*=T*/ value) {
-        remaining--;
-        if (values != null) {
-          values[pos] = value;
-          if (remaining == 0) {
-            result._completeWithValue(values);
+    try {
+      // As each future completes, put its value into the corresponding
+      // position in the list of values.
+      for (Future future in futures) {
+        int pos = remaining;
+        future.then((Object/*=T*/ value) {
+          remaining--;
+          if (values != null) {
+            values[pos] = value;
+            if (remaining == 0) {
+              result._completeWithValue(values);
+            }
+          } else {
+            if (cleanUp != null && value != null) {
+              // Ensure errors from cleanUp are uncaught.
+              new Future.sync(() { cleanUp(value); });
+            }
+            if (remaining == 0 && !eagerError) {
+              result._completeError(error, stackTrace);
+            }
           }
-        } else {
-          if (cleanUp != null && value != null) {
-            // Ensure errors from cleanUp are uncaught.
-            new Future.sync(() { cleanUp(value); });
-          }
-          if (remaining == 0 && !eagerError) {
-            result._completeError(error, stackTrace);
-          }
-        }
-      }, onError: handleError);
+        }, onError: handleError);
+        // Increment the 'remaining' after the call to 'then'.
+        // If that call throws, we don't expect any future callback from
+        // the future, and we also don't increment remaining.
+        remaining++;
+      }
+      if (remaining == 0) {
+        return new Future.value(const []);
+      }
+      values = new List/*<T>*/(remaining);
+    } catch (e, st) {
+      // The error must have been thrown while iterating over the futures
+      // list, or while installing a callback handler on the future.
+      if (remaining == 0 || eagerError) {
+        // Just complete the error immediately.
+        result._completeError(e, st);
+      } else {
+        // Don't allocate a list for values, thus indicating that there was an
+        // error.
+        // Set error to the caught exception.
+        error = e;
+        stackTrace = st;
+      }
     }
-    if (remaining == 0) {
-      return new Future.value(const []);
-    }
-    values = new List/*<T>*/(remaining);
     return result;
   }
 
diff --git a/tests/lib/async/future_test.dart b/tests/lib/async/future_test.dart
index 52185f2ae80..71c509b6fd3 100644
--- a/tests/lib/async/future_test.dart
+++ b/tests/lib/async/future_test.dart
@@ -876,6 +876,26 @@ void testWaitCleanUpError() {
   });
 }
 
+void testWaitSyncError() {
+  var cms = const Duration(milliseconds: 100);
+  var cleanups = new List.filled(3, false);
+  var uncaughts = new List.filled(3, false);
+  asyncStart();
+  asyncStart();
+  runZoned(() {
+    Future.wait(new Iterable.generate(5, (i) {
+      if (i != 3) return new Future.delayed(cms * (i + 1), () => i);
+      throw "throwing synchronously in iterable";
+    }), cleanUp: (index) {
+      Expect.isFalse(cleanups[index]);
+      cleanups[index] = true;
+      if (cleanups.every((x) => x)) asyncEnd();
+    });
+  }, onError: (e, s) {
+    asyncEnd();
+  });
+}
+
 void testBadFuture() {
   var bad = new BadFuture();
   // Completing with bad future (then call throws) puts error in result.
@@ -1075,6 +1095,7 @@ main() {
 
   testWaitCleanUp();
   testWaitCleanUpError();
+  testWaitSyncError();
 
   testBadFuture();