Using WeakReference in MvpBasePresenter is flawed

[olshevski]

Hi,

Using WeakReference inside MvpBasePresenter may potentially lead to NullPointerException in the code like this (taken from the sample code on your site):

if (isViewAttached())
          getView().showHello(greetingText);

There is no guarantee that GarbageCollector will not remove object from memory between two subsequent calls to WeakReference.get(). This way even when isViewAttached() returns true, getView() may return null. The correct and only way to use WeakReference in this case would be to call WeakReference.get() only once and assign the result to strong reference.

[sockeqwe]

Thanks for reporting! Indeed, that is true, unlikely but in theory this could happen! Actually I wanted to deprecate isViewAttached() in 3.0.

The proper way is:

V view = getView();
if (view !=null){
    view.showHello(...);
}

We could add a shorthand like this to MvpBasePresenter:

ifViewAttached(view -> view.showHello(...));

which internally uses View view = getView(); if (view != null) { ... } as shown in the first code snippet.

What do you think?

[olshevski]

I understand that it is very unlikely considering all conditions that should be met. But still it is a piece of code that feels uncomfortable c:

I guess deprecating isViewAttached() may prevent propagation of this code snippet. But I still can imagine someone writing their code this way:

if (getView() != null) {
    getView().doThis();
    getView().doThat();
}

It seems to be a common practice among various programmers.

ifViewAttached(view -> view.showHello(...)); seems fine.

Personally, I just don't see the point in this WeakReference-bulletproofing of someone else's bad code. But that's a whole another "ideal world" discussion.

[sockeqwe]

3.1.0 deprecates getView() and isViewAttached()and adds ifViewAttached(). Thanks for your feedback!

[gim-]

Wouldn't deprecating isViewAttached() and marking getView() as @Nullable be enough? That way if someone writes something terrible like this:

if (getView() != null) {
    getView().doThis();
    getView().doThat();
}

The lint-check in Android Studio will show a warning that getView().doThis(); and getView().doThat(); can throw NPE. If someone ignores that then app crashing would be his own fault. ifViewAttached() could be just a nice additional helper method.

EDIT: Ok, I just checked it and it seems like Android Studio will not show any warnings inside a block after if (getView() != null).

[sockeqwe]

If someone ignores that then app crashing would be his own fault.

Yes but with a more concise API (especially in kotlin) like

ifViewAttached { view -> 
   view.doThis()
   view.doThat()
}

we can avoid pain like NPE. Hence I think the deprecation of isViewAttached() and getView() is justified. What are your concerns about ifViewAttached() or why would you like to stick with getView()?

[jshvarts]

In Kotlin, can if checks be avoided and safe call operator be used instead? view?.doThis()

[sockeqwe]

yes but still this could happen (theoretically):

view?.toThis()  // this is executed
// gc runs and view == null
view?.toThat() // this is not executed because view == null because of gc

whereas with ifViewAttached() its a do all in the lambda because view is guaranteed not to be null or don't execute the lambda at all. Also we have to take java users into account.

Also this API seems to me more consistent to what MvpQueuingBasePresenter offers:

onceViewAttached { view -> 
   view.doThis()
   view.doThat()
}