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

fix activity leak after configuration change #415

Closed
wants to merge 1 commit into from
Closed

fix activity leak after configuration change #415

wants to merge 1 commit into from

Conversation

shpasha
Copy link
Contributor

@shpasha shpasha commented May 16, 2024

ISSUE SOLVED: 112

Problem

During the configuration change process, the onDispose method in AndroidScreenLifecycleOwner is not called. As I understand it, this is normal, because almost all DisposableEffects in voyager ignore configuration changes.

But this leads to the fact that atomicContext.getAndSet(null) in onDispose is not executed and the activity leaks.
The side effect of such a leak is that when the onDispose method in AndroidScreenLifecycleOwner is called, it will not work correctly, because this code
if (activity != null && activity.isChangingConfigurations) return
will complete the function execution because reference here for the old activity in which isChangingConfigurations is true

Solution

I simply add a callback to the lifecycle that resets the atomicContext field when activity is in onDestory state.

Copy link
Contributor Author

@shpasha shpasha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also another solution

Let's look at this code from AndroidScreenLifecycleOwner:

override fun onDispose(screen: Screen) {
        val context = atomicContext.getAndSet(null) ?: return
        val activity = context.getActivity()
        if (activity != null && activity.isChangingConfigurations) return << useless check?
        viewModelStore.clear()
        disposeEvents.forEach { event ->
            lifecycle.safeHandleLifecycleEvent(event)
        }
    }

If onDispose is not called when the configuration changes (or am i wrong?), maybe we can get rid of activity.isChangingConfigurations check, then get rid of the reference to the activity context and store a reference to the application context instead?

@DevSrSouza
Copy link
Collaborator

If onDispose is not called when the configuration changes (or am i wrong?), maybe we can get rid of activity.isChangingConfigurations check, then get rid of the reference to the activity context and store a reference to the application context instead?

I was dive into the code this days and I also though about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants