6

lately i have been researching about memory leaks in java/android and pretty much everywhere it says that instead of anonymous classes i should use static inner classes with weak references.
so, in my android app i started doing that but very quickly got tired of it because it's a lot of boilerplate code... i think have an alternative solution which i would prefer to use, but i'm juts not sure that it is a valid alternative to static inner classes in terms of preventing memory leaks. as i said before, i haven't seen this solution suggested anywhere else (all say to use static inner classes) so thats why im not sure my alternative will work.

ill use a simple example from my app:
i have a class called WebClient which handles asynchronous web requests and it accepts an interface called iCallback which returns the response from the server to the caller, and in my activity once i get this callback i need to dismiss a dialog, and maybe perform some activity related things (like trigger onBackPressed() and setResult()).
so here is my static inner class i have created:

private static class CallBack implements WebClient.ICallback
{
    private WeakReference<ProgressDialog> mProgDiag;
    private WeakReference<BaseActivity> mActivity;

    public CallBack(BaseActivity activity, ProgressDialog progDiag)
    {
        this.mProgDiag = new WeakReference<>(progDiag);
        this.mActivity = new WeakReference<>(activity);
    }

    @Override
    public void onCallback(String data)
    {
        String responseAsString = Utils.extractStringFromResponse(...);

        final BaseActivity parentActivity = mActivity.get();
        ProgressDialog dialog = mProgDiag.get();

        if(dialog != null)
        {
            dialog.dismiss();
        }

        if (responseAsString == null)
        {
            if(parentActivity != null)
            {
                Utils.makeServerErrorDialog(parentActivity,
                                            new iDialogButtonClickedListener()
                                            {
                                                @Override
                                                public void onDialogButtonClicked()
                                                {
                                                    parentActivity.onBackPressed();
                                                }
                                            });
            }

            return;
        }

        //everything is ok
        if (responseAsString.equals("1"))
        {
            if(parentActivity != null)
            {
                Intent result = new Intent();
                result.putExtra(...);

                parentActivity.setResult(Activity.RESULT_OK, result);
            }
        }

        else
        {
            Utils.reportErrorToServer(...);

            if(parentActivity != null)
            {
                parentActivity.setResult(Activity.RESULT_CANCELED);
            }
        }

        if(parentActivity != null)
        {
            parentActivity.onBackPressed();
        }
    }
}

so for every variable i need in this static inner class i have to create a new weak reference, then retrieve the object itself, and then every time i want to access it i need to check whether it's null... that seems like a lot of code to me.

and here is my suggested alternative:

public abstract class BaseActivity extends AppCompatActivity
        implements WebClient.ICallback
{
    private static final String TAG = "BaseActivity";

    WebClient.ICallback mCallBack;
    ProgressDialog mProgDiag;

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

        setContentView(...);

        mCallBack = this;

        //some code to invoke a server request on button click
        //and passing mCallBack to the request
    }

    @Override
    public void onCallback(String data)
    {
        String responseAsString = Utils.extractStringFromResponse(...);

        mProgDiag.dismiss();

        if (responseAsString == null)
        {
            Utils.makeServerErrorDialog(this,
                                        new iDialogButtonClickedListener()
                                        {
                                            @Override
                                            public void onDialogButtonClicked()
                                            {
                                                onBackPressed();
                                            }
                                        });

            return;
        }

        //everything is ok
        if (responseAsString.equals("1"))
        {
            Intent result = new Intent();
            result.putExtra(...);

            setResult(Activity.RESULT_OK, result);
        }

        else
        {
            Utils.reportErrorToServer(...);

            setResult(Activity.RESULT_CANCELED);
        }

        onBackPressed();
    }

    @Override
    protected void onPause()
    {
        mCallBack = null;

        super.onPause();
    }

    @Override
    protected void onResume()
    {
        super.onResume();

        mCallBack = this;
    }
}

to me this seems much cleaner: no creating and retrieving instances of weak references for every variable i need access to, i can directly invoke activity methods (e.g. onBackPressed()), and no checking for null everywhere.
the only place i would now have to check for null is inside WebClient class before invoking the callBack method.

so my question is, does this approach achieve the same result in terms of preventing memory leaks? is it a "worthy" alternative to static inner classes?

TacB0sS
  • 10,106
  • 12
  • 75
  • 118
or_dvir
  • 479
  • 1
  • 7
  • 22
  • Good question, been also wondering myself if I really need a WeakReference for every variable and checking for null every time – Denny May 04 '17 at 09:46
  • Well, some `null` checks are still needed in your solution. But I think your main problem is that you check those value when you need them. Just do the check once (for `parentActivity` in your example). – AxelH May 04 '17 at 09:51
  • There's no such thing as a "static inner class" in Java, as the Java definition of "inner class" is a nested class that is not static. – Lew Bloch May 04 '17 at 10:01
  • 2
    There's absolutely nothing wrong with anonymous classes. They are extremely useful. Making every nested class static with weak references is weird superstition. There's no engineering basis for such a blanket rule. As you are finding, it leads to over-engineered crap and away from the problem domain. Give up the bad advice someone gave you ("pretty much everywhere"? I think not!) and return to sensible programming practices. – Lew Bloch May 04 '17 at 10:06
  • What would be `sensible programming practices` in this case? – Denny May 04 '17 at 10:10
  • @AxelH you are probably right, there are probably some modification i can do to use less checks, but i cannot do that everywhere, and it doesnt really matter for my specific case as this is a general question – or_dvir May 04 '17 at 11:15
  • A general question that point : "_so for every variable i need in this static inner class i have to create a new weak reference, then retrieve the object itself, and then every time i want to access it i need to check whether it's null_" so I find that right on the problem. This is easily corrected by having a good design in the method. – AxelH May 04 '17 at 11:17
  • @LewBloch im not saying that they are bad and you should never use them. sometimes they are useful, but sometimes they can lead to memory leaks like in my case above where the activity is killed before the callback has been triggered. to fix this issue, static inner classes (or static nested class) are used because they do not hold an implicit reference to the containing activity. so i am wondering if my proposed solution is equivalent in terms of preventing memory leaks – or_dvir May 04 '17 at 11:25

1 Answers1

3

Unfortunately, your approach does not work. By implementing the WebClient.ICallback in your activity, rather than an inner class, you don't get rid of the leak. The leak happens not because the references to activity and dialog are implicit in an anonymous class, or in lambda, or in a non-static inner class instance; the happens when the WebClient keeps this reference while the activity is gone (it is not destroyed, because there is a strong reference to it).

The special mCallBack that you set to null when the activity is paused, gains nothing. Just as well, you can simply pass your activity instance to the WebClient. Now there is a strong reference to your activity, which is managed by someone (async handlers of the WebClient), who is not under your control. If you are unlucky, the async handler will get stuck somewhere and will never release this reference.

Please read this detailed explanation.

Note that WebView itself can cause a memory leak, if special measures are not undertaken!

Community
  • 1
  • 1
Alex Cohn
  • 56,089
  • 9
  • 113
  • 307
  • why does mCallback not gain anything? This is my logic, please explain why i'm wrong: if i pass the REFERENCE mCallback to WebClient, then it hold the same reference as the one that is held by the activity. so, when the activity is paused, that reference is null both on the activity AND in WebClient (because they both point to the same object in memory) - therefore, when the activity is paused, WebClient no longer holds a reference to the activity (it holds a reference to null), and the activity can be garbage collected. – or_dvir May 09 '17 at 12:06
  • Unfortunately, Java references don't work this way. When you pass reference to mCallBack to WebClient, it does not keep a 'pointer' to `myActivityInstance.mCallBack`. Rather, it keeps a separate reference that *duplicates* mCallBack. So, when you set mCallBack to **null**, the WebClient is not effected. If Java were working with reference counts, we could say that setting `mCallBack = this` set refcount=2, and launching WebClient increases it to 3. When onPause() sets `mCallBack = null` the refcount is 2 again. Java does not use refcount, but in this case the result is the same. – Alex Cohn May 09 '17 at 13:09