[CFE] Confusing async return semantics clarified -- no action needed

[eernstg]

A similar topic is currently being discussed in #44395, this is different, but shares several characteristics.

Consider the following program:

Future<int> f() async {
  await null; // We're not evaluating the `throw` expression synchronously.
  throw 'f';
}

Future<Future<int>> g() async {
  await null; // We do not reach `f()` synchronously.
  return f();
}

void main() async {
  var x = await g(); // Should just assign the `Future<int>` to `x`, but throws.
  print('Done');
}

The invocation f() should return a Future<int> which is completed with the thrown object and stack trace arising from the throw expression in the body of f. That future has a type (statically and dynamically) which is the future value type of g, and hence that future should be used to complete the Future<Future<int>> which has been returned when g was invoked. In main, this Future<Future<int>> is the value of the evaluation of g(), and it is awaited. The resulting object should be the Future<int> which was completed with an error.

However, the program actually throws rather than assigning that future to x, in dart, whereas dart2js correctly reaches the print statement and completes the execution of main (and at that point it throws, because remaining computations are handled after main terminates).

I would consider this to be a bug in the VM, or in the VM specific Kernel code transformations performed by the CFE. However, I think it would be useful to double check the intended behavior in the language team first.

@lrhn, @leafpetersen, @natebosch, @jakemac53, @stereotype441, @munificent, WDYT?

[lrhn]

The g function should indeed return the future as value, not await it and return its value, so g's returned future is completed with the future returned by f().

The var x = await g(); should then assign that value to x and continue unabated, printing Done and then later report an uncaught async error.

That's what dart2js does (according to dartpad.dev), but the VM doesn't print Done.
With more instrumentation:

Future<int> f() async {
  print("4");
  await null; // We're not evaluating the `throw` expression synchronously.
  print("7");
  throw 'f';
}

Future<Future<int>> g() async {
  print("1");
  await null; // We do not reach `f()` synchronously.
  print("3");
  var res = f();
  // res.ignore(); // makes it work.
  print("5: $res"); // Should be `5: Instance of 'Future<int>`
  return res;
}

void main() async {
  print("0");
  var fut = g();
  print("2: $fut"); // Should be `2: Instance of 'Future<Future<int>>`
  var x = await fut; // Should just assign the `Future<int>` to `x`, but throws.
  print("6: $x"); // Should be `6: Instance of 'Future<int>`
  print('Done');
}

This prints 1-7 in order when compiled with dart2js (with Done coming before 7 since no-one awaits the completion of the f() future).
On the VM it skips 6 and "Done". The types of the futures are fine. If f returned 1 instead, the VM behaves correctly, so this is error related.

BUT, If I insert res.ignore(); in g, so we don't get any "uncaught async error" errors, then the VM does print 6. It just does so after it prints 7, which probably means that the VM's timing is different, and the uncaught error from f happens to kill the isolate before the await fut; completes.

So, the VM is doing the right thing in a different order. That's at least curious, but not necessarily wrong.

Even more curious, even if I set isolate.current.setErrorsFatal(false); (and wait for a following ping to make sure it's received), that's not enough to keep the isolate alive. Adding an error listener to the isolate is.
(Adding this at the beginning of main:

  Isolate.current.setErrorsFatal(false);
  var port = ReceivePort();
  var eport = ReceivePort();
  Isolate.current.addErrorListener(eport.sendPort);
  eport.forEach((e) {
    print("Reported uncaught: e[0]");
    eport.close();
  }).ignore();
  Isolate.current.ping(port.sendPort);
  await port.first;

and if I drop either setErrorsFatal or addErrrorListener, then we never get to "6". I think that's a bug, setErrorsFatal(false) should be enough.)

OK, let me do the full async analysis on the original program:

Enter main (create return future for async function, F0)
Call g() from main
Enter g (create return future for async function, F1)
Create Future.value(null) for await null - F2
await F2 in g (this or previous step scheduled microtask to notify F2's listeners of completion) - M1
(When F2 was created, either a microtask was scheduled to complete it, or it was created as completed, and then the await on F2 scheduled a microtask to call the callback.)
Return F1 from g to main - F1
await F1 in main's await g().

Microtask M1 runs, notifying listeners on F2.
The await null on F2 in g completes with the value null.
Call f() from g
Enter f (create return future for async function, F3)
create Future.value(null) - F4
await F4 (this or previous step scheduled microtask to notify F3's listeners of completion) - M2
return F3 from f() to g
The async g function returns the future value F3.
This completes F1 with F3 as value.
Schedule listeners on F1 to be notified - M3
(Here we can either synchronously or asynchronously notify the listeners on F2, in which case M3 goes before M2,
otherwise M2 goes before M3.)

When M3 runs, it notifies listeners on F1
await on F1 in main completes
Prints "Done".
main returns null.
Future F0 is completed with null.
(Schedules a microtask to notify listeners, there are none, nobody notices)

When M2 runs, it notifies listeners on F4
await null in f() completes.
f throws "f"
f completes with error object "f"
F3 is completed with that error
schedule listeners on F3 to be notified - M4 (which can again be synchronous)

When M4 runs, it notifies listeners on F3.
There are none, and it's an error future, so we report an uncaught top-level error.
This kills the isolate.

We can't guarantee the ordering of these future notifications because we don't actually promise anything, and implementations can use either sync or async completers to implement it (using a sync completer is a completely valid implementation choice when the async function cannot complete synchronously).
We do know that M1 runs first. and that M2 runs before M4, but M3 can run anywhere after M1.
If M2+M4 runs before M3, the isolate dies before printing "Done". If M3 runs before that, it prints "Done" before exiting.

Adding res.ignore() above makes the M4 notification not report an uncaught error, and therefore the isolate surivives long enough to see M3 complete and main print "Done".

All in all, I don't think any of this is wrong except that setErrorsFatal doesn't work without addErrorListener.

[eernstg]

Why wouldn't it still be a bug that the VM throws at await fut?

When M4 runs, it notifies listeners on F3.
There are none, and it's an error future, so we report an uncaught top-level error.

I guess the crucial point is that F1 does not count as a listener on F3.

Even though we could say, intuitively, that the error completion has not been orphaned as long as we may await F1 and then await F3, it still counts as an orphaned error completion unless F3 is actually awaited before it is completed.

Is that really the semantics that we want? Maybe the message is that you simply never want to use Future<Future<T>> for any T after all?

[lrhn]

The VM doesn't throw at await fut. It just terminates the isolate before it's notified that fut has completed with a perfectly valid Future<int> value. A future value which happens to have already completed with an error at that point, but await fut; doesn't care.

I guess the crucial point is that F1 does not count as a listener on F3.

It doesn't. From the perspective of g (which returns F1), the F3 future returned by f is just a value. It doesn't even know that it's a future, because all it checks is whether it's a Future<Future<int>>, which it isn't. Then it plows full-speed into the value propagation track and completes F1 with F3 as a value. No-one adds a listener on F3 at any point, nor should they since no code here actually waits for, or wants, or even knows what to do with, an int.

The semantics of error futures is that it's an uncaught async error if they complete with an error without having any listener. When you call completeError on a Completer, it's an asynchronous completion, and you have until some later microtask to add an error listener. The general rule is that if a function returns a Future, you must synchronously await it or attach an error listener. The next microtask might be too late. (Using a synchronous completer, you should be certain that you have already returned/released the future in an earlier microtask, otherwise it's too soon to complete with an error.)
Is this the semantics we want? I don't know, but it's the one we've had since Dart 1.0, so changing it is likely going to be confusing. The goal was that no error was ever lost by accident. (We can get philosophical: If an error happens in the woods and no-one is there to see it, is it really an error that should be printed on stderr?)

And also heck yes, you never want to use Future<Future<X>>. Ever. For anything. It's bad design! (Why wait for something that you then have to wait for again? Just make it one Future<X>) And it's dangerous because you risk not getting the inner future in time to attach error handlers to it, because we do not promise anything about the order of futures actually being completed, only ever that it's "in a later microtask". If you choose to do Future<Future<X>> anyway, you should either make sure the inner future never has an error, or make sure to .ignore() the error when you receive it, so the error won't be considered uncaught. It's a high caliber foot gun.

You can end up with a Future<Future<X>> by "accident", if you have a generic asynchronous API which you instantiate with a future. In practice that API is likely flawed, and should be documented as "don't use with futures", because you can't ignore asynchrony in asynchronous code. (You should not have a stream of futures either, it's also a double wait where you should only have a single wait, don't tell anyone until the result is actually ready).

[eernstg]

@lrhn, thanks for the great analysis! I think there's no obvious improvement on the table, so I'm closing this.