adrielcafe/voyager

ViewModel is cleared too early

bettercalltolya opened this issue · 7 comments

bettercalltolya commented

I encountered an issue with navigation in my app. Let's say I have Feature1 and Feature2 screens and both use getViewModel() inside Content function.
I navigate Feature2 -> [pop] back to Feature1 -> [push] open again Feature2 pretty fast.

What's happening in my logs: when I pop Feature2 screen, VM gets cleared in about 30ms, but screen is disposed in 600-700ms, so if then I push Feature2 again within these 600-700ms, I'm stuck on Feature2 screen with cleared VM.

twanschik commented

I‘ve encounter a related issue without having to click fast: when using a transition animation when popping screen 2 the viewmodel gets cleaned up to early. The transition will call it.Content() which will try to get the viewmodel but it was already cleared. This will result in a crash.

I used the latest version rc3

  • 👍1
kihaki commented

We have encountered a similar issue, also without clicking fast. ViewModels will be cleared before the animations are done, which will result in a ViewModel leak, as follows:

  1. Push ScreenA, ScreenA will create a ViewModelA
  2. Pop ScreenA, first all the lifecycle will be cleared with ScreenLifecycleStore.clear(ScreenA), this will clear ViewModelA, then the exit animation will be played.
  3. The exit animation will call ScreenA.Content() which will create a new ViewModelA (since a new ViewModelStore is created, since the previous one was already disposed in step 2).
  4. Once the exit animation is done, the ViewModelStore containing ViewModelA will never be cleared (since StepDisposableEffect will not clear the ScreenLifeCycleStore again for ScreenA), leaking the ViewModel.

This leads to some weird bugs in our application and should probably be fixed quickly. I might be able to create a PR once I have found a solution.

twanschik commented

Our current workaround is to disable calling the content function on animations if the lifecycle is already destroyed:

content = {
            if (LocalSavedStateRegistryOwner.current.lifecycle.currentState != Lifecycle.State.DESTROYED) {
                it.Content()
            }
        },

The downside of that is having no content when navigating from one screen to another. But at least, the app behaves as expected.

kihaki commented

Thanks. Having no contend on animations is not a solution for us unfortunately. I will try to find another workaround, maybe by disposing the screen manually after the animation completes and disabling Voyagers disposal.
This is a dealbreaker for us.

kihaki commented

So after some research I have found a working workaround for the issue that also renders the screen during the animations. There is a couple of issues at play here:

  1. The ViewModelStore provided in the animations is always the one of the Navigator.lastItem, for the content of both screens that are being animated, when they each should be rendered with their own respective ViewModelStore.
    Say you start with a stack of [ScreenA, ScreenB], then you pop ScreenB. During the animation, the ViewModelStore of Navigator.lastItem will be provided to both ScreenA and ScreenB, which means the ViewModelB will be pushed into the ViewModelStore of ScreenA.
    This can be fixed by adding the following in the transition code:
ScreenTransition(
    (...),
    content = { screen ->
        val lifecycleOwner = rememberScreenLifecycleOwner(screen)
        val hooks = lifecycleOwner.getHooks()

        CompositionLocalProvider(
            *hooks.providers.toTypedArray(),
        ) {
            screen.Content()
        }
    }
)

This fixes the first issue.

  1. When popping a Screen its ViewModelStore will be disposed immediately, before the transition animation completes, which is only okay if there are no transitions. If there are transitions, we have to dispose the ViewModelStore after the transition animation completes, so that the exiting Screens ViewModel will not be recreated for the transition animation.

My current workaround for this is to disable the default dispose behavior of Voyager entirely and disposing by hand after the transition is completed. Unfortunately for this I have to copy the private Navigator.dispose(screen: Screen) method that is internal to Voyager. Also, listening to the end of the transition animation is currently a bit akward, I have not found a clean solution for this yet since Jetpack Compose doesn't provide a listener hook. So I am still looking for improvements in that regard.

I will probably post a complete workaround once I have fleshed it out.
In any case, this should probably be fixed in Voyager itself at some point, since the broken default behavior leads to very surprising bugs.

twanschik commented

So after some research I have found a working workaround for the issue that also renders the screen during the animations. There is a couple of issues at play here:

  1. The ViewModelStore provided in the animations is always the one of the Navigator.lastItem, for the content of both screens that are being animated, when they each should be rendered with their own respective ViewModelStore.
    Say you start with a stack of [ScreenA, ScreenB], then you pop ScreenB. During the animation, the ViewModelStore of Navigator.lastItem will be provided to both ScreenA and ScreenB, which means the ViewModelB will be pushed into the ViewModelStore of ScreenA.
    This can be fixed by adding the following in the transition code:
ScreenTransition(
    (...),
    content = { screen ->
        val lifecycleOwner = rememberScreenLifecycleOwner(screen)
        val hooks = lifecycleOwner.getHooks()

        CompositionLocalProvider(
            *hooks.providers.toTypedArray(),
        ) {
            screen.Content()
        }
    }
)

This fixes the first issue.

  1. When popping a Screen its ViewModelStore will be disposed immediately, before the transition animation completes, which is only okay if there are no transitions. If there are transitions, we have to dispose the ViewModelStore after the transition animation completes, so that the exiting Screens ViewModel will not be recreated for the transition animation.

My current workaround for this is to disable the default dispose behavior of Voyager entirely and disposing by hand after the transition is completed. Unfortunately for this I have to copy the private Navigator.dispose(screen: Screen) method that is internal to Voyager. Also, listening to the end of the transition animation is currently a bit akward, I have not found a clean solution for this yet since Jetpack Compose doesn't provide a listener hook. So I am still looking for improvements in that regard.

I will probably post a complete workaround once I have fleshed it out. In any case, this should probably be fixed in Voyager itself at some point, since the broken default behavior leads to very surprising bugs.

Do you already have some code to share for the "waiting for animation to complete" problem? Thx.

kihaki commented

Yes, you can do the following:

  1. Disposing after the screen animations are done:
ScreenTransition(
        navigator = navigator,
        transition = { ... },
        content = { screen ->
            val lifecycleOwner = rememberScreenLifecycleOwner(screen)
            val hooks = lifecycleOwner.getHooks()

            // Fixes a voyager issue: https://github.com/adrielcafe/voyager/issues/106#issuecomment-1441314987
            CompositionLocalProvider(
                *hooks.providers.toTypedArray(),
            ) {
                screen.Content()

                // Dispose ViewModels after the screen transition animation is done
                transition.ListenerEffect(
                    onExitEnd = {
                        if (!navigator.items.contains(screen)) {

                            // Dispose inner Navigators if there are any
                            screen.getChildNavigators().forEach { navigator ->
                                navigator.items.forEach { childScreen ->
                                    navigator.dispose(childScreen)
                                }
                            }

                            // Dispose the screen
                            navigator.dispose(screen)
                        }
                    },
                )
            }
        },
    )

Here you have to call the Navigator.dispose() method via reflection because unfortunately it is currently internal.

  1. Calling Navigator.dispose() via reflection:
private fun Navigator.dispose(
    screen: Screen,
) {
    try {
        callDispose(screen)
    } catch (error: Throwable) {
        ... log this or not ...
    }
}

private fun Navigator.callDispose(screen: Screen): Unit = javaClass.methods
    .first { method ->
        method.name.startsWith("dispose") && // Will also cover methods that have a generated name (with $ sign)
            method.parameterCount == 1 &&
            method.parameterTypes
                .map { clazz: Class<*> -> clazz.isAssignableFrom(screen::class.java) }
                .all { it }
    }
    .let { method ->
        method.isAccessible = true
        method.invoke(this, screen)
    }
  1. To listen to the animation ending you will have to add some code too:
@Composable
public inline fun Transition<EnterExitState>.ListenerEffect(
    noinline transitionSpec: @Composable Transition.Segment<EnterExitState>.() -> FiniteAnimationSpec<Int> = {
        spring(visibilityThreshold = 1)
    },
    label: String = "ListenerEffect",
    onEnterStart: () -> Unit = {},
    onEnterEnd: () -> Unit = {},
    onExitStart: () -> Unit = {},
    onExitEnd: () -> Unit = {},
) {
    // This slightly akward code might be improved in the future if there is a better solution:
    // https://kotlinlang.slack.com/archives/CJLTWPH7S/p1677138472614839
    val animationProgress by animateValue(Int.VectorConverter, transitionSpec, label) { enterExitState ->
        when (enterExitState) {
            EnterExitState.PreEnter -> 200
            EnterExitState.Visible -> 100
            EnterExitState.PostExit -> 0
        }
    }
    when (animationProgress) {
        200 -> onEnterStart()
        100 -> when (currentState) {
            EnterExitState.PreEnter -> when (targetState) {
                EnterExitState.Visible -> onEnterEnd()
                else -> Unit // ignore
            }
            EnterExitState.Visible -> when (targetState) {
                EnterExitState.PostExit -> onExitStart()
                else -> Unit // ignore
            }
            else -> Unit // ignore
        }
        0 -> onExitEnd()
    }
}

The animation listener code might be improved, its a bit rudimentary but it works (see the slack thread in the comments).

Hope this helps you out!

Please note that the .childNavigators() extension is also made by us but is too deeply integrated to just share. Basically we store the currently active Navigators in a place where non-compose can access it. That way we can find the child navigators of a particular screen.