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

On rebuild, don't clear dependencies until the new future of a futureProvider completes #1253

Open
tomgilder opened this issue Mar 3, 2022 · 13 comments
Assignees
Labels
breaking enhancement New feature or request
Milestone

Comments

@tomgilder
Copy link

Hello, I was wondering if there could there be a way of FutureProvider keeping watched dependent providers alive until the next value is generated?

I found the behaviour of this simple app very surprising:

import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';

void main() {
  runApp(ProviderScope(child: MaterialApp(home: HomePage())));
}

// I expected this provider to always stay alive...
final counter = StateProvider.autoDispose((ref) => 0);

// ...because this provider is always listened to
final futureProvider = FutureProvider.autoDispose((ref) async {
  ref.maintainState = true;

  await Future.delayed(const Duration(milliseconds: 100));
  return ref.watch(counter);
});

class HomePage extends ConsumerWidget {
  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final value = ref.watch(futureProvider);

    return Scaffold(
      body: Center(
        child: ElevatedButton(
          onPressed: () => ref.read(counter.notifier).state++,
          child: value.when(
            data: (value) => Text(value.toString()),
            loading: () => const CircularProgressIndicator(),
            error: (_, __) => const Text('Error'),
          ),
        ),
      ),
    );
  }
}

I’m aware that moving ref.watch(counter) above the await will fix this issue, as would directly listening to counter from the widget tree, but neither are ideal in my situation.

This case seems seems especially counterintuitive as it was counter that trigger the rebuild.

Would it be possible to have a behaviour that defers unsubscribing from providers until after the next value has been generated? So it would go...

  1. futureProvider rebuild is triggered
  2. While it’s waiting for completion, futureProvider retains whatever auto-dispose providers the previous build listened to
  3. Once futureProvider has completed, then any auto-dispose providers no longer listened to are disposed

I feel for me this would be a more obvious default (but I’m fairly sure it’ll cause other problems I haven’t thought of 😀). This seems to have come up in various issues before, but as far as I can see there isn’t an easy solution right now.

Thanks for the great work, as always!

@tomgilder tomgilder added enhancement New feature or request needs triage labels Mar 3, 2022
@rrousselGit
Copy link
Owner

The issue is that some futures can take a very long time before they complete.
It doesn't sound right to keep the provider alive in those cases

With version 2.0, there are a few features related to autoDispose that could help. Namely cacheTime and disposeDelay

I'm considering setting disposeDelay to 5 seconds by default. So unless yourFutureProvider takes more than 5 seconds, its dependencies wouldn't be disposed.

@tomgilder
Copy link
Author

The issue is that some futures can take a very long time before they complete. It doesn't sound right to keep the provider alive in those cases

Personally I feel that over-retaining rather than under-retaining is better, and it matches my mental model of what I expect.

I think my mental model is similar to a garbage collector, where any reference to another provider retains it. And this surprised me because there's no other obvious way of retaining another provider. It feels like maybe we're almost getting in to weak and strong references?

Whilst there definitely are futures that take a long time to complete, I don't feel they're that common in app state management, and I can't think of a time where I've had one.

With version 2.0, there are a few features related to autoDispose that could help. Namely cacheTime and disposeDelay

These looks really useful, thanks

I'm considering setting disposeDelay to 5 seconds by default. So unless yourFutureProvider takes more than 5 seconds, its dependencies wouldn't be disposed.

I feel that could a bad idea and lead to even more surprising and confusing behaviour.

For example I could see a situation where a HTTP endpoint normally returns quickly, but one day the server is slow and it takes 7 seconds... and you end up with a provider that never loads, because some state keeps being disposed. I image that'd be pretty hard to debug.

@rrousselGit
Copy link
Owner

A bigger problem is that I don't want "watch" to behave differently based on the provider

And it's not exactly clear how this could be generalized to others.

@rrousselGit
Copy link
Owner

For example I could see a situation where a HTTP endpoint normally returns quickly, but one day the server is slow and it takes 7 seconds... and you end up with a provider that never loads, because some state keeps being disposed. I image that'd be pretty hard to debug.

Why would it never load?

If a provider is disposed + recreated because the request was too slow, that shouldn't trigger side effects

@rrousselGit rrousselGit changed the title Request: retain auto-dispose providers with FutureProvider On rebuild, don't clear dependencies until the new future of a futureProvider completes Apr 16, 2022
@tomgilder
Copy link
Author

@rrousselGit I can't remember exactly what I was thinking of, but could you end up in a hard-to-understand situation like this?

// If this disposes by default after 5 seconds...
final localStateProvider = StateProvider.autoDispose(...);

final homePageDataProvider = FutureProvider.autoDispose<String>(
  (ref) async {
    // ...then if this endpoint is refreshed, and takes longer than 5 seconds,
    // the local state is disposed? 
    //
    // But less than 5 seconds and app state is re-watched so it doesn't get disposed?
    final endpointData = await ref.watch(endpointProvider.future);
  
    final localState = ref.watch(localStateProvider);

    ...
  },
);

So your app works fine... until one day the server is slow and local state starts resetting?

@rrousselGit
Copy link
Owner

Yes, that could happen.

I'm thinking about doing: "When rebuilding a provider, keep dependencies until value other than AsyncLoading is emitted".
That would not be specific to FutureProvider

@tomgilder
Copy link
Author

@rrousselGit just to give you some real-world context, this is what we're doing in a production app...

final endpointProvider = StateNotifierProvider.autoDispose<
    EndpointNotifier<String>, AsyncValue<String>>((ref) {
  // Manages fetching data from a HTTP endpoint
});

final futureProvider = FutureProvider.autoDispose((ref) async {
  final shouldDoRequest = await ref.watch(someConditionProvider.future);
  if (shouldDoRequest) {
    return await ref.watch(endpointProvider.future);
  }

  return null;
});

We have separate providers for every endpoint (and it's working fantastically, hoping to do many blog posts about this!).

Ideally, if futureProvider rebuilds, as long as shouldDoRequest is true, endpointProvider should always stay alive.

Some of the endpoint data is quite large, so we want to dispose it the second nothing is listening to it anymore.

Would "keep dependencies until value other than AsyncLoading is emitted" achieve that, even if futureProvider gets rebuild after emitting an AsyncData?

@rrousselGit
Copy link
Owner

Yes.

@saibotma
Copy link

It would be great to have this issue resolved from my point of view.
I'd love to try to create a PR implementing it similar to how @rrousselGit proposed it:

I'm thinking about doing: "When rebuilding a provider, keep dependencies until value other than AsyncLoading is emitted". That would not be specific to FutureProvider

Is there anything else that needs to be taken into account?

@rrousselGit
Copy link
Owner

I love that you want to contribute. But I won't lie: It's at the very top of my todo-list. I was about to do it this week, if not today.

It's probably not worth that you look into it.

@saibotma
Copy link

That actually sounds even better! Happy to then test it when you're done.

@rrousselGit
Copy link
Owner

I guess I ended up taking a break 🤷
I'll come back to this soon.

@Stumblinbear
Copy link

Just ran into this after one of my providers started firing off hundreds of requests because I was await-ing a future earlier on in it. Resolved by switching it to use .asData?.value, but this is quite the footgun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants