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

Future.wait should support Flutter's SynchronousFuture #50204

Open
rrousselGit opened this issue Oct 14, 2022 · 12 comments
Open

Future.wait should support Flutter's SynchronousFuture #50204

rrousselGit opened this issue Oct 14, 2022 · 12 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async type-enhancement A request for a change that isn't a bug

Comments

@rrousselGit
Copy link

Related thread: flutter/flutter#71554

It appears that Future.wait doesn't handle SynchronousFuture gracefully, causing the following code to crash:

Future main() async {
  await Future.wait([
    Future.delayed(Duration(milliseconds: 500)),
    SynchronousFuture(null),
  ]);
}

I am aware that @lrhn mentioned how "SynchronousFuture is inherently incompatible with the specified Future semantics", accompanied by "Don't use SynchronousFuture".

But I don't think this is a reasonable recommendation. SynchronousFuture, although a bit unique, is an important use case.

When caching asynchronous code, this can be the difference between a flicker or not in the UI. As replacing a SynchronousFuture with a normal Future would delay the resolution of the Future by one frame. This would in turn delay the UI rendering by one frame too.

This matters because, besides SynchronousFuture, there is no other reasonable workaround. FuturOr doesn't work for example, as await futureOr does not complete synchronously. At the same time, it is unreasonable to expect users to do:

T result;
if (futureOr is T) {
  result = futureOr;
} else {
  result = await futureOr;
}

Writing such a code is not intuitive and cannot be refactored. Awaiting two FutureOr would involve writing this exact sequence of code twice. As such, this isn't a pattern that can be reasonably used at scale.

So currently the only real solution is to use SynchronousFuture, as this is a reusable solution.

With that consideration, I believe it would be reasonable to update Future.wait to support SynchronousFuture.

@lrhn
Copy link
Member

lrhn commented Oct 14, 2022

Probably unsurprising, but I'm strongly opposed to supporting SynchronousFuture in any way.

Both because it's unnecessarily complicating code that shouldn't need that complication, and because it's a game of infinite whack-a-mole to find all the places where code breaks when presented with a misbehaving future.

There might be good use-cases for synchronously receiving a result, but then the code should not be using the Future class to carry the value. That's not what Future is for, and not what Future using code expects. Subverting the protocol is a nice hack when it works, but it is not a proper solution, because it doesn't actually work, except by being lucky. (It's pure luck that a synchronous future even works with await, there is absolutely no guarantee that it should.)

I recommend introducing a different class, which is not a Future, for the desired behavior.

abstract class AsyncResult<T> {
  Future<T> asFuture();
  bool get isComplete;
  T get value;
}

and pass that around instead of SynchronousFuture.

Or adding methods to SynchronousFuture like:

abstract class SynchronousFuture<T> ... {
  bool get isComplete;
  T get value; // Throws if completed with error, throws if not complete.
}

and use those methods when you need the result immediately, but have the Future API behave appropriately.

If you are willing to use Future.wait, which definitely introduces an extra delay, then you don't really need a synchronous future. You should just convert the synchronous future to a real future first.
If synchronous future had an asFuture() method, you could just call that and pass that future to Future.wait.

The problem with all of these suggestions is that SynchronousFuture relies on being a future in order to pass through APIs that pass around futures, being oblivious to the value actually being a synchronous future That's just not sound or safe behavior.

@rrousselGit
Copy link
Author

rrousselGit commented Oct 14, 2022

Thanks for the quick reply!

In my opinion, neither of your proposal solve the problem at hand

Both proposals significantly complexify interacting with the Future. They are hardly any different than FutureOr in the end.
In a way, your proposals & FutureOr are basically equivalent from removing async/await and reverting to using .then. That's signiciantly hindering the usability of Futures.

If you are willing to use Future.wait, which definitely introduces an extra delay, then you don't really need a synchronous future.

That's not true.

The core of the issue is that the SynchronousFuture is abstracted (so received as Future).
Most of the time, folks interact with that SynchronousFuture by doing await future, in which case SynchronousFuture works as expected.

The Future.wait issue is that in rare cases, folks may use it with Future.wait (which is not something I have control over).

Not using SynchronousFuture would make the code worse 90% of the time (which do await future), for the the 10% use-case that use it with Future.wait.

This is a pretty bad trade-off IMO.

@rrousselGit
Copy link
Author

In the end, I would describe the current situation as a lose-lose situation.

As a package author, my package has abstracted the problem of a function that sometimes execute synchronously and sometimes asynchronously. But the language doesn't allow me to offer a simple way of interacting with the result.

  • I cannot implement custom async/await keywords as a package
  • I cannot reasonably ask my users to use .then or equivalent
  • If I expose a SynchronousFuture, that works for most people. But it'll silently fail in some edge-cases (cf here)
  • If I expose something other than Future (futureOr or what you suggested), I end-up massively complexifying simple use-cases for edge-cases.
  • if I force the code to always execute asynchronously, the resulting UI output is worse.

So although the problem is conceptually solved by my package, the language/SDK prevents me from solving it for my users.
And all of the proposals made to allow me to solve the problem get rejected.

@lrhn
Copy link
Member

lrhn commented Oct 14, 2022

OK, counterpoint to my previous post, because why have only one opinion?

Most users don't write code that calls then/etc on futures directly. They just use async/await and a small handful of utility functions (with Future.wait being a notable example).

That means that as long as await works with SynchronousFuture (which it looks like it does, but we haven't actually ever properly tested that, since we don't have SynchronousFuture in the SDK), and that handful of functions are made to work, then the vast majority of people will be able to blissfully pretend that SynchronousFuture is not an inherently bad idea.
(It's dangerous, but so is a sync Completer, for the same reasons. Use with care. The completer is safer, because you don't usually pass your completer to third-party code.)

The big question is then what the list of essential functions is, how easy it is to fix them, and what the runtime cost/overhead will be?

I'd assume everything in dart:async can be made to work, and maybe everything in package:async. It's not as much as it sounds like, since most of package:async is about streams. Futures are simple compared to streams, so there isn't that much need for helper functions. Most Future helpers just wrap and forward, which should not need any fixing.

It does suggest that we are accepting the behavior of SynchronousFuture, and that everybody who writes manual asynchronous code in the future should be prepared for futures to do a callback during the call to then/etc. That's not what we actually want to say, so we are muddying the waters by implicitly supporting undesired behavior.

(I can see that SynchronousFuture can only carry values, not errors. That's mildly comforting, synchronous errors would be incredibly error prone.)

To get back on my original high horse: What if await had not worked with a future which called the callback during the then call? If it had thrown a StateError in that case instead (which would have been completely reasonable based on the expected behavior), what would you have done then?
(Then we wouldn't be where we are today, which is a reason I really try to make code throw early on unsupported behavior, so we don't get locked in to supporting something we didn't actually want. However, we are where we are today, and I probably won't get away with making await throw tomorrow.)

@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async type-enhancement A request for a change that isn't a bug labels Oct 14, 2022
@rrousselGit
Copy link
Author

To get back on my original high horse: What if await had not worked with a future which called the callback during the then call? If it had thrown a StateError in that case instead (which would have been completely reasonable based on the expected behavior), what would you have done then? (Then we wouldn't be where we are today

We'd definitely still be here today 😄

Over the past years, I've raised various issues related to this problem

For example, I've asked for making await futureOr synchronous when receiving an instance of T. Or I've asked for statement macro support with metaprogramming so that I can write a custom:

FutureOr<T> futureOr;
T value = @wait(futureOf);

The problem is not specific to SynchronousFuture. Rather it's about an unsolved problem in the Dart language/SDK, where the current most reasonable workaround is SynchronousFuture.

There are multiple workarounds to the issue. Although all the available existing solutions have a major flaw at the moment. For instance, even if this issue wasn't a problem, SynchronousFuture would still not be quite ideal

We're currently stuck in a trinity of problem: Either the workaround is too complex, or it's silently failing, or it has very poor readability/writabily.

For example one solution could be to tell users of my packages to stop using async/await and SDK APIs.
I could build a custom Future type, and manipulate it only with .then:

Then I could ask my users to write code such as:

CustomFuture<int> add(int increment) {
  return CustomFuture(() {
    CustomFuture<int> a = getFuture();
    return a.then((value) => value + increment);
  });
}

That would fix the flicker issue. But that's going back many years in the past just for one problem.

So the issue isn't SynchronousFuture here. The issue is that we don't have an official solution to the problem at hand and can only try to hack one with what we can.

@lrhn
Copy link
Member

lrhn commented Oct 14, 2022

While I fear the API risks, I think using FutureOr for something which is inherently either synchronous or asynchronous, would be the best choice, and making await futureOr synchronous on a non-future would be reasonable.

I don't want everybody to return FutureOr<X> instead of Future<X> just because it might be faster. That's a horrible API.
But for specific cases where you intend the value to sometimes be synchronous, it might make sense.
(Who am I kidding, people will use FutureOr everywhere, with comments saying // FutureOr for speed!.)

It's just a little breaking to make await null; not delay, because that's what you do today to insert an async delay in tests.

(So, make it type based so that await e where e has static type T, and e evaluates to v with runtime type R, behaves as:

  • If T is FutureOr<X>, await if the value is a Future<X> and don't if it's not.
  • otherwise if T implements Future<X>, definitely wait for it.
  • otherwise if T is dynamic then, if R implements Future, wait for it and evaluate to the result. If not, wait a bit and then evalute to v.
  • otherwise wait a little and evaluate to v.

That way, you only get synchronous behavior if the static type is FutureOr, it's opt-in. Will probably still break a lot of existing code that depends on precise timing. Or at least the tests depend on precise timing. Bad tests!)

@rrousselGit
Copy link
Author

Ah, well, that for sure would be great!

About the "FutureOr for speed!", I think this could be attenuated with lint rules.

If there's a "await Future" or another future invocation that's always async, a lint could say "Don't use FutureOr as return value".

The same thing could be applied to "await null" probably.
A static analysis tool could refactor it to "await Future.value()"

@lrhn
Copy link
Member

lrhn commented Oct 15, 2022

If we do allow await futureOr to be synchronous, that does raise the question of whether an async method can choose to return FutureOr as well, and potentially complete synchronously with a value? I'm not willing to go that far. I'd at least want an asyncOr keyword for such methods.

The problem with FutureOr is that it makes typing much harder. We already have a problem at return in async functions, one I am trying very hard to get rid of.
Allowing FutureOr in more places is not going to make things easier.

@rrousselGit
Copy link
Author

I don't mind an asyncOr. That's one of the thing I suggested before:

dart-lang/language#2033

@blaugold
Copy link
Contributor

blaugold commented Nov 6, 2022

If I understand it correctly, this issue as well as dart-lang/language#2033 and usage of SynchronousFuture are primarily motivated by wanting to consume the outcome of a Future that is already completed, in a single frame.

That is a use case specific to Flutter and is currently not possible because the UI build and layout phases (which can interleave) are synchronous and single pass. Instead of changing or extending Dart's async language features, the Flutter framework could be changed to support multiple build phases, where microtasks are executed in between build phases until no new microtasks are scheduled.

For reference, browsers drain the microtask queue after dispatching an event before painting.

I've filed a proposal (flutter/flutter#113763) for this in the Flutter framework repo.

@Ortes
Copy link

Ortes commented May 3, 2024

Any update on this ?
Waiting for one frame while the result is already there is pretty frustrating :/

@lrhn
Copy link
Member

lrhn commented May 3, 2024

Still no plan to change the implementation to support futures that do not follow the Future contract.
Even if we change a number of functions, there will always be yet another function somewhere which assumes that futures behave as they are documented to (have to!) behave. We have no plan to change that behavior either.

Consider every operation that actually works with a SynchronousFuture as a happy accident, and don't assume that it will keep working.

If you don't use SynchronousFuture with any platform functionality, including await, and just treat it as as an unrelated type that happens to have the same methods as Future, then you are probably safe. Otherwise it's deep into "unspecified behavior" territory.

The discussion here has mainly been about providing possible alternatives to SynchronousFuture. Also no current plans for those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-async type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants