Add an API wrapping runZonedGuarded that surfaces the first error

[natebosch]

First discussed at flutter/flutter#141821 (comment)

The runZonedGuarded API (and runZoned when it is still used with the deprecated onError argument) is tricky to used because of the caveat that errors in the zone cannot escape that zone and a returned Future from the callback may just never complete it it would have been an error.

We should consider an API, here or in the core libraries, that wraps runZonedGuarded and does surface the first error that occurs. To start the discussion:

  /// Runs [callback] in a zone and returns the result or the first
  /// unhandled error that occurs in the zone.
  ///
  /// If an unhandled asynchronous error occurs before the Future
  /// returned from [callback] has completed, the callback result
  /// will be ignored (if it ever completes), and the returned Future
  /// will complete with the same error.
  ///
  /// If there are multiple unhandled asynchronous errors, or if the
  /// Future returned by [callback] has an error after an unhandled
  /// asynchronous error, all but the first are passed to `onError`,
  /// or ignored if there is no `onError`.
  ///
  /// This API may be useful to avoid Futures that never complete
  /// due to an error that cannot escape it's error zone.
  Future<T> Result.runZonedSmuggleErrors(Future<T> Function() callback, {void Function(Object, StackTrace) onError});

cc @lrhn for thoughts.

[lrhn]

We could change runZonedGuarded to let an error completion escape the zone. At least that will prevent the returned future from never completing if the computation fails.

I think that'll be mostly non-breaking for reasonable uses.
I think we should just do that, since I haven't managed to get rid of the error zone blocking errors entirely.

We could also add a flag that makes it replace a successful completion with an error if there has been any unhandled async error before completion.

I'm less convinced by that, because it seems at odds with how async operations behave.

• it only triggers if the async error happens before completion, not after. That's inconsistent. Error handling is hard enough without race conditions.
• should the first sync error not be passed to the onError callback? If we let it escape, then it shouldn't also be unhandled. But what if the main computation does have an error, should we then go back and pass the first unhandled error to onError anyway? And why the first error.

Seems more consistent to not use an existing error, but instead replace the successful completion with a StateError("Async failure during computation"), to signal that the computation was not successful, but leave the actual unhandled errors to all be handled by onError.
It's still a race condition, but at least it's symmetrical in the unhandled errors.

[natebosch]

I think that'll be mostly non-breaking for reasonable uses.

We should go through the breaking change process and double check this assumption. I think the typical patterns to work around current behavior should be OK since they typically check completer.isCompleted and should be able to ignore the extra new error completion, or at least not cause a new exception when it occurs.

I can imagine that some implementations may double-log an error if it starts to also surface as an error future in the calling zone.

Edit: If we only surface this error through the returned Future and stop calling the onError callback with it - as would match the design I proposed here - I do think this could cause problems. It's very likely that existing implementations expect that the onError callback would be called, and likely don't have an equivalent error handler on the Future.

makes it replace a successful completion with an error if there has been any unhandled async error before completion.

I think current workarounds are more likely to surface the error immediately and not wait for the successful completion.

[lrhn]

I checked my assumptions, and it won't work. Turns out the runZoneGuarded returns null in case of a synchronous error. There is no provision for returning Future.value(null) for an async error, and it's probably not even possible to type it. And it's not possible to return null synchronously for an async error.

While we can keep doing that, and make an async error future smuggle the error out (maybe), it's not consistent.
On the other hand, a future which never completes is probably worse than an inconsistent async error.
So I'll try to find a way to make a returned error future be intercepted, letting onError handle the error, and return a future which throws a fixed error from the outside zone, like the StateError above.
Basically change the non-completing future to a future which completes with a marker error, to say that it couldn't complete with the real error.

But it would also change the behavior of never needing to handle errors from a returned future.
(How about we just deprecate runZonedGuarded and create a better API, one which does emit errors in the result, and only cares about catching uncaught errors, and maybe another one which catches all errors and returns Future<void> when it's done.)

I'm not very keen on eagerly completing the returned future with an unhandled async error. For two reasons.

1. It's arbitrary. We take an error from one location and surfaces it in another location. It doesn't stop the computation, and it assumes that an async error in some spun-off computation means that the main computation can't complete. Maybe it can, and that's precisely why the user added onError to gently handle the things that fail, until something succeeds. (Not great design, they should catch errors instead, but it's not an invalid use.)
2. More importantly, it can't really work. The runZonedGuarded isn't required to return a future to begin with. It must return, synchronously, a value of type R?, where we have no idea what R is. We can check if it actually does return a future, but it's incredibly hard to do anything with that future and preserve its type, when we don't know what the type is. (We can use catchError and whenComplete, since those are guaranteed to retain the original type, but we can't change the element type, and we can't create a completer of the same type, which means it'll be very hard to actually smuggle an error past the error zone boundary.)

I almost had a way of smuggling an error past the zone, by doing .asStream() inside the zone, and .first outside, but that still won't work if someone uses a subtype of Future:

class Banana implements Future<bool> { Future<bool> _banana; ... }
...
  Banana createBanana() => ...
  var b = runZoneGuarded(createBanana, (e, s) => ...);

Here the type parameter, and type of of b, is Banana. There is absolutely no way one can intercept a Banana, recognize that it's a future, extract the error, smuggle it out, and put it back into a Banana.

We might be able to do the hack if the type parameter is Future<X> or FutureOr<X>, but we may still end up changing the kind of Future being returned (which could, for example, be a SyncFuture).

So, all in all, runZonedGuarded is probably unsalvageable. Let's scrap it and create a new and better one instead :)
Like you suggested, just without "smuggle" in the name.

[lrhn]

A possible alternative could be:

import "dart:async";

/// Runs [action] in a new [Zone] where [onError] handles uncaught errors.
///
/// Returns the result of [action]. If that result is a [Future], it is
/// copied into a future belonging to the surrounding zone, so that
/// an error can be handled.
Future<R> catchUnhandledAsyncErrors<R>(
  FutureOr<R> Function() action, {
  required void Function(Object, StackTrace) onError,
  ZoneSpecification? zoneSpecification,
}) {
  void handleError(Zone self, ZoneDelegate parent, Zone zone, Object error,
      StackTrace stack) {
    onError(error, stack);
  }

  var spec = zoneSpecification == null
      ? ZoneSpecification(handleUncaughtError: handleError)
      : ZoneSpecification.from(zoneSpecification,
          handleUncaughtError: handleError);

  var outer = Zone.current;
  return runZoned<Future<R>>(zoneSpecification: spec, () {
    FutureOr<R> result;
    try {
      result = action();
    } catch (e, s) {
      return outer.run(() => Future<R>.error(e, s));
    }
    if (result is Future<R>) {
      var c = outer.run(() => Completer<R>.sync());
      result.then(c.complete, onError: c.completeError);
      return c.future;
    } else {
      return Future<R>.value(result);
    }
  });
}


void main() {
  FutureOr<bool> banana() async {
    Future.error("Plantain");
    throw "Banana";
  }
  var f = catchUnhandledAsyncErrors(banana, onError: (e, s) {
    print("Uncaught: $e");
  });
  f.catchError((e, s) {
    print("Caught: $e");
    return true;
  });
}