Future provider memory leak

[Colman]

Describe the bug
Listeners are added when they shouldn't be when using future providers. This leads to a memory leak since the listeners are never removed.

Dependencies

flutter_riverpod: 2.5.1
riverpod_annotation: 2.3.5
riverpod_generator: 2.4.0
riverpod_lint: 2.3.10

Code

@riverpod
Future<int> a(ARef ref) async {
  print('buildA');
  ref.onAddListener(() {
    print('addA');
  });
  ref.onRemoveListener(() {
    print('removeA');
  });
  ref.onCancel(() {
    print('cancelA');
  });
  await Future.delayed(const Duration(seconds: 5));
  await ref.watch(bProvider.future);
  return 5;
}

@riverpod
Future<int> b(BRef ref) async {
  print('buildB');
  ref.onAddListener(() {
    print('addB');
  });
  ref.onRemoveListener(() {
    print('removeB');
  });
  ref.onCancel(() {
    print('cancelB');
  });
  return 6;
}

class SomeScreen extends ConsumerWidget {
  @override
  Widget build(BuildContext context, WidgetRef ref) {
    ref.watch(aProvider);
    return Container();
  }
}

If you display SomeScreen, then click back within 5 seconds (the delay in aProvider), the following is printed:

buildA
addA
removeA
cancelA
buildB
addB

A listener for bProvider is added but never removed even though nothing is listening to aProvider anymore. Shouldn't aProvider check its own aliveness before adding a listener to bProvider on the line ref.watch(bProvider.future)?

Expected behavior
The listener on bProvider should never be added

[rrousselGit]

All of that sounds normal to me.

A was mounted, and A is listening to B. So B is indeed listened.
Even though A is unused, it still needs to listen to B. After-all if B is recomputed, A will have to be recomputed the next time it is used.

[Colman]

I guess I just expected both A and B to be disposed when I leave the page and wait. I expected the state of B to be reset when I go back to the page the second time, but it remains. Why should a disposed resource A keep its dependencies B alive forever?

I find it weird that an unused auto-dispose provider leaves a listener attached to its dependency forever. This fundamentally causes a memory leak because if you:

1. Navigate to SomeScreen
2. Go back
3. Repeat steps 1-2 many times

In the end, you'll only have one SomeScreen mounted, so A will end up with one listener. B will have 1 useful listener and many other "ghost" listeners that are never cleared. So if those steps are repeated, you end up with 1 million listeners on B even though only one of them is for a resource that isn't disposed? There's no way that this can be expected behavior.

Why doesn't the watch method in A check if A is used/not disposed before adding the listener to B?

[rrousselGit]

Oh, I think I got confused about the issue. I'll investigate. Sounds like a bug for sure.

Thanks for the report.

[Colman]

My project relies on this task being fixed urgently. If I were to make a PR and it's merged successfully, could it be a part of a minor release, or would it have to wait until 3.0?

[rrousselGit]

It depends on the changes involved. But if it has to be a 3.0.0 fix, you could always stick to using your fork in the meantime

[Colman]

I'm not exactly sure how to fix this. If provider a in my example is disposed since we left the screen, should it be allowed to watch anything? Or should a wait until it has no listeners and its build is completed before disposing?

[mattermoran]

@rrousselGit I managed to fix it by simply checking the _mounted status in ref functions and throwing error if not but I don't know if it could cause any other issues?
here's the pr #3598

[AhmedLSayed9]

I've got the same bug (which was hard to catch).

A temporary solution is to update aProvider to watch the future before the await:

@riverpod
Future<int> a(ARef ref) async {
  print('buildA');
  ref.onAddListener(() {
    print('addA');
  });
  ref.onRemoveListener(() {
    print('removeA');
  });
  ref.onCancel(() {
    print('cancelA');
  });
  final bFuture = ref.watch(bProvider.future);
  await Future.delayed(const Duration(seconds: 5));
  await bFuture;
  return 5;
}

[mattermoran]

@AhmedLSayed9 that kinda works but you would end up making that network call even if provider is disposed and there's no need. also it gets quite messy the more futures you have in your provider.
but if you ok with waiting all those futures even when disposed you might want to consider keepAlive link instead in this case you only have 2 extra lines no matter how many futures you have

@riverpod
Future<int> a(ARef ref) async {
  print('buildA');
  ref.onAddListener(() {
    print('addA');
  });
  ref.onRemoveListener(() {
    print('removeA');
  });
  ref.onCancel(() {
    print('cancelA');
  });
  
  final keepAliveLink = ref.keepAlive(); // make it non disposable
  
  await Future.delayed(const Duration(seconds: 5));
  await ref.watch(bProvider.future);
  
  keepAliveLink.close(); // release
  
  return 5;
}

[AhmedLSayed9]

you would end up making that network call even if provider is disposed

This would occur in both cases. I'd use CancelToken for that matter anyway.

you might want to consider keepAlive link instead

The KeepAliveLink will prevent aProvider from being disposed if bProvider throws an error.

[mattermoran]

This would occur in both cases. I'd use CancelToken for that matter anyway.

Yup that's why I said if you ok with that. Some requests unfortunately can't be canceled e.g supabase db calls, etc

The KeepAliveLink will prevent aProvider from being disposed if bProvider throws an error.

Fair enough. You'd want to have a wrapper function that would handle with try/catch/finally which gets quite ugly regardless.

The proper solution is to just throw when trying to use watch/listen of disposed refs which is coming in v3. For now we may just have to stick to a fork