unableto clear mErrorListener causing memory leaked. about volley HOT 16 CLOSED

Not all leaks : if the listeners are anonymous inside a method, their references are not kept when the method returns but the leak still occurs because the NetworkDispatcher holds it.

from volley.

Yes, I agree that it probably makes sense to clear mErrorListener when a request is canceled. Some notes in case someone wants to try and put a PR together for this:

• While this might trigger some leak detection tools, I don't think the leak is terribly problematic, because it should be transient (lifetime of the HTTP request) rather than permanent (lifetime of the app). Once the request actually finishes, nothing in Volley's internals will be referencing it, which means that it can be GC'd along with any listeners it is referencing. If your timeouts are short, I think you're fine.

• cancel() is a bit broken right now in terms of thread safety. If you only call it from the UI thread, you're fine, but there's no documentation or enforcement that says you have to do that. So we need to be careful - if you call cancel from another thread, deliverError might still be called. I'd like a saner story around cancel() thread safety. (When I last looked at this, my plan was to have cancel() set mCanceled = true only if called from the main thread, and queue a runnable on the main thread to do so otherwise).

• I'm guessing most (all?) Request subclasses use a similar pattern of allowing you to provide a (successful) response listener. It's unfortunate that subclasses have to manage their own response listeners... I wish that Request had taken the Response.Listener and dispatched it itself - guessing there's a good reason for this that I don't have the full context for, maybe generic type related - but it'd be hard to change that aspect of the API now. Best we can do is clean this up in all of the toolbox Request classes and leave it to library users to fix this in their own requests if they care (and as noted above, I don't really think this is a huge deal unless you have very long request timeouts).

from volley.

Hi, just realize it had solved in another volley fork 2 years ago,

This is the issue. I facing exactly same issue, which causing activity leaked by NetworkDispatcher.

The modified file is NetworkDispatcher's run()

while (true) {
            long startTimeMs = SystemClock.elapsedRealtime();
            // release previous request object to avoid leaking request object when mQueue is drained.
            //set request to null
            request = null;
            try {

and Request class, look for onFinish() new method in Request class, it clear error listener after finish() called.

from volley.

why "request = null"?

from volley.

// release previous request object to avoid leaking request object when mQueue is drained.
request = null;

This is the fixed from the fork, issue 93.

from volley.

origin code :

while (true) {
            long startTimeMs = SystemClock.elapsedRealtime();
            Request<?> request;
            try {
                // Take a request from the queue.
                request = mQueue.take();

modified ：

while (true) {
            long startTimeMs = SystemClock.elapsedRealtime();
            // release previous request object to avoid leaking request object when mQueue is drained.
            //set request to null
            request = null;
            try {

what is the difference?

from volley.

Hi, you need to look the full source code.

When the code run some where

 try {
    request = mQueue.take();

It will block when the queue is drained, some how your request may still hold a request reference...

So I guess manually set it to null will clear the reference before the code block at queue.take(), and you need to do this for subclass of your request object

    @Override
    protected void onFinish()
    {
        super.onFinish();
        listener = null; // set listener to null
    }

I use this method, and listener leaked solved. (you can use Leak canary for checked, or Android studio memory dump)

from volley.

i got this.
just as my previous question.I found google/android-volley is different from mcxiaoke/android-volley。

google version:

while (true) {
            long startTimeMs = SystemClock.elapsedRealtime();
            Request<?> request;//**here is different**
            try {
                // Take a request from the queue.
                request = mQueue.take();

mcxiaoke version:

Request<?> request;
while (true) {
            long startTimeMs = SystemClock.elapsedRealtime();
            // release previous request object to avoid leaking request object when mQueue is drained.
            //set request to null
            request = null;
            try {
                 request = mQueue.take();

they both reallocate 'Request<?> request' variable before 'queue.take()',even when the queue is drained.

In addition,if a request 'blocking' too long,leak still exists.

from volley.

from volley.

@xesam I think this a Dalvik issue, read A small leak will sink a great ship for context.

from volley.

Manually set response handlers to null when request is done. It's how we do that now.

That clears all memory leaks.

from volley.

I made a small helper app to reproduce the leak here as reported by LeakCanary / Android Studio. It has a button which issues a request, and then after some random short delay cancels the request and recreates the activity. However, I only succeeded in reproducing the leak on an Oreo device; on KitKat I couldn't get either tool to report a leak.

There are two objects in question here:

• A Request object has an ErrorListener, and generally, a Listener; these objects are often Activities or other heavy lifecycle objects. cancel() doesn't clear these references, so a canceled Request which is still referenced elsewhere can leak these objects. If we clear these references in cancel(), it appears to resolve the leak.

• Various Volley internals hold on to Request objects even after they are canceled. This is significantly harder to fix IMO, because these requests are shuttled through various queues throughout the request lifecycle. There are comments above addressing one or two of these references like in NetworkDispatcher, or CacheDispatcher, but I do not think it is feasible to stamp these all out. However - I think it should be significantly rarer that a Request is associated with a lifecycle component other than the response listeners, or at least can be designed around. So as long as we're clearing the Listener references within the Request, I think the impact of holding a reference to the Request should be minimal and won't trigger LeakCanary or other Activity leak detection tools.

So, my proposal for this is to clear the ErrorListener in Request and clear the Listener in the various Request classes in .toolbox on cancel(), while updating the cancel() documentation to note that Request subclasses should clear their listeners there to prevent leaks. This does appear to resolve the activity leak in my test app, which I think is sufficient here.

I'm not sure I understand @natinusala's comment regarding the listeners being anonymous. NetworkDispatcher does not hold any reference to Listeners, only Requests. So as long as the Request no longer references the listener, I don't see how there would be a leak.

Will try to send out a fix for the above soon. If someone believes that this is insufficient to resolve activity leaks or that there's a hole in the above, please let me know. I'd especially appreciate a sample app that exhibits the issue and details on which platform(s) are affected since at least some of the above discussion linked to issues on older versions of Android.

from volley.

The memory leak can be circumvented on the client side using a WeakReference, right?

This is generally advisable when the lifetime of the listener's destination may be less than the lifetime of the object holding the listener. There's no telling how long requests may stay in the request queue, and clients are probably not good about canceling all their outstanding requests?

from volley.

from volley.

It can't be fixed without some Volley change as Request itself holds a private final reference to ErrorListener. An app can't set it to null without modifying Volley source directly. My proposed fix sets it to null and provides an appropriate hook for custom requests to do the same for their completion listeners.

Putting aside questions about which approach is cleaner/safer, I don't think weak references are viable because it could be reasonable for a client to be firing and forgetting requests. While it's common that listeners are activities that thus should be dropped when the activity goes away, this is not always the case.

from volley.

OK - this is now resolved in all of the Request subclasses in the .toolbox package, and to the extent possible in Request itself.

If you are concerned about resolving this in your own Request subclasses, you'll need to override cancel() and release your Listener reference yourself. You can look at the requests in .toolbox package (i.e. StringRequest) for one way to do this. Be sure to do this in a thread-safe manner if you may be calling cancel() from a separate thread as the ResponseDelivery you are using (typically ExecutorDelivery, which means typically the main thread).

from volley.