Exception from Future seems leaked in async generator even error catching code has been executed

[timnew]

Not sure whether it is a bug or so, but it definitely quite surprising.

I noticed the exception from the future still bubble up even when a try catch or Future.catch was applied.

Here is some experiment I tried:

Stream<String> test1(Future<String> future) async* {
  try {
    yield "a";
    yield await future;
    yield "b";
  } catch (ex) {
    yield ex.toString();
  }
  yield "c";
}

Stream<String> test2(Future<String> future) async* {  
  yield "a";
  yield await future.catchError((ex) => ex.toString()); 
  yield "c";
}


Stream<String> test3(void Function() action) async* {
  try{
    action();
  }catch (ex) {
    yield ex.toString();
  }
}


Stream<String> test4() async* {
  try {
    yield "a";
    yield await Future.error("error");
    yield "b";
  } catch (ex) {
    yield ex.toString();
  }
  yield "c";
}

Stream<String> test5() async* {  
  yield "a";
  yield await Future<String>.error("error").catchError((ex) => ex.toString()); 
  yield "c";
}


Stream<String> test6() async* {
  try{
    throw "error";
  }catch (ex) {
    yield ex.toString();
  }
}

Future<void> printStream(Stream stream) async => print(await stream.toList());

void main()  async {
  await printStream(test1(Future.error("error")));  
  await printStream(test2(Future.error("error")));
  
  await printStream(test3(()=> throw "error"));
  await printStream(test4());  
  await printStream(test5());
  await printStream(test6());
}

I would expect the code wrong without any error.
But in practise, error was thrown from test1 and test2, but not the others.

Here is the intractable example: https://dartpad.dev/4bf9caaca179b50000b8abed1707a7d4?null_safety=true

Not sure is this an expected behaviour or some sorts of bug?

[timnew]

My guess on the reason behind the issue is:

1. Future.error throws the exception in next micro task.
2. If the catch hasn't been registered when the error is raised, it will be treated as a unhandled exception.
3. async generator is actually expanded into a much more complicated shape, depends flow of the code.
4. the expanded generator and error raising logic formed a racing condition. And the result depends on the shape of expanded code.
   So that's why If the Future is locally created, the uncaught error won't happen ( as I assume the code get executed in the same micro task).

The issue is quite annoying is because it is a false positive error, and it does fails tests. Also it seems to be impossible to fix on the developer side.

[lrhn]

Correct.
An async* function is delayed in its start, so the error future you pass into it will be considered unhandled before you get to the await future. It might also delay on yield statements (but usually won't).

Never pass around futures that you haven't already handled the error for, not unless you know for certain that they'll still get a listener synchronously.

[timnew]

@lrhn Is it possible to let this issue open? As I feel it is quite an unexpected issue, and the behaviour is quite inconsistent.

Never pass around futures that you haven't already handled the error for, not unless you know for certain that they'll still get a listener synchronously.

And would you mind to explain this in more details, I found it is fairly hard to achieve in most cases. The only general solution I could imagine would be materialise the error into a Result(from package:async) or so. So ensure there is no error throw, but it also make the code much bloated and complex.

[lrhn]

Whenever you receive a Future from someone else, you should expect it to be able to complete as soon as in the next microtask. Because it can.
The future might complete with an error at that point. If you haven't added an error handler before then, that error will become an uncaught asynchronous error, and might take down the entire program (in the VM, not in the browser).

That's the general rules for futures: You must decide what to do with them immediately, even if the result won't come in until later, because you don't know how much (or how little) later that will be.

That's not usually a problem. Asynchronous code generally await futures immediately when they are available. If you need to wait for multiple futures, you use Future.wait on them (immediately), then await the result.

It's rare that you want to store a future for later, because it's generally a good idea to raise errors as soon as possible.

That's also why passing a Future as an argument is rare in non-platform code. It's better to wait for the result, raise any error, or pass the value as an argument. The exception is functions which abstract over handling the future, but those must then be written carefully and make sure to listen for the result immediately.

Passing a future into an async* function is therefore always a bad idea, because async* functions do not execute immediately, they only start when someone calls listen on the returned stream, and even then they delay for at least one more microtask before they start running.

We can keep the issue open, but I'm not sure for what. There are no plans to change the current behavior (the only thing I'd change if I could is to make async* functions start running immediately), so the only thing we can do is add more documentation. That might be reasonable, I'm not actually aware which documentation we have around this (since I never needed to read it).

@kwalrath Do you know which documentation we have around needing to handle future errors immediately?

[kwalrath]

@kwalrath Do you know which documentation we have around needing to handle future errors immediately?

We discourage using the Future API (encouraging async-await instead), but places that cover Futures in general include these:

• https://dart.dev/guides/language/language-tour#handling-futures (the 4th most popular page on dart.dev, with 8% of pageviews)
• https://dart.dev/codelabs/async-await (10th, 2.5%)
• https://dart.dev/guides/libraries/library-tour#future (21st, 1%)
• https://dart.dev/guides/libraries/futures-error-handling (31st, 0.7%)

I think if we add tips/warnings to the language tour & the async-await codelab, that's our best chance of getting eyeballs.

[kwalrath]

Thinking further... could a lint help here?

/cc @devoncarew

[lrhn]

The most trivial lint would be "don't pass futures to async* functions".

The generalization would be a lint like "handle futures synchronously" (better name needed) which would trigger if you have a future-typed variable which is assigned, and then not read again before the next asynchronous break (await/await for/yield). For an async* function, that should trigger for all Future parameters because the start of the body is itself delayed.

The problem with that is that there can be false positives. If I do var future = something().catchError((_){}); then I explicitly handle the future's errors (which is the real point of requiring you to handle them). Also, if you know the future will never have an error, there is no problem, and any forced handling of errors is unnecessary overhead.