Skip to content

How can we make dart:async better for UIs? #33713

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

Closed
matanlurey opened this issue Jun 29, 2018 · 8 comments
Closed

How can we make dart:async better for UIs? #33713

matanlurey opened this issue Jun 29, 2018 · 8 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue customer-flutter customer-google3 P3 A lower priority bug or feature request

Comments

@matanlurey
Copy link
Contributor

Given Dart's new focus on being the best client/UI language, I thought I'd revisit this.

Internally there is a long-ish thread with customers having issues, primarily around Future and async/await, and UI components that are shorter-lived than the pending operations encapsulated by the Future.

An internal user writes:

With async/await it is very easy to forget that this is an asynchronous code, and the state of the object can change in the background. I think we cannot have a "synchronized" keyword like in Java, as this code is expected to be "interrupted", but I was wondering if we can have some other language support for that. Any ideas? Or maybe there are some best practices on how to avoid such issues?

/cc @lrhn who already jumped into this conversation, and pointed out issues with it :)

This seems to affect both AngularDart and Flutter, though in different ways.

AngularDart

A basic pattern is something like below:

class Comp implements OnDestroy, OnInit {
  final State state;

  Comp(this.state);

  @override
  void ngOnInit() async {
    var result = await someRpc();
    // You might hit this line after ngOnDestroy has already been called
    // and the component is in a dead/inactive/cleaned-up state.
   state.update(result);
  }

  @override
  void ngOnDestroy() {
    // This is the only lifecycle event you have available to do clean-ups.
   state.dispose();
  }
}

See the bug? state.update(result) might happen after state.dispose().

Users need to either:

  • Ensure all stateful objects implement some sort of state machine to track validitity

  • Stop using async/await

  • Use something like CanceleableOperation from pacakge:async:

    var result1 = await new CancelableOperation.fromFuture(someThing(), onComponentDestroyed).valueOrCancellation;
    var result2 = await new CancelableOperation.fromFuture(someThing(), onComponentDestroyed).valueOrCancellation;
    var result3 = await new CancelableOperation.fromFuture(someThing(), onComponentDestroyed).valueOrCancellation;
    

ReactJS also has/had this problem, and suggested wrapping all ES6 promises with makeCancelable - fairly close to the CancelableOpreation code above (though less wordy):

const cancelablePromise = makeCancelable(
  new Promise(r => component.setState({...}))
);

cancelablePromise
  .promise
  .then(() => console.log('resolved'))
  .catch((reason) => console.log('isCanceled', reason.isCanceled));

cancelablePromise.cancel(); // Cancel the promise

const makeCancelable = (promise) => {
  let hasCanceled_ = false;

  const wrappedPromise = new Promise((resolve, reject) => {
    promise.then(
      val => hasCanceled_ ? reject({isCanceled: true}) : resolve(val),
      error => hasCanceled_ ? reject({isCanceled: true}) : reject(error)
    );
  });

  return {
    promise: wrappedPromise,
    cancel() {
      hasCanceled_ = true;
    },
  };
};

Flutter

Flutter tries to encapsulate many of the issues with FutureBuilder. From what I've hear from both framework authors and customers, there is several holes with this approach, as well. I'll let them fill in the details.

@matanlurey matanlurey added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). customer-flutter customer-google3 labels Jun 29, 2018
@matanlurey
Copy link
Contributor Author

From a strictly AngularDart perspective, we could introduce a class or mixin that tries to encapsulate the hard parts of doing async work in a component:

// Better name pending.
abstract class AsyncComponent implements OnDestroy {
  /// Wraps a [future] to ensure it never completes if the component has been destroyed.
  Future<T> whenActive<T>(Future<T> future);
}

Example use:

class Comp extends AsyncComponent implements OnInit {
  @override
  void ngOnInit() async {
    final result = await whenActive(someRpcService());
    // Will never make it here if the component has been destroyed.
  }
}

I imagine we'd have to write some sort of lint to make this very effective, but it is an idea.

@alanrussian
Copy link

alanrussian commented Jun 29, 2018

For Flutter: I, and I believe the rest of my team, have been happy with the FutureBuilder and StreamBuilder approach but there's a major problem issue with it that prevents us from using it in many places: #33024.

Flutter has even built a custom future-like class to handle that issue with Futures. https://master-docs-flutter-io.firebaseapp.com/flutter/foundation/SynchronousFuture-class.html

@matanlurey
Copy link
Contributor Author

I think the underlying issue is very different, but related - Future isn't meant to have synchronous access to data, but at least some users/framework authors have found that inconvenient or difficult to work around.

One more thing I probably should mention is there still an issue of wasting resources, even if we use the wrapper approach or FutureBuilder, there is no way to cancel the underlying RPC.

Imagine downloading a 50Mb video from the server, and the user navigates away, on mobile...

@jonahwilliams
Copy link
Contributor

State.isMounted

Flutter has to work around some of the same issues regarding async work that Angular dart does. For starters, we have State.isMounted which works exactly as it did in React (before it was deprecated). We do specifically disallow async in lifecycle methods as well, but this is more for general correctness and timing issues with setState. Because Widget trees are immutable, there is less that can go wrong when async work continues past disposal, except for calling setState which we catch with an development error. Of course, the user will still end up doing extra work, and we'll still have to hold more widgets in memory then we would like

class MyWidgetState extends State<MyWidget> {
  String _name;
  
  Future<void> doSomeWork() async {
    final String name = await Api.getName();
    // might have been dismounted, check before calling `setState`
    if (isMounted) {
      setState(() { _name = name });
    }
  }

  @override
  void dispose() {
      // only chance to clean up resources.
     super.dispose();
   }

  Widget build(BuildContext context) => ...
}

FutureBuilder

There are some tangentially related issues with a widget called FutureBuilder. This takes a Future and a builder closure and rebuilds whenever the Future changes. However, recently I've been seeing several issues where it is used incorrectly, but in a non-obvious way. In the example below, instead of passing a Future that is cached on a State object, I just directly pass the invocation of an async method to my FutureBuilder. This works fine until maybe I add an animation on the ancestor of this widget, and now every second when build is called, a new Future is created and the FutureBuilder restarts. Inside the FutureBuilder, it's not possible to tell the difference between any two futures so we have to .

Future<String> _getName() async { ... }

Widget build(BuildContext context) {
  new FutureBuilder<String>(
    future: _calculation(), // a Future<String> or null
    builder: (BuildContext context, AsyncSnapshot<String> snapshot) {
      switch (snapshot.connectionState) {
        case ConnectionState.none: return new Text('Press button to start');
        case ConnectionState.waiting: return new Text('Awaiting result...');
        default:
          if (snapshot.hasError)
            return new Text('Error: ${snapshot.error}');
          else
            return new Text('Result: ${snapshot.data}');
      }  
  },
  )
}

Is Future a value or a Computation?

A future has value-like semantics, in that it is immutable and un-cancellable. But it is also an eager computation, and naturally full of side effects. And the combination of these two features means that we can't, as framework authors, solve either problem above. Introducing our own classes like SynchronousFuture is only a stop-gap measure for ourselves, since our users will naturally be drawn towards the simplicity of await.

@matanlurey
Copy link
Contributor Author

Good point, one of the issues is that the "best" syntax (await) has a specific contract already.

@jdemilledt
Copy link

As someone who is making an application that may have impatient users, having a way to handle this would be great. How would one work around this until a fix comes out?

@lrhn
Copy link
Member

lrhn commented Jul 2, 2018

It's true that Future sets itself between two chairs. It's immutable and sharable (you can listen to it more than once, and passing it around is fine), but it also does represent the result of a computation, including potential throws, and we have issues with passing error futures around (suddenly everybody needs to catch the error).
If we designed Future today, I'd probably have made it one-listen-only. Alas, we didn't, and changing that now is potentially very breaking.

We have CancelableOperation instead of CancelableFuture pretty much on my insistance. The latter would just add a method to Future that nobody else is prepared for. Most futures aren't cancelable, all future operations would throw away the cancelability, so it was really just a hack to pass a "future + cancel function" pair through a type-check that expecter a Future, and cast it back at the other end. Which is a great hack when really necessary, but not good library design.

The issue here seems to be a lack of easy state management and sequencing. There is nothing inherently wrong with the original example except that the setState is called without checking that it's still valid. Everything up to that was fine. It was not wrong to call ngDestroy before ngOnInit was done, even if that means the component was live without being fully initialized (I
guess it was not wrong ... if it was, then ngOnInit must just not be async at all). So, what we need is not mutual exclusion, that would just have prevented you from calling ngDestroy.

The whenActive wrapper will work (should probably make it state.whenActive), but only if you remember using it, and then you could also just write if (state.isDisposed) return; after the await.

Is the answer here perhaps for state.update to throw when the state has been disposed?
Throwing has issues, but if it throws a CancelledException and the caller of ngOnInit() captures such from any returned future, then it might be enough.
I think that's the only way to ensure that the issue is addressed. Everything else is adding more code to ensure something, but nothing prevents you from just forgetting that extra code.

There is still some room for language fiddling around this, including await, even if I don't think it will fix this particular problem.

If we ever get automatic resource disposal (like C#'s IDisposable and using), then a cancelable operation should be disposable. We'd probably use cancel as the dispose method name then (too many things already use that). Then CancelableOperation could be disposable. Wouldn't make a difference here, though, since the cancel needs to happen in a separate method.

We could also make other objects than Future "awaitable". Maybe just anything with a Future-like then method, but probably we have to add something new that Future also gets then. Then CancelableOperation could be both awaitable and disposable. Still not sure if that would actually help.

@munificent munificent removed the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Jul 11, 2018
@munificent munificent changed the title Meta: How can we make dart:async better for UIs? How can we make dart:async better for UIs? Jul 11, 2018
@matanlurey matanlurey added the P3 A lower priority bug or feature request label Aug 27, 2018
@lrhn
Copy link
Member

lrhn commented Oct 2, 2020

No plans to fix this in the platform libraries. It's a domain specific issue with state management.

@lrhn lrhn closed this as completed Oct 2, 2020
@lrhn lrhn added the closed-not-planned Closed as we don't intend to take action on the reported issue label Oct 2, 2020
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. closed-not-planned Closed as we don't intend to take action on the reported issue customer-flutter customer-google3 P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

6 participants