understanding leaks in android MVP

Question

I've read some similar questions on here but due to the lack of code presented I'm not certain my question describes the same scenarios.

I hope that the following snippets and questions will help others clarify what and when something is leaked in this MVP implementation: https://github.com/frogermcs/GithubClient/tree/1bf53a2a36c8a85435e877847b987395e482ab4a

BaseActivity.java:

public abstract class BaseActivity extends AppCompatActivity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setupActivityComponent();
    }

    protected abstract void setupActivityComponent();
}

SplashActivityModule.java:

@Module
public class SplashActivityModule {
    private SplashActivity splashActivity;

    public SplashActivityModule(SplashActivity splashActivity) {
        this.splashActivity = splashActivity;
    }

    @Provides
    @ActivityScope
    SplashActivity provideSplashActivity() {
        return splashActivity;
    }

    @Provides
    @ActivityScope
    SplashActivityPresenter
    provideSplashActivityPresenter(Validator validator, UserManager 
    userManager, HeavyLibraryWrapper heavyLibraryWrapper) {
        return new SplashActivityPresenter(splashActivity, validator, 
                                           userManager, heavyLibraryWrapper);
    }
}

SplashActivityPresenter is injected inside SplashActivity.java:

public class SplashActivity extends BaseActivity {
    ...

    @Inject
    SplashActivityPresenter presenter;

    @Override
    protected void setupActivityComponent() {
        GithubClientApplication.get(this)
                .getAppComponent()
                .plus(new SplashActivityModule(this))
                .inject(this);
    }

SplashActivityPresenter.java:

public class SplashActivityPresenter {
    public String username;

    private SplashActivity splashActivity;
    private Validator validator;
    private UserManager userManager;
    private HeavyLibraryWrapper heavyLibraryWrapper;

    public SplashActivityPresenter(SplashActivity splashActivity, 
        Validator validator, UserManager userManager, 
        HeavyLibraryWrapper heavyLibraryWrapper) {
        this.splashActivity = splashActivity;
        this.validator = validator;
        this.userManager = userManager;
            this.heavyLibraryWrapper = heavyLibraryWrapper;

        //This calls should be delivered to ExternalLibrary right after it will be initialized
        this.heavyLibraryWrapper.callMethod();
        this.heavyLibraryWrapper.callMethod();
        this.heavyLibraryWrapper.callMethod();
        this.heavyLibraryWrapper.callMethod();
    }

    public void onShowRepositoriesClick() {
        if (validator.validUsername(username)) {
            splashActivity.showLoading(true);
            userManager.getUser(username).subscribe(new 
SimpleObserver<User>() {
                @Override
                public void onNext(User user) {
                    splashActivity.showLoading(false);
                    splashActivity.showRepositoriesListForUser(user);
                }

                @Override
                public void onError(Throwable e) {
                    splashActivity.showLoading(false);
                    splashActivity.showValidationError();
                }
            });
        } else {
            splashActivity.showValidationError();
        }
    }
}

1. If the user rotates the screen while the username is being fetched we are leaking the activity instance being referenced inside the observer's callbacks since the activity is destroyed.
2. If the user rotates the screen without an in-progress fetch, the activity instance is not leaked.
3. In order to fix this leak(1), we need to store the subscription and unsubscribe it in presenter.onDestroy() (called from SplashActivity onDestroy()).
4. Someone told me that doing (3) is not enough, and that inside onDestroy() we must also set the activity instance to null. I disagree because unsubscribing the subscription cancels the request, preventing the callbacks(like onNext(User)) that reference the activity from being invoked.
5. He also told me that while (3) and (4) prevent the ACTIVITY leaking, the PRESENTER is leaked during rotation ALSO since the activity references it. I disagree because a new presenter is created per rotation and initialized to the injected presenter when BaseActivity onCreate invokes setupActivityComponent. The old presenter is automatically garbage collected as a member of SplashActivity.

Can someone respond to the points I outlined above so I can confirm my understanding or learn where I may be wrong? Thank you

Answer 1

I'll do my best to answer these as accurately as possible (welcome edits if inaccurate), but this answer is good reading: https://stackoverflow.com/a/10968689/4252352

1. If the user rotates the screen while the username is being fetched we are leaking the activity instance being referenced inside the observer's callbacks since the activity is destroyed.

A : This is correct, however will be short term memory leak until the resources are cleared after dispose. However this should not be relied upon, if you had a Flowable/Observable it may never dispose and clear resources. It is important to note that all lambdas in a Rx chain (usually operators like map, filter etc) that don't reference (capture) the enclosing class are leak free.

2. If the user rotates the screen without an in-progress fetch, the activity instance is not leaked.

A. Correct, you never have an active subscription.

3. In order to fix this leak(1), we need to store the subscription and unsubscribe it in presenter.onDestroy() (called from SplashActivity onDestroy())

A This should stop the issue. However a better approach, and in MVP the View should be an abstraction/interface and your Presenter should have entry and exit points for the view and not on the constructor i.e bind(view : View) and unbind() <-- cleanup here, a presenter should not be aware of specific android hook callbacks. This has massive advantages, not only from a OOP (program to interfaces not implementations) aspect, but also testing.

4. Someone told me that doing (3) is not enough, and that inside onDestroy() we must also set the activity instance to null. I disagree because unsubscribing the subscription cancels the request, preventing the callbacks(like onNext(User)) that reference the activity from being invoked.

A. I'd would firstly ask then to clarify their reasoning. As your Presenter is scoped to the Activity (they both have the same lifecycle) unsubscribing should be enough. However if your Presenter has a longer lifecycle than the Activity it would be necessary to remove the reference (this may have been the rationale of the person who you spoke to).

5. He also told me that while (3) and (4) prevent the ACTIVITY leaking, the PRESENTER is leaked during rotation ALSO since the activity references it. I disagree because a new presenter is created per rotation and initialized to the injected presenter when BaseActivity onCreate invokes setupActivityComponent. The old presenter is automatically garbage collected as a member of SplashActivity.

A. The Presenter is leaked, if the Activity is leaked (along with everything the activity references!)