When to destroy activity's presenter

[konmik]

We need a more reliable way to detect when to destroy activity's presenter.

Here is where the issue that started the discussion: #42 (comment)

TL/DR: activity's finish() can be called some time after onPause, so this code will leak the activity's presenter: https://github.com/konmik/nucleus/blob/master/nucleus/src/main/java/nucleus/view/NucleusActivity.java#L76

Possible solutions:

a) Leverage retained fragments. This will solve the presenter destruction issue but we will leak background tasks sometimes.

b) Extend PresenterLifecycleDelegate lifecycle to explicitly call onDestroy.

c) Install system-wide lifecycle listener registerActivityLifecycleCallbacks.

[duo2005duo]

if an activity may not restart forever,why not leak the background tasks?

[konmik]

Because some background tasks can be very resource-consuming (streaming, image processing, etc).

[duo2005duo]

I think the system will drop an activity only after a certain time(
maybe several minutes).The resource-consuming tasks will be completed in the certain time and the result have already been saved with the help of saveInstanceState.
if the tasks is long enough to exceed the certain time.Just let it be a tasks with no end.Tasks will accumulate in such situation,which waste the system resource and the system will crash in the end.

[konmik]

Android never drops activities after some time alone. It drops them only with the entire process (together with background tasks).

[duo2005duo]

No,If an activity is paused or stopped, the system can drop the activity from memory by either asking it to finish, or simply killing its process

[konmik]

Do you have any references to docs or tests?

I created a test once which runs 100500 activities one on top of another - just to check if Android will finish some of background Activities when it becames short on memory. It didn't.

[konmik]

There is also a discussion on stackoverflow somewhere with the same conclusion.

[duo2005duo]

Yes, doc I think the system drop the activity in this situation because of time but not memory.

[konmik]

:D such an obvious lie in official docs. I never had a sutuation when Android finishes my Activities without my own command. It can finish them indirectly if I say FLAG_ACTIVITY_CLEAR**, but it never finishes them by itself.

How much time do you think we need for activity to be finished because of time?

[konmik]

I've found something interesting: http://developer.android.com/guide/components/tasks-and-back-stack.html

"If the user leaves a task for a long time, the system clears the task of all activities except the root activity. "

[konmik]

http://developer.android.com/guide/topics/manifest/activity-element.html#always

[duo2005duo]

Ok,It said 30 minutes .I happened to see it before and I forget where,you find it.

[konmik]

It is always nice to know more info about these lifecycles, thanks for mentioning them! :)

[bejibx]

Hey, did you red this article? i think it is really nice way to bound to Activity lifecycle.

[konmik]

@bejibx I hope you're kidding. This ugly workaround using one of the most buggy Android APIs will never make its way to my code. Never.

[bejibx]

Workaround is kind of ugly, true, but it seems to me pretty much every possible solution to beat Activity lifecycle is ugly. Still I'm a bit confused, because loaders seems to work like a charm in my projects before.

[konmik]

@bejibx you're lucky then. Their lifecycle is not the same as Activity has and not the same as process has. So, they obviously add complexity instead of reducing it. And, in addition to the added complexity, it has so many callbacks and unneeded API complexities like sending arguments in a Bundle, forceLoad... they just suck.

[konmik]

Nucleus exists only because loaders do not work.

[bejibx]

I got your point and I'm definitely agree with you about Loaders API over-complexity. I hope I'll have some time on this weekends to investigate Android Activity and Loader sources to see if system somehow diferentiate restart from complete destroy.

[konmik]

44c7722

[duo2005duo]

44c7722 do not fix the situaction in case 2 "restart-activity" in your post.In that situation,the method isFinishing still returns false.

[konmik]

@duo2005duo Which post do you mean?

[konmik]

This one?

"Case 2: An Activity restart happens when a user has set "Don't keep activities" checkbox in Developer's settings and another activity becomes topmost." ?

[duo2005duo]

@konmik Yes

[konmik]

I think it will have isFinishing during its next onDestroy.

[bejibx]

@konmik this is true, just checked it in sample project.

[duo2005duo]

Of course.But do you think the retained fragment lifecycle is similar to current presenter lifecycle？Combining staticholder with saveInstanceState just do the same with retained fragment.I think it voliate KISS.

[duo2005duo]

And I think the loader lifecycle is the same with retain fragment.They are both NonConfigurationInstances.

[konmik]

@duo2005duo the idea is to not have the same background task running twice. Without having a static variable we can't detect if the task is still running. Ratained fragments do not fit because they get destroyed during "case 2".

[duo2005duo]

Yes,But I think case 2 is not so important,The user will not do this except the developer,so I said "similar". And it still need to be tested whether it will have isFinishing during its next onDestroy for cleaning stack we talked about yesterday

[duo2005duo]

And I think it will not have its fininishing,the activity will not destroy twice without have a second onCreate.

[konmik]

@duo2005duo You're right this is not very important. But why complicate things and use fragments if we can do the same without them? Fragments is not the only thing we need presenters for.

It will have a second onCreate and a second onDestroy with isFinishing. I tested.

[duo2005duo]

The result is interesting !:-D

[bejibx]

Also I found this mentioned earlier stackoverflow conversation about Android destroying activities. So I think when lead Android engineer says

The only memory management that impacts activity lifecycle is the global memory across all processes, as Android decides that it is running low on memory and so need to kill background processes to get some back.

we can trust him.

[bejibx]

I think I found this rare case when we can leak our presenters because isFinishing() returns false.
What we need is "Do not keep Activities" developer option enabled and following backstack:
Activity 1 -> Activity 2 -> [Activity 3]
So at this point Activity 1 and 2 was destroyed with isFinishing() == false and we expect to destroy our pesenters when user navigates to them later with back button. Now imagine if we use up navigation from Activity 3 to Activity 1. onDestroy on Activity 2 will not be called (tested) and we'll lost our presenter.

[konmik]

@bejibx You're right, we will have a leak in this case. I was already thinking about this.

I think we need to recommend android:alwaysRetainTaskState flag to prevent such leaks.

[konmik]

While this flag doesn't help with "Do not keep Activities" it will help with Android finishing activities by itself.

[bejibx]

But did you test if isFinishing() == false when Android destroys Activities this way? I was trying to test it yesterday - I leaved my task for something around 3 hours and when I checked, it turned out that Android just killed my process. Maybe thats because my test was incorrect - I had only 1 task.

Also I tested what is gonna happen with flag android:clearTaskOnLaunch="true". Find out isFinishing() == true on all destroyed Activities.

[bejibx]

Also just found this interresting stackoverflow article.

[bejibx]

final ActivityRecord resetTaskIfNeededLocked(ActivityRecord taskTop,
            ActivityRecord newActivity) {
        boolean forceReset =
                (newActivity.info.flags & ActivityInfo.FLAG_CLEAR_TASK_ON_LAUNCH) != 0;
        if (ACTIVITY_INACTIVE_RESET_TIME > 0
                && taskTop.task.getInactiveDuration() > ACTIVITY_INACTIVE_RESET_TIME) {
            if ((newActivity.info.flags & ActivityInfo.FLAG_ALWAYS_RETAIN_TASK_STATE) == 0) {
                forceReset = true;
            }
        }
//...

If we look at ActivityStack#resetTaskIfNeededLocked(ActivityRecord, ActivityRecord) function we can see that no matter why activity should be killed - because of inactive reset time came off or because clearTaskOnLaunch flag was setted on Activity, actual Activity-by-time-removal mechanism stays the same, it only affects forceReset flag. So I assume if isFinishing() correctly returns true in my second test, we'll have same result for Activities destroyed by inactive reset mechanism.

[konmik]

I'm thinking about retained fragments and onRetainCustomNonConfigurationInstance :(

[konmik]

Here is a possible implementation, BTW: https://github.com/square/flow/blob/master/flow/src/main/java/flow/InternalLifecycleIntegration.java

I need to test retained fragments over edge cases.

[bejibx]

Calm down for a moment, we just need to test first what happens with Activity when system destroys it because of time. It is confirmed this could be easily reproduced on Android 2.3:

This behaviour can be easily reproduced on devices running Gingerbread and earlier. Launch an app and create some back history, then hit the home button and wait half an hour. Launch the app again from the home screen and the state has been cleared as if it were starting a new task. Perfect.

I'll test this on emulator tonight or tomorrow if I there is no time today and report results.

[konmik]

@bejibx Even if it will call onDestroy with isFinishing this will not solve the "Do not keep activities" + "Clear stack" issue.

[konmik]

moving to retained fragments will also automatically solve fragment's presenter lifecycle problem where we need to manually destroy presenter.

[konmik]

We will have sometimes several instances of the same background task running but it will, at least, be a predictable scenario every other android developer is facing.

[duo2005duo]

@konmik The RxJava will stop the unneccessary task instances as soon as possible with the help of isUnscribed().There is no problem.

[bejibx]

Hey, can we use isChangingConfigurations() instead of isFinishing()? So presenter will NOT be destroyed only if it is configuration change incoming.

[konmik]

@bejibx awesome. I will try it.

[konmik]

3.0.0-beta is on maven

[konmik]

2863995