Future.wait should gracefully handle futures completing synchronously

[nex3]

The following code breaks Future.wait:

    main() {
      var completer = new Completer.sync();
      Future.wait([1, 2].map((i) {
        if (i == 1) return completer.future;
        completer.complete();
        return new Future.value();
      }));
    }

This produces the following obscure error message:

Unhandled exception:
Uncaught Error: null

­0 _rootHandleUncaughtError.<anonymous closure> (dart:async/zone.dart:883)

­1 _asyncRunCallbackLoop (dart:async/schedule_microtask.dart:41)

­2 _asyncRunCallback (dart:async/schedule_microtask.dart:48)

­3 _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:84)

­4 _startIsolate (dart:isolate-patch/isolate_patch.dart:244)

­5 _startMainIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:192)

­6 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:130)

This may seem like a contrived example, but when many futures and completers are being passed around, it can come up in practice. This was masking the underlying cause of 21068, for instance.

[floitschG]

The documentation for Completer.sync explicitly states: "This constructor should be avoided unless the completion of the future is known to be the final result of another asynchronous operation.".
If this rule is respected Future.wait can never enter the situation you describe.

---

Added Invalid label.

[nex3]

There's no situation that should cause the core libraries to crash my program with such an unhelpful message. If I'm doing something invalid here, at the very least it should crash with a message indicating what I'm doing wrong and a stack trace indicating where I'm doing it.

It's also unreasonable—and impossible in practice—to strictly forbid the use of Completer.sync for synchronous event handling. Systems like barback that have strong requirements about synchronous versus asynchronous propagation of information will exist; these systems will use the Future/Completer infrastructure, because it's far easier than manually reimplementing it and it works well with the outside world and when integrating with asynchronous code.

Code using Completer.sync in this way exists and indeed is part of the core platform infrastructure. Please don't close this bug, which demonstrates a clear instance of unusable behavior by dart:async, because you disagree philosophically with that code.

---

Added Triaged label.

[lrhn]

I'll bet there are lots of cases that will cause the core libraries to fail unhelpfully if you break the contract of an interface.
We promise, and it's one of the few things we do promise, that a future.then callback will not trigger until the current microtask is done. That is part of the contract of Future. That is why completing with a sync controller while not in tail-position of another event, is not just discouraged - it's broken.

It might have been a mistake to make sync controllers/completers public - or make the constructor so close to the normal one. They are powerful, to the point of being able to break other code if misused. It's like having a List that changes its length every time you read it: list[list.length - 1] -> RangeError. That's not something you can protect against, it's fundamentally broken.

We can try hardening Future.wait, e.g., by making it throw if it detects a too-early completion, or by working around it and still complete, but that will just be one place where the code breaks. There are others, and I generally don't want to check against broken behavior.

[lrhn]

Or, to put it more bluntly: I'd rather remove sync completers from the library than try to write all code to guard against incorrect use of them.

The event model of futures is clear: then-callbacks happen as a separate microtask. Sync controllers are available only as optimizations, where one event knows that it is done, and it can directly/synchronously transition into a what is morally another microtask.

If you are using synchronous completers and futures in any other way, you are inventing your own future semantics, and it is incompatible with the rest of the system. That's perfectly fine, but you can't mix your futures with dart:async code and expect it to work.

[lrhn]

Added NotPlanned label.