Leaking last request in CacheDispatcher

[tidbeck]

Android Studio reported a leaked Activity that I tracked down to CacheDispatcher keeping a reference to a request:

The issue was fixed in now deprecated mcxiaoke/android-volley repository:
leaking the last request object in CacheDispatcher

And the reson for the leak is explained in:
A small leak will sink a great ship

[jpd236]

Thanks; it seems legitimate. I'm not completely a fan of the above fix (mostly because it adds a needless catch-all-exception block). AFAICT this should be very simple to fix: just add a request = null before the request = mCacheQueue.take() as the latter is a blocking call.

Pull requests welcome :)

[jpd236]

Looking more closely at the screenshot, it seems to indicate that what's holding onto the activity isn't the local variable in dispatcher loop (or at least, isn't just that), but is mCacheQueue, the queue itself. queue is a member variable of PriorityBlockingQueue which contains the contents.

If that's the leak, then I don't see how this kind of fix would resolve it. Are you testing on a device which uses Dalvik or one which uses ART? If it's the latter, then none of the analysis from that blog post should apply as it states that they never saw the issue with ART (and the stack trace from the screenshot would seem to bear that out).

It seems like a more fundamental issue to me; if you're using a heavy object (i.e. Activity) as your response listener, and that object goes away while the request is in flight, then there are a number of Volley internals like this one that will retain a reference to the request, even if you're a well-behaved client that calls cancel() on the request when that heavy object is destroyed. See also #15 (comment).

I think this is going to be a more involved change if we want to fix it. We could look into using WeakReference anywhere that we hold onto an object that might be/hold a Fragment/Activity.

In your particular case it seems like you've set the request tag (mTag) to a fragment instance; have you tried more cleanly separating your Request objects from any Fragments/Activities and seeing if that addresses your particular leak?

[tidbeck]

I think you are correct. The problem I see is related to the queue it self. I found this issue PriorityBlockingQueue keeps hard reference to last removed element which has some example code for printing the queue after the last element has be taken. On a 4.2.2 device (Galaxy core), the last element does not seem to be nulled out, but it is on later versions of Android (tested 6.0 and 7.0). I'm not sure how to confirm that this bug exists in Android source code it self.

[tidbeck]

Will close this, as I'm not able to provider more information or a solution at this time.

[jpd236]

FYI, we got a similar report in #114 with a sample app, and it seems like newer versions of Android have reintroduced the issue that was previously resolved with ART.

In summary:

• mCacheQueue holding a reference to a request is expected while a request is in flight, and unfortunately it's not feasible to flush all references from Volley's internals on cancel(). However, if this leak is concerning, Volley 1.1.0 (pending final release) mitigates this by clearing listeners in cancel(), though you'll need to do the same in your own Request subclasses to fully mitigate the issue. So even if the Request is held longer than it needs to be, it won't hold any child references to Listeners (which are often Activities) once it has been canceled.

• The "Java local" holding a reference is unexpected once the request is actually complete. This is the issue cited above in the linked blog post and should be worked around in Volley 1.1.0 without needing changes to apps.