From 536c1b741d18395a8aa041de484f3dc46fb57692 Mon Sep 17 00:00:00 2001 From: Jeff Davidson Date: Fri, 10 Nov 2017 14:35:22 -0800 Subject: [PATCH] Workaround memory leak in dispatchers. (#119) * 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 --- consumer-proguard-rules.pro | 9 ++ rules.gradle | 4 + .../com/android/volley/CacheDispatcher.java | 145 +++++++++--------- .../com/android/volley/NetworkDispatcher.java | 106 +++++++------ 4 files changed, 145 insertions(+), 119 deletions(-) create mode 100644 consumer-proguard-rules.pro diff --git a/consumer-proguard-rules.pro b/consumer-proguard-rules.pro new file mode 100644 index 00000000..38d2cf19 --- /dev/null +++ b/consumer-proguard-rules.pro @@ -0,0 +1,9 @@ +# Prevent Proguard from inlining methods that are intentionally extracted to ensure locals have a +# constrained liveness scope by the GC. This is needed to avoid keeping previous request references +# alive for an indeterminate amount of time. See also https://github.com/google/volley/issues/114 +-keepclassmembers,allowshrinking,allowobfuscation class com.android.volley.NetworkDispatcher { + void processRequest(); +} +-keepclassmembers,allowshrinking,allowobfuscation class com.android.volley.CacheDispatcher { + void processRequest(); +} diff --git a/rules.gradle b/rules.gradle index af81ac2b..21df8985 100644 --- a/rules.gradle +++ b/rules.gradle @@ -9,6 +9,10 @@ android { sourceCompatibility JavaVersion.VERSION_1_7 targetCompatibility JavaVersion.VERSION_1_7 } + + defaultConfig { + consumerProguardFiles 'consumer-proguard-rules.pro' + } } // Check if the android plugin version supports unit testing. diff --git a/src/main/java/com/android/volley/CacheDispatcher.java b/src/main/java/com/android/volley/CacheDispatcher.java index b0432f3f..cd3635dd 100644 --- a/src/main/java/com/android/volley/CacheDispatcher.java +++ b/src/main/java/com/android/volley/CacheDispatcher.java @@ -93,82 +93,89 @@ public void run() { while (true) { try { - // Get a request from the cache triage queue, blocking until - // at least one is available. - final Request request = mCacheQueue.take(); - 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; + processRequest(); + } catch (InterruptedException e) { + // We may have been interrupted because it was time to quit. + if (mQuit) { + return; } + } + } + } - // 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. - if (!mWaitingRequestManager.maybeAddToWaitingRequests(request)) { - mNetworkQueue.put(request); - } - continue; - } + // Extracted to its own method to ensure locals have a constrained liveness scope by the GC. + // This is needed to avoid keeping previous request references alive for an indeterminate amount + // of time. Update consumer-proguard-rules.pro when modifying this. See also + // https://github.com/google/volley/issues/114 + private void processRequest() throws InterruptedException { + // Get a request from the cache triage queue, blocking until + // at least one is available. + final Request request = mCacheQueue.take(); + request.addMarker("cache-queue-take"); - // If it is completely expired, just send it to the network. - if (entry.isExpired()) { - request.addMarker("cache-hit-expired"); - request.setCacheEntry(entry); - if (!mWaitingRequestManager.maybeAddToWaitingRequests(request)) { - mNetworkQueue.put(request); - } - continue; - } + // If the request has been canceled, don't bother dispatching it. + if (request.isCanceled()) { + request.finish("cache-discard-canceled"); + return; + } - // 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"); + // 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. + if (!mWaitingRequestManager.maybeAddToWaitingRequests(request)) { + mNetworkQueue.put(request); + } + return; + } - 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; + // If it is completely expired, just send it to the network. + if (entry.isExpired()) { + request.addMarker("cache-hit-expired"); + request.setCacheEntry(entry); + if (!mWaitingRequestManager.maybeAddToWaitingRequests(request)) { + mNetworkQueue.put(request); + } + return; + } - if (!mWaitingRequestManager.maybeAddToWaitingRequests(request)) { - // Post the intermediate response back to the user and have - // the delivery then forward the request along to the network. - mDelivery.postResponse(request, response, new Runnable() { - @Override - public void run() { - try { - mNetworkQueue.put(request); - } catch (InterruptedException e) { - // Restore the interrupted status - Thread.currentThread().interrupt(); - } - } - }); - } else { - // request has been added to list of waiting requests - // to receive the network response from the first request once it returns. - mDelivery.postResponse(request, response); - } - } + // 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"); - } catch (InterruptedException e) { - // We may have been interrupted because it was time to quit. - if (mQuit) { - return; - } + 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; + + if (!mWaitingRequestManager.maybeAddToWaitingRequests(request)) { + // Post the intermediate response back to the user and have + // the delivery then forward the request along to the network. + mDelivery.postResponse(request, response, new Runnable() { + @Override + public void run() { + try { + mNetworkQueue.put(request); + } catch (InterruptedException e) { + // Restore the interrupted status + Thread.currentThread().interrupt(); + } + } + }); + } else { + // request has been added to list of waiting requests + // to receive the network response from the first request once it returns. + mDelivery.postResponse(request, response); } } } diff --git a/src/main/java/com/android/volley/NetworkDispatcher.java b/src/main/java/com/android/volley/NetworkDispatcher.java index 0384429f..2c04ce07 100644 --- a/src/main/java/com/android/volley/NetworkDispatcher.java +++ b/src/main/java/com/android/volley/NetworkDispatcher.java @@ -83,70 +83,76 @@ private void addTrafficStatsTag(Request request) { public void run() { Process.setThreadPriority(Process.THREAD_PRIORITY_BACKGROUND); while (true) { - long startTimeMs = SystemClock.elapsedRealtime(); - Request request; try { - // Take a request from the queue. - request = mQueue.take(); + processRequest(); } catch (InterruptedException e) { // We may have been interrupted because it was time to quit. if (mQuit) { return; } - continue; } + } + } - try { - request.addMarker("network-queue-take"); - - // If the request was cancelled already, do not perform the - // network request. - if (request.isCanceled()) { - request.finish("network-discard-cancelled"); - request.notifyListenerResponseNotUsable(); - continue; - } - - addTrafficStatsTag(request); + // Extracted to its own method to ensure locals have a constrained liveness scope by the GC. + // This is needed to avoid keeping previous request references alive for an indeterminate amount + // of time. Update consumer-proguard-rules.pro when modifying this. See also + // https://github.com/google/volley/issues/114 + private void processRequest() throws InterruptedException { + long startTimeMs = SystemClock.elapsedRealtime(); + // Take a request from the queue. + Request request = mQueue.take(); + + try { + request.addMarker("network-queue-take"); + + // If the request was cancelled already, do not perform the + // network request. + if (request.isCanceled()) { + request.finish("network-discard-cancelled"); + request.notifyListenerResponseNotUsable(); + return; + } - // Perform the network request. - NetworkResponse networkResponse = mNetwork.performRequest(request); - request.addMarker("network-http-complete"); + addTrafficStatsTag(request); - // If the server returned 304 AND we delivered a response already, - // we're done -- don't deliver a second identical response. - if (networkResponse.notModified && request.hasHadResponseDelivered()) { - request.finish("not-modified"); - request.notifyListenerResponseNotUsable(); - continue; - } + // Perform the network request. + NetworkResponse networkResponse = mNetwork.performRequest(request); + request.addMarker("network-http-complete"); - // Parse the response here on the worker thread. - Response response = request.parseNetworkResponse(networkResponse); - request.addMarker("network-parse-complete"); + // If the server returned 304 AND we delivered a response already, + // we're done -- don't deliver a second identical response. + if (networkResponse.notModified && request.hasHadResponseDelivered()) { + request.finish("not-modified"); + request.notifyListenerResponseNotUsable(); + return; + } - // Write to cache if applicable. - // TODO: Only update cache metadata instead of entire record for 304s. - if (request.shouldCache() && response.cacheEntry != null) { - mCache.put(request.getCacheKey(), response.cacheEntry); - request.addMarker("network-cache-written"); - } + // Parse the response here on the worker thread. + Response response = request.parseNetworkResponse(networkResponse); + request.addMarker("network-parse-complete"); - // Post the response back. - request.markDelivered(); - mDelivery.postResponse(request, response); - request.notifyListenerResponseReceived(response); - } catch (VolleyError volleyError) { - volleyError.setNetworkTimeMs(SystemClock.elapsedRealtime() - startTimeMs); - parseAndDeliverNetworkError(request, volleyError); - request.notifyListenerResponseNotUsable(); - } catch (Exception e) { - VolleyLog.e(e, "Unhandled exception %s", e.toString()); - VolleyError volleyError = new VolleyError(e); - volleyError.setNetworkTimeMs(SystemClock.elapsedRealtime() - startTimeMs); - mDelivery.postError(request, volleyError); - request.notifyListenerResponseNotUsable(); + // Write to cache if applicable. + // TODO: Only update cache metadata instead of entire record for 304s. + if (request.shouldCache() && response.cacheEntry != null) { + mCache.put(request.getCacheKey(), response.cacheEntry); + request.addMarker("network-cache-written"); } + + // Post the response back. + request.markDelivered(); + mDelivery.postResponse(request, response); + request.notifyListenerResponseReceived(response); + } catch (VolleyError volleyError) { + volleyError.setNetworkTimeMs(SystemClock.elapsedRealtime() - startTimeMs); + parseAndDeliverNetworkError(request, volleyError); + request.notifyListenerResponseNotUsable(); + } catch (Exception e) { + VolleyLog.e(e, "Unhandled exception %s", e.toString()); + VolleyError volleyError = new VolleyError(e); + volleyError.setNetworkTimeMs(SystemClock.elapsedRealtime() - startTimeMs); + mDelivery.postError(request, volleyError); + request.notifyListenerResponseNotUsable(); } }