Anonymous Listener of volley request causing memory leak

Question

I am using volley library for making web-services call. I made a general class for making all web services call and making service call from there and made anonymous listener for successful and error response.

But when I use leak canary it is showing memory leak related to context. Below is my snippet of code:

public void sendRequest(final int url, final Context context, final ResponseListener responseListener, final Map<String, String> params) {
    StringRequest stringRequest;
    if (isNetworkAvailable(context)) {

      stringRequest = new StringRequest(methodType, actualURL + appendUrl, new Listener<String>() {
            @Override
            public void onResponse(String response) {
                dismissProgressDialog(context);
                try {
                    (responseListener).onResponse(url, response);
                } catch (JsonSyntaxException e) {
                    // Util.showToast(context, context.getResources().getString(R.string.error));
                    Crashlytics.logException(e);
                }
            }

        }, new ErrorListener() {
            @Override
            public void onErrorResponse(VolleyError error) {
                // Util.showToast(context,context.getString(R.string.error));

                dismissProgressDialog(context);
                if (error instanceof NetworkError) {
                     Util.showToast(context, context.getResources().getString(R.string.network_error));
                } else if (error instanceof NoConnectionError) {
                     Util.showToast(context, context.getResources().getString(R.string.server_error));
                } else if (error instanceof TimeoutError) {
                     Util.showToast(context, context.getResources().getString(R.string.timeout_error));
                } else {
                     Util.showToast(context, context.getResources().getString(R.string.default_error));
                }


            }

        }) {
            @Override
            protected Map<String, String> getParams() throws AuthFailureError {
                return params;
            }


            @Override
            public Map<String, String> getHeaders() throws AuthFailureError {
                return request.getHeaders(context, actualURL, false);
            }
        };
        stringRequest.setRetryPolicy(new DefaultRetryPolicy(30000, DefaultRetryPolicy.DEFAULT_MAX_RETRIES, DefaultRetryPolicy.DEFAULT_BACKOFF_MULT));
        VolleySingleton.getInstance(context).addRequest(stringRequest);
    } else {
         Util.showToast(context, context.getString(R.string.internet_error_message));
    }
}

And I created an interface named response listener for redirecting responses to activity or fragment. I made request as follows.

Request.getRequest().sendRequest(Request.SOME URL, SplashScreenActivity.this, SplashScreenActivity.this, new HashMap<String, String>());

But I am facing memory leak as:

In 2.1.1:31.
* activity.SplashScreenActivity has leaked:
* GC ROOT com.android.volley.NetworkDispatcher.<Java Local>
* references network.Request$5.mListener (anonymous subclass of com.android.volley.toolbox.StringRequest)
* references network.Request$3.val$responseListener (anonymous implementation of com.android.volley.Response$Listener)
* leaks activity.SplashScreenActivity instance
* Retaining: 1.2MB.
* Reference Key: b8e318ea-448c-454d-9698-6f2d1afede1e
* Device: samsung samsung SM-G355H kanas3gxx
* Android Version: 4.4.2 API: 19 LeakCanary: 1.4 6b04880
* Durations: watch=5052ms, gc=449ms, heap dump=2617ms, analysis=143058ms

Any idea to remove this leak any help is appreciated.

Deyoung

Answer 1

Generally, Anonymous classes have a strong reference to the enclosing class instance. In your case, that would be SplashScreenActivity. Now I guess, your Activity is finished before you get the response from your server through Volley. Since the listener has a strong reference to enclosing Activity, that Activity cannot be garbage collected until the Anonymous class is finished. What you should do is tag all the requests you are sending with the Activity instance, and cancel all the requests at onDestroy() callback of Activity.

stringRequest.setTag(activityInstance);

To cancel all pending requests:

requestQueue.cancellAll(activityInstance);

Also, use Application context inside VolleySingleton to create the RequestQueue.

mRequestQueue = Volley.newRequestQueue(applicationContext);

Don't use your Activity context there and don't cache your Activity instance inside VolleySingleton.

Lagas

I had this issue and I tried cancellAll() using tag. But for some reason memory leak continued. Then I used cancellAll() and passed a request filter that cancels all requests. Problem disappeared. I have no idea why that happened! – Winonawinonah

@Dinesh that's correct approach but injecting this code in activity will closely couple this solution with that particular activity and you need to perform same logic in every activity making network calls – Photographer

@NayanSrivastava Actually, I dont do network calls from Activity. I will have View Layer, Domain Layer, and Data Layer. Only Data Layer sends requests - through Volley. BTW, setTag accepts any object stringRequest.setTag(anyObject); – Lagas

@DineshBob That's good, I was just arguing instead of controlling API calls outside what if we handle them withIn the layer that makes API calls – Photographer

Answer 2

Basically the anonymous approach is terrible in Android or in any ClientSideSystem where you don't have massive memory. What is happening is, you have passed Context as parameter in method and anonymous holds a reference of it. The real mess comes now in the scene when the thread inside which makes network call could not finish it's job and before that the calling activity for some reason either destroys or recycles in that case GC is not able to collect the activity as wokerThread might still be holding reference onto it. Please go through this for detail description.

The solution could be either static inner classes or independent classes, in both cases use WeakReference to hold resources and do a null check before using them.

Advantage of WeakReference is it will allow GC to collect the object if no-one else if holding reference onto it.

Photographer

@ Nayan Srivastava can u please edit code i m not getting how to fix this in existing code – Deyoung

Instead of new Listener<String>() { @Override public void onResponse(String response) { dismissProgressDialog(context); try { (responseListener).onResponse(url, response); } catch (JsonSyntaxException e) { // Util.showToast(context, context.getResources().getString(R.string.error)); Crashlytics.logException(e); } } } directly create a class which implements this and pass that classes Object inside – Photographer

i have to create class which implements Listner interface and have to pass that class object at there. like class ResponseClass implements Listener,ErrorListener{ Override public void onResponse(Object response) { } Override public void onErrorResponse(VolleyError error) { } } – Deyoung

@ Nayan Srivastava i m not able to understand clearly whether i have to pass that class object from the point from which i m making request or in this method – Deyoung

Answer 3

I had a similar problem detected with LeakCanary where Volley's mListener was referencing my response listener, and my listener was referencing an ImageView, so it could update it with the downloaded image.

I made my response listener an inner class within the activity ..

private class MyVolleyResponseListener <T> implements com.android.volley.Response.Listener <Bitmap> {

        @Override
        public void onResponse(Bitmap bitmap) {
            thumbNailView.setImageBitmap(bitmap);
        }
}

.. and stopped and started the volley request queue inside onDestroy() in the activity ..

requestQueue.stop();
requestQueue.start();

This has fixed the leak.

Carlina

Answer 4

I know I m a bit late to join the party, but few days back this problem did spoil my weekend. In order to figure out, I went on to research a bit which finally got the solution.

The issue lies in the last request object getting leaked in Network Dispatcher & Cache Dispatcher.

@Override
    public void run() {
        if (DEBUG) VolleyLog.v("start new dispatcher");
        Process.setThreadPriority(Process.THREAD_PRIORITY_BACKGROUND);

        // Make a blocking call to initialize the cache.
        mCache.initialize();

        Request<?> request;
        while (true) {
            // release previous request object to avoid leaking request object when mQueue is drained.
            request = null;
            try {
                // Take a request from the queue.
                request = mCacheQueue.take();
            } catch (InterruptedException e) {
                // We may have been interrupted because it was time to quit.
                if (mQuit) {
                    return;
                }
                continue;
            }
            try {
                request.addMarker("cache-queue-take");

                // If the request has been canceled, don't bother dispatching it.
                if (request.isCanceled()) {
                    request.finish("cache-discard-canceled");
                    continue;
                }

                // Attempt to retrieve this item from cache.
                Cache.Entry entry = mCache.get(request.getCacheKey());
                if (entry == null) {
                    request.addMarker("cache-miss");
                    // Cache miss; send off to the network dispatcher.
                    mNetworkQueue.put(request);
                    continue;
                }

                // If it is completely expired, just send it to the network.
                if (entry.isExpired()) {
                    request.addMarker("cache-hit-expired");
                    request.setCacheEntry(entry);
                    mNetworkQueue.put(request);
                    continue;
                }

                // We have a cache hit; parse its data for delivery back to the request.
                request.addMarker("cache-hit");
                Response<?> response = request.parseNetworkResponse(
                        new NetworkResponse(entry.data, entry.responseHeaders));
                request.addMarker("cache-hit-parsed");

                if (!entry.refreshNeeded()) {
                    // Completely unexpired cache hit. Just deliver the response.
                    mDelivery.postResponse(request, response);
                } else {
                    // Soft-expired cache hit. We can deliver the cached response,
                    // but we need to also send the request to the network for
                    // refreshing.
                    request.addMarker("cache-hit-refresh-needed");
                    request.setCacheEntry(entry);

                    // Mark the response as intermediate.
                    response.intermediate = true;

                    // Post the intermediate response back to the user and have
                    // the delivery then forward the request along to the network.
                    final Request<?> finalRequest = request;
                    mDelivery.postResponse(request, response, new Runnable() {
                        @Override
                        public void run() {
                            try {
                                mNetworkQueue.put(finalRequest);
                            } catch (InterruptedException e) {
                                // Not much we can do about this.
                            }
                        }
                    });
                }
            } catch (Exception e) {
                VolleyLog.e(e, "Unhandled exception %s", e.toString());
            }
        }

As you can see a new request object is created before it takes from the queue. This overcomes the problem of memory leak.

P.S: Don't use Volley from the Google repository as it is deprecated and has this bug since then. In order to use Volley, go for this :

https://github.com/mcxiaoke/android-volley

The above repository is free from any memory leaks whatsoever. Ciao.

Vichy

The above repository is also deprecated according to its README, should I still use it ? – Essy

@droidster this one is is actually the repository with the leaking network dispatcher – Essy