Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"await for" errors are handled too late #45645

Open
nex3 opened this issue Apr 9, 2021 · 3 comments
Open

"await for" errors are handled too late #45645

nex3 opened this issue Apr 9, 2021 · 3 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@nex3
Copy link
Member

nex3 commented Apr 9, 2021

Errors that are thrown in the body of an await for within an async function aren't forwarded through that function's future until the next event is emitted by the stream in question. For example:

Stream<int> generate() async* {
  yield 1;
  await Future.delayed(Duration(seconds: 5));
  yield 2;
}

Future<void> wrapper() async {
  await for (var value in generate()) {
    print("handling ${value}");
    throw "oh no";
  }
}

void main() {
  wrapper().catchError((error) {
    print("caught $error");
  });
}

This prints 1, and then only after five seconds does it print caught oh no. This is a pretty substantial problem: certain streams may emit events separated by a large amount of time, and some (such as stdin) may never emit another event. This could cause the error to be silently ignored, potentially leading to deadlocks.

Instead of this behavior, I would expect an error to immediately cancel the subscription to the stream and forward the error to the outer future.

@nex3
Copy link
Member Author

nex3 commented Apr 9, 2021

Note that this also happens when using Stream.forEach(), so the underlying issue may be in dart:async. I'm currently using the following extension to work around it:

extension StreamExtension<T> on Stream<T> {
  /// Workaround for dart-lang/sdk#45645.
  CancelableFuture<void> forEachFixed(FutureOr<void> callback(T element)) {
    var completer = new Completer<void>();

    var subscription = this.listen(null, onError: completer.completeError, onDone: completer.complete);
    subscription.onData((event) async {
      subscription.pause();
      try {
        await callback(event);
        subscription.resume();
      } catch (error, stackTrace) {
        subscription.cancel();
        completer.completeError(error, stackTrace);
      }
    });

    return completer.future;
  }
}

nex3 added a commit to sass/dart-sass that referenced this issue Apr 10, 2021
@lrhn
Copy link
Member

lrhn commented Apr 10, 2021

The issue with await for is likely an issue with async*. When you cancel the subscription, you wait for the future returned by cancel to complete. It does that when the async* function completes (after running through any finally blocks).

Our async* functions are implemented incorrectly on some platforms (maybe all, not sure any more). They do not check after the yield has synchronously delivered an event whether they have been cancelled or paused, they just continue execution.
That means that the subscription.cancel call must wait until the next yield to exit the async* function.

The reason your "fixed" forEach appears to work is that it forgets to await the subscription.cancel(). If it did that, as it should, it too would be delayed until the cancel has properly completed and the async* method has terminated.

See: #34775

nex3 added a commit to sass/dart-sass that referenced this issue Apr 13, 2021
nex3 added a commit to sass/dart-sass that referenced this issue Apr 14, 2021
@keertip keertip added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async labels Apr 14, 2021
nex3 added a commit to sass/dart-sass that referenced this issue Apr 14, 2021
nex3 added a commit to sass/dart-sass that referenced this issue Apr 14, 2021
nex3 added a commit to sass/dart-sass that referenced this issue Apr 14, 2021
@lrhn lrhn added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async labels Sep 23, 2021
@mraleph
Copy link
Member

mraleph commented Sep 23, 2021

/cc @mkustermann @cskau-g

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants