Dispose in reverse order

[ds84182]

Currently, flutter_hooks disposes hooks from the top down, which is strange considering that Flutter itself disposes states from the bottom up. This causes exceptions when removing ChangeNotifier listeners that get cleaned up by useEffect's disposal callback.

[rrousselGit]

Ah, indeed. I would swear I reversed it. I remember fixing the failing tests and everything...

Thanks, will do.

[derolf]

We’ve just encountered the same problem. Can you please merge the fix?

[rrousselGit]

It's not that simple. While such change fixes a few things, it also breaks others.

Similarly, React doesn't dispose of them in reverse order. And clearly, they use hooks on a much larger scale

[derolf]

Could you merge it and just give the library a flag to define forward/reverse order.

[rrousselGit]

@derolf You mentioned that this caused a lot of issues. Could you clarify what these are?

For now, I'm still not convinced if we should do something about it or not.

[derolf]

Generally, I think reverse order is natural since that is what you usually do in dispose/destructors: you dispose stuff in the reverse order of creation. That is especially important if there are dependencies among the things you create.

Concrete: I have a bunch of hooks to create scrollcontrollers, texteditingcontrollers etc. and then in a large fraction I am using useListenable to connect to them. During destruction, the controllers are killed first and then ListenableHook tries to remove itself which crashes the app.

[rrousselGit]

The issue is, reverse order cause problems too because hooks have a "keys" that allow them to be disposed of on command.

So for example, we can have:

final controller = useMemoized(() => MyController(someKey), [someKey]);
useMyController(controller);

In that situation, we're stuck. Because changing someKey would dispose of the previous controller, but useMyController would still remove its listener after the disposal.

So having hooks disposed only from top to bottom allows some form of consistency.

[rrousselGit]

Similarly, React doesn't dispose hooks in reverse order of creation, and their hooks are used on a much larger scale.

A potential solution may be to offer 3 kind of hooks instead of two (or at least 2 + a flag):

• a hook to create/dispose an object
• another one to listen to the object
• a third one that does both

Although this may not solve more advanced use-cases with animations and tweens and stuff.

[derolf]

In that case either order will be wrong since when someKey changes and the old controller gets killed, useMyController will crash once it attempts to unsubscribe from the old (now dead) controller.

Probably we need a solution that allows to "scope" hooks inside other hooks.

[derolf]

a third one that does both

That's also what I am thinking of. It would be nice if all Listenables in Flutter would implement a Disposable interface...

[rrousselGit]

The issue being, React doesn't have this problem, because they never use Listenable equivalents.

I tried to speak a bit with Dan Abramov on that topic some months ago, but got no concrete solution out of it.

[cgestes]

What do you propose to fix the following:

    final queryTextController = useTextEditingController();

    useEffect(() {
      final cbOnChanged = () { // do something usefull };
      queryTextController.addListener(cbOnChanged);
      return () => queryTextController.removeListener(cbOnChanged);
    });

This throws:

The following assertion was thrown while disposing _EffectHookState:
A TextEditingController was used after being disposed.

Should I just drop the removeListener? :D

[derolf]

I created our own variant of _ListenableHook:

  @override
  void dispose() {
    try {
      hook.listenable.removeListener(_listener);
    } catch (_) {
      // see https://github.com/rrousselGit/flutter_hooks/issues/81
    }
  }

[rrousselGit]

Interesting fix!

I'm not too good with GC, would it potentially create memory leaks (And work with streams)?

[smiLLe]

@derolf i am running into the same issue. Are you still using the custom hook, or do you guys have something new?

[rrousselGit]

As a comeback to that:

The issue is, reverse order cause problems too because hooks have a "keys" that allow them to be disposed of on command.

So for example, we can have:

final controller = useMemoized(() => MyController(someKey), [someKey]);
useMyController(controller);

In that situation, we're stuck. Because changing someKey would dispose of the previous controller, but useMyController would still remove its listener after the disposal.

So having hooks disposed only from top to bottom allows some form of consistency.

Maybe one possibility is to delay the disposal of hooks in that situation to the end of the frame.

By doing so, we would have the list of all the hooks that need to be disposed of. So we could do it in reverse order.

[rrousselGit]

After all this time, I've finally decided to reverse the dispose order.
Sorry for the delay.

[derolf]

Sounds good, but why finally?

[rrousselGit]

No particular reason. I was always doing something else instead.

…

On Sat, Jun 20, 2020, 14:25 derolf ***@***.***> wrote: Sounds good, but why finally? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#81 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEZ3I3OWCFEZ33HIXXHUJETRXS2FVANCNFSM4HK4VQAQ> .

[davidmartos96]

It's not that simple. While such change fixes a few things, it also breaks others.

Similarly, React doesn't dispose of them in reverse order. And clearly, they use hooks on a much larger scale

@rrousselGit I recall there were some limitations with an original draft you created. Is that solved?
Thanks for the update! 😄

[rrousselGit]

Yes. I overestimated the consequences at that time.

The only situation where this could be surprising is, it may create some exceptions after a hot-reload that reorder hooks.
Considering hot-reload is dev only, I've estimated that it's not worth thinking about.

[andrea689]

Hi, today I updated from 0.9.0 to 0.14.1 and I received A ScrollController was used after being disposed after any hot reload.

Is there a way to fix this?

I use this code:

final scrollController = useMemoized(() => ScrollController());

useEffect(() {
  scrollController.addListener(onScroll);
  return () => scrollController.removeListener(onScroll);
}, [key]);

useEffect(() {
  return () => scrollController.dispose();
}, []);

[rrousselGit]

Move your first useEffect after the second

[smiLLe]

@andrea689 you are calling the .removeListener after your .dispose
try this

final scrollController = useMemoized(() => ScrollController());

useEffect(() {
  return () => scrollController.dispose();
}, []);

useEffect(() {
  scrollController.addListener(onScroll);
  return () => scrollController.removeListener(onScroll);
}, [key]);

[andrea689]

Thank you to all, it is working! Sorry but I'm new on hooks.

[AAverin]

I am trying to rebuild my tabController depending on some feature flags being on or off.

final featureFlags = useState(database.getFeatureFlags());
database.featureFlagsBox.watch(key: "feature_flags").listen((event) {
      featureFlags.value = event.value;
    });
final allPages = useMemoized(() {
      return _buildPages(featureFlags.value);
    }, [featureFlags.value]);
final _ticker = useSingleTickerProvider();
final _tabController = useTabController(initialLength: allPages.length, vsync: _ticker, keys: [featureFlags.value]);

Doing that results in and error:

The following assertion was thrown building CollectionsPage(dirty, dependencies: [_EffectiveTickerMode, _LocalizationsScope-[GlobalKey#13ef1], _InheritedTheme], useState<FeatureFlags>: Instance of 'FeatureFlags', useMemoized<List<PageViewDescription>>: [Instance of 'PageViewDescription'], useSingleTickerProvider):
A TabController was used after being disposed.

Once you have called dispose() on a TabController, it can no longer be used.
The relevant error-causing widget was: 
  CollectionsPage .../lib/components/screens/home/HomeScreen.dart:44:67
When the exception was thrown, this was the stack: 
#0      ChangeNotifier._debugAssertNotDisposed.<anonymous closure> (package:flutter/src/foundation/change_notifier.dart:117:9)
#1      ChangeNotifier._debugAssertNotDisposed (package:flutter/src/foundation/change_notifier.dart:123:6)
#2      ChangeNotifier.dispose (package:flutter/src/foundation/change_notifier.dart:212:12)
#3      TabController.dispose (package:flutter/src/material/tab_controller.dart:299:11)
#4      _TabControllerHookState.dispose (package:flutter_hooks/src/tab_controller.dart:57:33)

[brunodmn]

Hello, I am having a similar problem with useState to show a progress dialog hud.

#this is my code init

@override
  Widget build(BuildContext context) {
    final showDialog = useState(false);

#this is my state change

showDialog.value = true;
await CoreMethods.downloadFile();           
 showDialog.value = false;

it works fine, unless I leave the screen while loading, in this case the state is disposed but code keeps running and try to set it anyway.

Would be nice if I could check whether showDialog is disposed before update the value (I wouldn't need to change value if its disposed already).

Is there some workaround or fix?

Thanks

[davidmartos96]

@brunodmn You can trye the useIsMounted hook. Check whether the state is mounted after the downloadFile function finishes.
https://pub.dev/documentation/flutter_hooks/latest/flutter_hooks/useIsMounted.html

[brunodmn]

@brunodmn You can trye the useIsMounted hook. Check whether the state is mounted after the downloadFile function finishes.
https://pub.dev/documentation/flutter_hooks/latest/flutter_hooks/useIsMounted.html

It worked this way, thank you! I'd tried with useIsMounted before, but not on build body.

bellow my updated snippets, for reference:

init...

 @override
  Widget build(BuildContext context) {
    final isMounted = useIsMounted();
    final _showDialog = useState(false);

state change

showDialog.value = true;
            await CoreMethods.downloadFile();
            if (isMounted()) {
              _showDialog.value = false;
            }
          }),