Cache/NetworkDispatcher leaking finished requests #114
Comments
Since this release introduces new APIs to support migrating off Apache HTTP and handling multiple headers, the minor version should be bumped. These changes are all backwards compatible so there is no need to bump the major version.
|
Please refer mcxiaoke/android-volley@c111d14 , onFinish() should be there so that any class extending Request can clear api listeners. |
|
At a glance, I can't understand why this should be necessary. After Request#finish, the Request object goes out of scope - nothing inside Volley's internals should be holding onto it. That means it should be eligible for GC, as should all of its children, including any listeners. Do you have more information about the leak you are seeing? In particular, do you have a sample app that reproduces it? Are you using LeakCanary and can provide the leak information? What device and Android version are you using to test with, and does it reproduce on other versions? |
|
Hi Team,
Leak is shown by leak canary. In testvolley.dn.java.testvolley:1.0:1.
|
|
Thank you for all of the detailed information and especially the sample app! I can at least reproduce the leak now, although I'm not yet sure why it's happening. I think this came up previously in #19 and was closed as we didn't have needed details. While it was definitely necessary to have Requests clear their listeners on cancel, because Volley internals are still holding onto the Request object while the request is being dispatched, it should not be necessary for Requests to have to do anything after the request is finished. The bug would be that something in Volley is holding a reference to a finished Request as it shouldn't be doing so. That being said, I can't make sense of why CacheDispatcher/NetworkDispatcher are seen as holding references to the finished request. In the heap dumps / LeakCanary output it just shows up as "Java local" without a variable name. But I don't see what local variable is holding onto the request. |
|
I don't see any indication that the ApiRequest instance is finished. Regarding the reported leak of ApiActivity, either:
Assuming RequestQueue.cancelAll() is correct, one way to avoid this false alarm would be to move cancelAll() from ApiActivity's onDestroy method to either onStop or onPause, perhaps preceded by a call to isFinishing(). I also notice that the anonymous listeners class holds a strong reference to ApiActivity. For robustness, I recommend using a static nested class with a weak reference to the target Activity. This might be enough to assuage LeakCanary. Looking at the instances reported by LeakCanary, I think the following two objects are, respectively, the current instance of ApiActivity and its userApiListeners instance: The interesting instances where the leak occurs are: Observations:
|
|
I don't see any indication that the ApiRequest instance is finished.
Regarding the reported leak of ApiActivity, either: There is a bug in RequestQueue.cancelAll() -or-
LeakCanary is fooled by a delayed call to Activity.onDestroy()
Even then cache dispatcher is referencing request listner. |
|
@Dayanand515 thanks for correcting my analysis. |
|
@joebowbeer what do you recommend to fix this leak. |
|
@Dayanand515 The response for the ApiRequest that is retained by CacheDispatcher has not yet been delivered: Are you sure this request is finished? |
|
@joebowbeer markDelivered looks like its only called for successful responses. This sample app gets an error response by intentionally hitting a bogus URL. @Dayanand515 we need to figure out why something is holding a reference to the Request inside Volley. Requiring every Request subclass to clear the listener when a request is finished is not ideal at all (we had to do it to cover the cancel case, but it shouldn't be necessary here), and still means we're leaking the Request object even if the listeners aren't leaked. The CL you linked covers the error listener, but presumably in most cases the response listener is also affected and that is out of the scope of Request itself. Need to look at this more, likely by trying to strip down the code to the minimum possible reproduceable case. It's possible this is a Volley bug... but it's also possible this is something wrong at a lower level (which might or might not be something we can work around). |
|
What I'm observing is very strange. I can not reproduce the issue if I comment out the call to mCurrentRequests.remove(request) in RequestQueue#finish. I can't possibly see how that would be causing a leak as it's supposed to be removing a reference to the request. But as far as I can tell, it's 100% reliable. |
|
@jpd236 that would suggest a deadlock or long wait was needed to acquire the mCurrentRequests lock? Without a body the lock acquisition may be elided. |
|
This anonymous inner Runnable in CacheDispatcher is a little fishy: It references the final |
I think the way forward here will be to try to distill this into a minimal repro case. |
|
OK, I have a fairly minimal repro case which is attached here. This isolates this from all of Volley's internals and any actual network activity / switches between activities. Still investigating. |
Android has a long-standing known issue where local variables aren't explicitly cleared even when they go out of scope, which can cause their contents to leak. Clearing the variable explicitly is not a sufficient fix because the clear is optimized out as it's not observable. The best we can do is extract the contents of the loops in question to helper methods and hope that they are complex enough that code optimizers like Proguard won't inline them. Should contain no functional changes otherwise. Verified against provided sample app. Fixes google#114
|
OK. This does indeed appear to have been the same as #19. Contrary to the linked blog post there, this issue was not actually fully resolved and has resurfaced in recent versions of the platform, even with ART. The workaround from the blog post of posting an empty item to the queue every second is not ideal as that would churn threads that should otherwise be idle. Instead we can try the tack of extracting the body of the loop to a helper method. It's not a 100% guarantee since Proguard could theoretically optimize it, but in practice it's probably good enough and is likely the best we can do. Here's an example of that workaround in practice: https://android.googlesource.com/platform/frameworks/support/+/cd07a0cfd9c9501a03c574d2d48df51c82b73e33 PR coming shortly. |
Android has a long-standing known issue where local variables aren't explicitly cleared even when they go out of scope, which can cause their contents to leak. Clearing the variable explicitly is not a sufficient fix because the clear is optimized out as it's not observable. Extracting the loop contents to a helper function is likely to work but risks being inlined by a code optimizer. So we poll() once (which is non-blocking) before waiting to ensure that the previous request is always cleared expediently. Should contain no functional changes otherwise. Verified against provided sample app. Fixes google#114
Android has a long-standing known issue where local variables aren't explicitly cleared even when they go out of scope, which can cause their contents to leak. Since BlockingQueue#take() blocks forever until a new item is ready, this means the last request will remain in memory until a new request pushes it out. Extracting a helper method is a workaround for this - see, for example, the following CL in the Android support lib: https://android.googlesource.com/platform/frameworks/support/+/cd07a0cfd9c9501a03c574d2d48df51c82b73e33 The following other solutions were attempted but were not sufficient: - Clear the variable prior to take() - optimized out because the write is not observable, so it has no impact on the bytecode. - Call poll() prior to take() - for some reason, this doesn't work when proguard optimization is on. With code optimization, there's no guarantee that this will work. However, it appears to be the best we can do and follows precedent / advice from the ART team. Should contain no functional changes otherwise as this is just extracting code to a helper method, and thus should be safe for 1.1.0. Verified against provided sample app. Fixes google#114
* Workaround memory leak in dispatchers. Android has a long-standing known issue where local variables aren't explicitly cleared even when they go out of scope, which can cause their contents to leak. Since BlockingQueue#take() blocks forever until a new item is ready, this means the last request will remain in memory until a new request pushes it out. Extracting a helper method is a workaround for this - see, for example, the following CL in the Android support lib: https://android.googlesource.com/platform/frameworks/support/+/cd07a0cfd9c9501a03c574d2d48df51c82b73e33 The following other solutions were attempted but were not sufficient: - Clear the variable prior to take() - optimized out because the write is not observable, so it has no impact on the bytecode. - Call poll() prior to take() - for some reason, this doesn't work when proguard optimization is on. With code optimization, there's no guarantee that this will work, though we now provide a Proguard config that should prevent inlining. However, it appears to be the best we can do and follows precedent / advice from the ART team. Should contain no functional changes otherwise as this is just extracting code to a helper method, and thus should be safe for 1.1.0. Verified against provided sample app. Fixes #114
|
@Dayanand515 - could you try compiling against the latest 1.1.0-SNAPSHOT from the https://oss.jfrog.org/artifactory/oss-snapshot-local/ repository to verify that it resolves the issue in your full app? |
|
@jpd236 / @joebowbeer 1.1.0-SNAPSHOT from oss-snapshot-local works as expected, doesn't reproduce any memory leaks. |
|
I have also tested and seems to be fixed. Thanks |
Hi Team,
com.android.volley.Request#finish is not clearing mErrorListener, not its calling onFinish as is the case with https://github.com/mcxiaoke/android-volley/blob/master/src/main/java/com/android/volley/Request.java finish.
due to which error listener is not getting cleared and memory leak is there.
The text was updated successfully, but these errors were encountered: