Regarding SubscriptionList on config changes

[jmreyes]

Hi!

First of all, thank you for this awesome approach to handling MVP and configuration changes! I have run into some doubts/problems regarding the RxPresenter implementation, hope you could help me clear them up.

The SubscriptionList inside the presenter gets only unsubscribed onDestroy. This apparently means that when we have a configuration change (isFinishing() not called) it's causing a "transient" memory leak (it gets detected by LeakCanary), where the Subscriptions inside SubscriptionList will keep a reference to the views that were active before the config change. The following screenshot shows what I mean, as found in MAT:

I mean "transient" since as soon as the current view is really destroyed the presenter is gone for good, so the old views can finally be garbage collected; there's however a period of time where we can have whole view hierarchies leaked.

The workaround I found was simply not adding the Subscriptions to the SubscriptionList with add() (note that I am not using the restartable functionality, and always using DeliverFirst). What's the downside of this? Everything seems to work as expected, but I've noticed the Presenter is kept around until the task is finished, which is bad but at least the views are not kept (avoiding all the problems this brings). Moreover, this way makes LeakCanary not complain any more.

Am I missing something? Please also note that I'm a novice in RxJava matters, so that could be my main problem.

Thanks a lot!

[konmik]

Hello,

When a view gets destroyed combineLatest receives null instead of view so it should not keep a reference to the view anymore.

Could you please give me the piece of code that reproduces the issue? I'll try to check it with LeakCanary as well.

[konmik]

The downside of not using restartables is that when you device is low on memory (or your device is Samsung with shitty memory settings) you can lose your background tasks while your app is in the background.

In my opinion, if you're not going to use restartables then there is no reason to use MVP at all. :D

[jmreyes]

Hi,

I've uploaded a project that reproduces the problem (it includes a memory dump for MAT showing two instances of MainActivity and two of MainActivityFragment in memory). I've also noticed this only happens when using DeliverFirst, not for DeliverLatestCache or DeliverReplay, and the leak appears for both restartables and non-restartables.

About my question, I was referring to the downside of skipping the call to add() for our Subscriptions. I understand the importance of restartables because of Android killing the process but later restoring the view and so on, even if I acknowledge that's something usually ignored in Android development. The thing is, I am working with a big codebase that I want to refactor into using this approach and my idea was to keep things as simple as possible at first, hence avoiding adding more code to keep request state for now.

Not sure why you mention that there's no reason for MVP in this case, in my opinion it's worth it even if it's only to make background tasks survive configuration changes. Of course that not depends on the MVP pattern itself but on the concrete implementation that takes care of that. That's why I like Nucleus, it's just simple and does the job. But yes, maybe if one is only interested in surviving config changes/process restarts for small projects, using MVP can be overkill. I'll have a look at your Satellite library, looks promising!

[konmik]

Thanks for the project, I'll look into it soon!

About not using restartables. There are no means for View to detect if it is not getting it's data because a request is too long or because the request has been destroyed. This is why restartables matter - they eliminate the "request has been destroyed" reason. Yep, it is up to you to use them. It was the reason for me. :D

[konmik]

ReactiveX/RxJava#3379

[konmik]

Check out 2.0.2.

Thanks for the great report! :)