Demoritas/rx activity leak

[demoritas]

Fix an activity leak on slow connections

[spirosoik]

@demoritas Many issues with code indentation I think you are not using the code style which @android10 suggests so your IDE beautifies many things which are not necessary and the actual change is hidden under these. I suppose that the only change is the addition of leakcanary or am I missing something? Moreover please squash your commits into one.

In addition it's not needed to add leakcanary in this repository but you can fix the problem which you have already detected. So remove the library and keep the fix only.

[demoritas]

@spirosoik sorry for the code style (my IDE is configured with the code style we use at work)

I separated the commits so it is easier to follow the the issue and the fix

1. commit: shows how to reproduce the leak
2. commit shows the class used to avoid the leak
3. commit applies the fix to the method calls

[android10]

@demoritas as @spirosoik pointed out, we should keep codestyle consistent so I would suggest that use the one in my repo:
https://github.com/android10/java-code-styles

On the other hand, I appreciate your efforts in finding memory leaks, which we should solve, however I do not wanna add any other library to keep the example clean, which is the main purpose of this repo, so anything but the necessary libraries to make it work should be removed (LeakCanary in this case).

[spirosoik]

@demoritas Please add javadoc comments to understand the problem which you are solving better. For example why are you using a SubscriptionDecorator in UseCases

[android10]

@spirosoik 👍 but the details should be added in the PR description. Comments generate noise and code should be self explanatory.

@demoritas please squash all the commit into one. I will write documentation in this repo with collaboration guidelines.

Thanks guys!

[spirosoik]

@android10 I agree for the noise, it would be better to explain us here in your comment what are you trying to solve.

[demoritas]

+1 for the codestyle, sorry for that, I try to re-setup this PR when I have time but leave it here as documentation for the issue for now if you aggree

Problem description: rx.Subscription#unsubscribe does not remove the reference to the rx.Subscriber. In your case for example the UserListSubscriber has a reference to the enclosing UserListPresenter which holds a reference to the Fragment (via viewListView) which holds reference to the activity (via UserListListener). If the Activity is scheduled to be destroyed during a network request (RestApiImpl#userEntityList()) it is leaked through the reference to the rx.Subscriber.

The PR is there to illustrate and easily reproduce the problem.

the first (and easy) fix would be nulling userListListener in onDetach() but this leaves the problem of the subscriber leaks which can be solved by wrapping the subscriber and clearing the reference to the inner subscriber in Subscriber#unsubscribe() as shown in this PR.

[demoritas]

[demoritas]

this shows that the subscriber is unsubscribed but still holds all references (ignore the sysouts in the code ^^)

[android10]

@demoritas great information!

[demoritas]

@android10 which code style are you using, if I applied SquareAndroid and format the code it applies changes
public UserListFragment() { super(); }
->
public UserListFragment() {
super();
}

[spirosoik]

@demoritas great explanation thanks for the information dude. this is the code style
https://github.com/android10/java-code-styles

[demoritas]

@spirosoik
I followed the instructions there and still have the change above (I applied the SquareAndroid codestyle))

[spirosoik]

@android10 any ideas for the codestyle? I think there are a few code lines which is not formatted with this codestyle.

[demoritas]

I added a PR for the immediate issue (nulling the reference to the activity), this PR here is more a workarround for an RX issue on android so maybe it is out of scope for the clean code project

[android10]

@demoritas this fixes the issue: a4f207e

This actually enables the garbage collector to wipe out all references to views.

The wrapper you proposed, was a good idea but I considered we were overthinking here and we would have ended up paying a price: accidental complexity. Check the solution, leak canary is not shouting anymore about any leaked activity. (I finally added it to the project).

I really appreciate your effort and contribution buddy!

[android10]

@demoritas @spirosoik please check guys. I'm closing this PR, the fixed was way easier than expected. I also added LeakCanary as @demoritas suggested in the beginning. Good catch by the way!