MVP, set the view of the presenter to null on destroy?

Question

I am currently trying to implement the MVP pattern on Android. However, I came to think about memory leaks (since the presenter holds a reference to the activity - the view). My question is, should I set the view of the presenter to null say onDestroy of the activity?

This is my main activity:

public class MainActivity extends AppCompatActivity implements MainView {
private Button loadUser;
private TextView mTextView;
@Inject
IMainPresenter mPresenter;

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);
    setUpViews();

    ((MyApp) getApplication()).getAppComponent().inject(this);
    mPresenter.setView(this);
}

private void setUpViews() {
    loadUser = (Button) findViewById(R.id.getUserBtn);
    loadUser.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View view) {
            mPresenter.loadUserName(2);
        }
    });
    mTextView = (TextView) findViewById(R.id.userNameTextView);
}

@Override
public void setUserName(String userName) {
    mTextView.setText(userName);
}

@Override
protected void onDestroy() {
    super.onDestroy();
    mPresenter.setView(null);
}
}

Answer 1

I always did like that, I mean, I always set the view to null on the onDestroy() method. I try to use the LightCycle library to avoid having to code all that boilerplate code. Here's an article that explains Fernando Ceja's Clean Architecture. This is an MVP architecture widely used (I've worked in a couple of companies that use this pattern), there you can see that he also sets the view to null in the onDestroy() method. Let me know if I can help you any further.

UPDATE:

This answer is kinda outdated, now you can use the LifecycleOwner class from the Android's Jetpack tools. It's basically the same deal but with different API.

Answer 2

I'll assume that you are using an Injection library (probably Dagger looking at your code) and the Presenter is annotated with @Singleton? If so, then setting to null is one option (and yes you should not be retaining an Activity instance on configuration changes).

Another option is to use a WeakReference in your Presenter that way setting to null is not required, although setting to null is more explicit.

You might consider using Interfaces with your Presenter rather than exposing the whole Activity to the Presenter instance - you may already be doign something like this, but not 100% clear from code provided.

Answer 3

Scenario 1.

If Presenter's lifetime is not longer then Activity's, there is no need to set view to null. The important part is to stop background thread where your presenter executes its work. If you stop Presenter's background thread in onDestroy, then the thread is terminated and doesn't keep a link to the Activity anymore. So both Activity and Presenter can be collected by GC soon.

This scenario can often be found in the projects which use MVP and don't handle orientation change.

Scenario 2.

If Presenter's can live longer than Activity (e.g., singleton), then you should set View to null. Here cases like that are described.

If you want presenter's thread survive configuration change you most likely make you presenter lifetime longer then activity's.

---

A would recommend you to switch to MVVM instead of MVP. If you use Google Architecture components in a right way, Lifecycle-aware components will unsubscribe your Views and stop threads for you.

Answer 4

If you don't set the view(Activity) to null then your presenter is going to continue holding the reference to Activity. I will use Activity as an example but the same works for Fragments.

Is this a problem and why?

It depends on the lifecycle of the presenter. You need to understand how long does your presenter live in memory.

Case 1:

If your presenter is defined as Singleton (which I think you shouldn't be doing) then it stays in memory as long as the app itself.

This is a problem because then a singleton class will hold a reference to Activity, which consequently prevents it from ever being garbage collected. When your Activity is destroyed, it will be useless to the user, but will still occupy memory (RAM). Apart from taking precious memory, the presenter might receive events like successful API response, which will trigger a view.doSomething method, which can cause crashes (working with destroyed Activity's UI is not something you should ever do, the Android framework will throw exceptions).

Generally, Presenters are not so widely scoped so it's not an issue. If your presenters are scoped to live longer than Activity, you need to manually set the view reference to null to solve the problem.

Case 2:

Your Presenter might not be defined as a Singleton, but some other Singleton, or similar widely scoped component might be holding a reference to your Presenter, making it live longer than Activity.

You might have something like Eventbus or UserRepository or UserController Singleton class. Your Presenter might hold a reference to such object and be subscribed to it. When you subscribe, you're basically setting a reference of yourself to that other class. In Presenter, when you call eventbus.subscribe(this) or userRepository.setListener(this), you're giving a Singleton a reference to yourself. Singleton object holding a reference to your object makes your object in memory as long as the Singleton object lives, which is forever. If you're using lambdas, anonymous classes, or nested classes, such constructs hold a reference to their enclosing class, it may cause the following chain of references: Singleton->Anonymous class->Presenter->Activity

In such case, Singleton indirectly references an Activity, preventing it from being garbage collected!

Instead of userRepository.setListener(this), you might be using something like this:

userRepository.updateUser(object: Listener {
   onSuccess() {
      // do something
   }
   onFailure() {}
})

Let's say the updateUser method does a long operation (sends an API request). In such case, the reference is kept only while the update user request is in progress. When onSuccess or onFailure is invoked the reference is typically (implementations may vary) cleared. So here, a memory leak problem is not such a big of a deal, as the reference will be cleared, only with a slight delay. However, you still have the issue of updating the view after it is destroyed(described in Case 1) - you can reproduce it by exiting the activity while the request is in progress.

To solve the described issues, you need to use something like userRepository.clear() or eventbus.unsubscribe(this). The implementations of such methods should cancel all in-flight requests and remove the references to subscriber/listener (presenter). If you do it right, you don't even need to set presenter's view reference to null. Garbage collector is smart enough to figure out your Activity and Presenter objects are holding reference to each other, but no other component is holding reference to them and delete both of them from Memory. However, my recommendation is to do it anyway, since it's an easy operation which does not have to be repeated everywhere (can be done in BasePresenter class) and may save you if you forget to clear some reference manually.

Answer 5

Could you not make the presenter lifecycle aware using https://developer.android.com/topic/libraries/architecture/lifecycle.html ? The presenter could then itself be responsible for setting the view to null when onDestroy() is called on the view.