Use AsyncError more in future error handling.

Avoids some cases of calling `Zone.errorCallback` and getting
an `AsyncError`, then unpacking it again to separate
errors and stack trace objects.

CoreLibraryReviewExempt: No API changes, no platform-specific code change.
Change-Id: I903dbfa6da74cfdf8f00e765e48311cc0cb40cac
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/405022
Commit-Queue: Lasse Nielsen <[email protected]>
Reviewed-by: Nate Bosch <[email protected]>

Author: lrhn

sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart

@@ -127,7 +127,7 @@ _async<T>(Function() initGenerator) {
       if (isRunningAsEvent) {
         _completeWithErrorCallback(asyncFuture, e, s);
       } else {
-        _asyncCompleteWithErrorCallback(asyncFuture, e, s);
+        asyncFuture._asyncCompleteErrorObject(_interceptCaughtError(e, s));
       }
     }
   }

sdk/lib/async/future.dart

@@ -314,9 +314,8 @@ abstract interface class Future<T> {
     try {
       result = computation();
     } catch (error, stackTrace) {
-      var future = new _Future<T>();
-      _asyncCompleteWithErrorCallback(future, error, stackTrace);
-      return future;
+      return _Future<T>()
+        .._asyncCompleteErrorObject(_interceptCaughtError(error, stackTrace));
     }
     return result is Future<T> ? result : _Future<T>.value(result);
   }
@@ -369,10 +368,8 @@ abstract interface class Future<T> {
   ///
   /// final error = await getFuture(); // Throws.
   /// ```
-  factory Future.error(Object error, [StackTrace? stackTrace]) {
-    AsyncError(:error, :stackTrace) = _interceptUserError(error, stackTrace);
-    return new _Future<T>.immediateError(error, stackTrace);
-  }
+  factory Future.error(Object error, [StackTrace? stackTrace]) =>
+      _Future<T>.immediateError(_interceptUserError(error, stackTrace));
 
   /// Creates a future that runs its computation after a delay.
   ///
@@ -561,25 +558,21 @@ abstract interface class Future<T> {
         return _future.._completeWithValue(<T>[]);
       }
       values = List<T?>.filled(remaining, null);
-    } catch (e, st) {
+    } catch (e, s) {
       // The error must have been thrown while iterating over the futures
       // list, or while installing a callback handler on the future.
       // This is a breach of the `Future` protocol, but we try to handle it
       // gracefully.
       if (remaining == 0 || eagerError) {
-        // Throw a new Future.error.
-        // Don't just call `_future._completeError` since that would propagate
-        // the error too eagerly, not giving the callers time to install
-        // error handlers.
-        // Also, don't use `_asyncCompleteError` since that one doesn't give
-        // zones the chance to intercept the error.
-        return new Future.error(e, st);
+        // Complete asynchronously to give receiver time to handle
+        // the result.
+        return _future.._asyncCompleteErrorObject(_interceptCaughtError(e, s));
       } 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;
+        stackTrace = s;
       }
     }
     return _future;
@@ -716,9 +709,11 @@ abstract interface class Future<T> {
         try {
           result = action();
         } catch (error, stackTrace) {
-          // Cannot use _completeWithErrorCallback because it completes
+          // Cannot use `_completeWithErrorCallback` because it completes
           // the future synchronously.
-          _asyncCompleteWithErrorCallback(doneSignal, error, stackTrace);
+          doneSignal._asyncCompleteErrorObject(
+            _interceptCaughtError(error, stackTrace),
+          );
           return;
         }
         if (result is Future<bool>) {
@@ -1331,7 +1326,7 @@ abstract interface class Completer<T> {
   bool get isCompleted;
 }
 
-// Helper function completing a _Future with error, but checking the zone
+// Helper function completing a _Future with an error, but checking the zone
 // for error replacement and missing stack trace first.
 // Only used for errors that are *caught*.
 // A user provided error object should use `_interceptUserError` which
@@ -1341,24 +1336,5 @@ void _completeWithErrorCallback(
   Object error,
   StackTrace stackTrace,
 ) {
-  var replacement = _interceptError(error, stackTrace);
-  if (replacement != null) {
-    error = replacement.error;
-    stackTrace = replacement.stackTrace;
-  }
-  result._completeError(error, stackTrace);
-}
-
-// Like [_completeWithErrorCallback] but completes asynchronously.
-void _asyncCompleteWithErrorCallback(
-  _Future result,
-  Object error,
-  StackTrace stackTrace,
-) {
-  var replacement = _interceptError(error, stackTrace);
-  if (replacement != null) {
-    error = replacement.error;
-    stackTrace = replacement.stackTrace;
-  }
-  result._asyncCompleteError(error, stackTrace);
+  result._completeErrorObject(_interceptCaughtError(error, stackTrace));
 }

sdk/lib/async/future_impl.dart

@@ -24,11 +24,28 @@ AsyncError? _interceptError(Object error, StackTrace? stackTrace) {
   return replacement;
 }
 
+/// As [_interceptError], but an `AsyncError` of the error if not replaced.
+///
+/// Also like [_interceptUserError], but for errors that are already
+/// asynchronous.
+AsyncError _interceptCaughtError(Object error, StackTrace? stackTrace) =>
+    _interceptError(error, stackTrace) ?? AsyncError(error, stackTrace);
+
 /// Used for user-provided error and stack trace that are to become async
 /// errors.
 ///
 /// Allows `Zone.current.errorCallback` to intercept and modify the error,
 /// and sets the stack trace on the error as if it was thrown.
+///
+/// Used for errors given to async functions like [Completer.completeError]
+/// or [StreamController.addError].
+///
+/// For errors that are caught (and have therefore been thrown already)
+/// use [_interceptCaughtError], which doesn't try to set a stack trace on the
+/// error object.
+///
+/// Errors that are already asynchronous (coming out of streams or futures)
+/// should not call [Zone.errorCallback].
 AsyncError _interceptUserError(Object error, StackTrace? stackTrace) {
   var zone = Zone.current;
   if (!identical(zone, _rootZone)) {
@@ -64,12 +81,11 @@ abstract class _Completer<T> implements Completer<T> {
   @pragma("wasm:entry-point")
   void completeError(Object error, [StackTrace? stackTrace]) {
     if (!future._mayComplete) throw new StateError("Future already completed");
-    AsyncError(:error, :stackTrace) = _interceptUserError(error, stackTrace);
-    _completeError(error, stackTrace);
+    _completeErrorObject(_interceptUserError(error, stackTrace));
   }
 
   // Overridden by either a synchronous or asynchronous implementation.
-  void _completeError(Object error, StackTrace stackTrace);
+  void _completeErrorObject(AsyncError error);
 
   // The future's _isComplete doesn't take into account pending completions.
   // We therefore use _mayComplete.
@@ -85,8 +101,8 @@ class _AsyncCompleter<T> extends _Completer<T> {
     future._asyncComplete(value == null ? value as dynamic : value);
   }
 
-  void _completeError(Object error, StackTrace stackTrace) {
-    future._asyncCompleteError(error, stackTrace);
+  void _completeErrorObject(AsyncError error) {
+    future._asyncCompleteErrorObject(error);
   }
 }
 
@@ -100,8 +116,8 @@ class _SyncCompleter<T> extends _Completer<T> {
     future._complete(value == null ? value as dynamic : value);
   }
 
-  void _completeError(Object error, StackTrace stackTrace) {
-    future._completeError(error, stackTrace);
+  void _completeErrorObject(AsyncError error) {
+    future._completeErrorObject(error);
   }
 }
 
@@ -351,9 +367,8 @@ class _Future<T> implements Future<T> {
     _setValue(value);
   }
 
-  _Future.immediateError(var error, StackTrace stackTrace)
-    : _zone = Zone._current {
-    _asyncCompleteError(error, stackTrace);
+  _Future.immediateError(AsyncError error) : _zone = Zone._current {
+    _asyncCompleteErrorObject(error);
   }
 
   /// Creates a future that is already completed with the value.
@@ -497,10 +512,6 @@ class _Future<T> implements Future<T> {
     _resultOrListeners = error;
   }
 
-  void _setError(Object error, StackTrace stackTrace) {
-    _setErrorObject(new AsyncError(error, stackTrace));
-  }
-
   /// Copy the completion result of [source] into this future.
   ///
   /// Used when a chained future notices that its source is completed.
@@ -722,14 +733,18 @@ class _Future<T> implements Future<T> {
     _propagateToListeners(this, listeners);
   }
 
-  void _completeError(Object error, StackTrace stackTrace) {
+  void _completeErrorObject(AsyncError error) {
     assert(!_isComplete);
 
     _FutureListener? listeners = _removeListeners();
-    _setError(error, stackTrace);
+    _setErrorObject(error);
     _propagateToListeners(this, listeners);
   }
 
+  void _completeError(Object error, StackTrace stackTrace) {
+    _completeErrorObject(AsyncError(error, stackTrace));
+  }
+
   // Completes future in a later microtask.
   void _asyncComplete(FutureOr<T> value) {
     assert(!_isComplete); // Allows both pending complete and incomplete.
@@ -809,11 +824,15 @@ class _Future<T> implements Future<T> {
   }
 
   void _asyncCompleteError(Object error, StackTrace stackTrace) {
+    _asyncCompleteErrorObject(AsyncError(error, stackTrace));
+  }
+
+  void _asyncCompleteErrorObject(AsyncError error) {
     assert(!_isComplete);
 
     _setPendingComplete();
     _zone.scheduleMicrotask(() {
-      _completeError(error, stackTrace);
+      _completeErrorObject(error);
     });
   }
 
@@ -919,7 +938,7 @@ class _Future<T> implements Future<T> {
                 joinedResult._completeWithResultOf(originalSource);
               },
               onError: (Object e, StackTrace s) {
-                joinedResult._completeError(e, s);
+                joinedResult._completeErrorObject(AsyncError(e, s));
               },
             );
             listenerValueOrError = joinedResult;
@@ -1020,9 +1039,11 @@ class _Future<T> implements Future<T> {
     Timer timer;
     if (onTimeout == null) {
       timer = new Timer(timeLimit, () {
-        _future._completeError(
-          new TimeoutException("Future not completed", timeLimit),
-          StackTrace.current,
+        _future._completeErrorObject(
+          AsyncError(
+            new TimeoutException("Future not completed", timeLimit),
+            StackTrace.current,
+          ),
         );
       });
     } else {
@@ -1035,7 +1056,7 @@ class _Future<T> implements Future<T> {
         try {
           _future._complete(zone.run(onTimeoutHandler));
         } catch (e, s) {
-          _future._completeError(e, s);
+          _future._completeErrorObject(AsyncError(e, s));
         }
       });
     }
@@ -1049,7 +1070,7 @@ class _Future<T> implements Future<T> {
       onError: (Object e, StackTrace s) {
         if (timer.isActive) {
           timer.cancel();
-          _future._completeError(e, s);
+          _future._completeErrorObject(AsyncError(e, s));
         }
       },
     );