From 9f3cb5e4602c7c70f4ee27a2dcdb0463f39f69cc Mon Sep 17 00:00:00 2001 From: Jeff Davidson Date: Wed, 27 Sep 2017 15:51:44 -0700 Subject: [PATCH] Improve request cancelation handling. - Provide stronger thread safety guarantees by locking access to any variable which can be mutated during the processing of a request (i.e. by the CacheDispatcher/NetworkDispatcher threads) and dispatching response callbacks while holding the same lock that is held when setting mCanceled. Unfortunately there's not much we can do about alternative ResponseDelivery mechanisms, which are hopefully rare. - Add a cancel listener interface and use it to prevent RequestFuture from blocking for the complete timeout (or indefinitely) if a request is canceled. - Since listeners are often Android component classes like Activitys, clear them upon cancellation. Unfortunately, this must be done in each Request subclass to work, assuming the subclass follows the standard pattern of holding the response listener as a member. But this enables concerned apps to resolve this leak. Fixes #15 Fixes #85 --- .../com/android/volley/ExecutorDelivery.java | 26 +---- src/main/java/com/android/volley/Request.java | 100 ++++++++++++++++-- .../android/volley/toolbox/ImageRequest.java | 7 +- .../android/volley/toolbox/JsonRequest.java | 7 +- .../android/volley/toolbox/RequestFuture.java | 29 ++++- .../android/volley/toolbox/StringRequest.java | 7 +- .../java/com/android/volley/RequestTest.java | 20 ++++ .../volley/toolbox/RequestFutureTest.java | 27 +++++ 8 files changed, 185 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/android/volley/ExecutorDelivery.java b/src/main/java/com/android/volley/ExecutorDelivery.java index 1babfcd1..8c6e0940 100644 --- a/src/main/java/com/android/volley/ExecutorDelivery.java +++ b/src/main/java/com/android/volley/ExecutorDelivery.java @@ -88,31 +88,7 @@ public ResponseDeliveryRunnable(Request request, Response response, Runnable run @SuppressWarnings("unchecked") @Override public void run() { - // If this request has canceled, finish it and don't deliver. - if (mRequest.isCanceled()) { - mRequest.finish("canceled-at-delivery"); - return; - } - - // Deliver a normal response or error, depending. - if (mResponse.isSuccess()) { - mRequest.deliverResponse(mResponse.result); - } else { - mRequest.deliverError(mResponse.error); - } - - // If this is an intermediate response, add a marker, otherwise we're done - // and the request can be finished. - if (mResponse.intermediate) { - mRequest.addMarker("intermediate-response"); - } else { - mRequest.finish("done"); - } - - // If we have been provided a post-delivery runnable, run it. - if (mRunnable != null) { - mRunnable.run(); - } + mRequest.deliverResponse(mResponse, mRunnable); } } } diff --git a/src/main/java/com/android/volley/Request.java b/src/main/java/com/android/volley/Request.java index a98277eb..c4d52516 100644 --- a/src/main/java/com/android/volley/Request.java +++ b/src/main/java/com/android/volley/Request.java @@ -56,6 +56,12 @@ public interface Method { int PATCH = 7; } + /** Callback interface to notify listeners when a request has been canceled. */ + public interface CancelListener { + /** Called when a request was canceled before the response could be delivered. */ + void onRequestCanceled(); + } + /** * Callback to notify when the network request returns. */ @@ -83,8 +89,11 @@ public interface Method { /** Default tag for {@link TrafficStats}. */ private final int mDefaultTrafficStatsTag; + /** Lock to guard state which can be mutated after a request is added to the queue. */ + private final Object mLock = new Object(); + /** Listener interface for errors. */ - private final Response.ErrorListener mErrorListener; + private Response.ErrorListener mErrorListener; /** Sequence number of this request, used to enforce FIFO ordering. */ private Integer mSequence; @@ -96,9 +105,11 @@ public interface Method { private boolean mShouldCache = true; /** Whether or not this request has been canceled. */ + // GuardedBy(mLock) private boolean mCanceled = false; /** Whether or not a response has been delivered for this request yet. */ + // GuardedBy(mLock) private boolean mResponseDelivered = false; /** Whether the request should be retried in the event of an HTTP 5xx (server) error. */ @@ -118,10 +129,11 @@ public interface Method { private Object mTag; /** Listener that will be notified when a response has been delivered. */ + // GuardedBy(mLock) private NetworkRequestCompleteListener mRequestCompleteListener; - /** Object to guard access to mRequestCompleteListener. */ - private final Object mLock = new Object(); + /** Optional listener for request cancel events. */ + private CancelListener mCancelListener; /** * Creates a new request with the given URL and error listener. Note that @@ -184,6 +196,18 @@ public Response.ErrorListener getErrorListener() { return mErrorListener; } + /** + * @return this request's {@link CancelListener}, if any. + */ + public CancelListener getCancelListener() { + return mCancelListener; + } + + /** Sets a {@link CancelListener} which will be notified when the request is canceled. */ + public void setCancelListener(CancelListener listener) { + mCancelListener = listener; + } + /** * @return A tag for use with {@link TrafficStats#setThreadStatsTag(int)} */ @@ -320,17 +344,81 @@ public Cache.Entry getCacheEntry() { } /** - * Mark this request as canceled. No callback will be delivered. + * Mark this request as canceled. + * + *

No callback will be delivered as long as {@link ExecutorDelivery} is used as the + * {@link ResponseDelivery} and as long as this method is not overridden. There are no + * guarantees otherwise. + * + *

Subclasses should generally override {@link #onCanceled()} instead to perform an action + * when the request is canceled (such as clearing response listeners to avoid leaks). */ public void cancel() { - mCanceled = true; + synchronized (mLock) { + mCanceled = true; + onCanceledInternal(); + } } /** * Returns true if this request has been canceled. */ public boolean isCanceled() { - return mCanceled; + synchronized (mLock) { + return mCanceled; + } + } + + private void onCanceledInternal() { + mErrorListener = null; + if (mCancelListener != null) { + mCancelListener.onRequestCanceled(); + mCancelListener = null; + } + onCanceled(); + } + + /** + * Called when a request is canceled. + * + *

The default implementation does nothing. Subclasses may override this to perform custom + * actions, such as clearing response listeners to avoid leaks. + */ + protected void onCanceled() { + } + + /** Deliver the given response as long as the request isn't canceled. */ + void deliverResponse(Response response, Runnable runnable) { + // To uphold the contract of cancel(), we hold the same lock here around both the check of + // isCanceled() as well as the actual response delivery. This ensures that after a call + // to cancel() returns, we will never deliver the response. + synchronized (mLock) { + // If this request has canceled, finish it and don't deliver. + if (isCanceled()) { + finish("canceled-at-delivery"); + return; + } + + // Deliver a normal response or error, depending. + if (response.isSuccess()) { + deliverResponse(response.result); + } else { + deliverError(response.error); + } + + // If this is an intermediate response, add a marker, otherwise we're done + // and the request can be finished. + if (response.intermediate) { + addMarker("intermediate-response"); + } else { + finish("done"); + } + + // If we have been provided a post-delivery runnable, run it. + if (runnable != null) { + runnable.run(); + } + } } /** diff --git a/src/main/java/com/android/volley/toolbox/ImageRequest.java b/src/main/java/com/android/volley/toolbox/ImageRequest.java index 0f33cd8c..7ef77851 100644 --- a/src/main/java/com/android/volley/toolbox/ImageRequest.java +++ b/src/main/java/com/android/volley/toolbox/ImageRequest.java @@ -42,7 +42,7 @@ public class ImageRequest extends Request { /** Default backoff multiplier for image requests */ public static final float DEFAULT_IMAGE_BACKOFF_MULT = 2f; - private final Response.Listener mListener; + private Response.Listener mListener; private final Config mDecodeConfig; private final int mMaxWidth; private final int mMaxHeight; @@ -221,6 +221,11 @@ protected void deliverResponse(Bitmap response) { } } + @Override + protected void onCanceled() { + mListener = null; + } + /** * Returns the largest power-of-two divisor for use in downscaling a bitmap * that will not result in the scaling past the desired dimensions. diff --git a/src/main/java/com/android/volley/toolbox/JsonRequest.java b/src/main/java/com/android/volley/toolbox/JsonRequest.java index 40877b11..66c5d7f7 100644 --- a/src/main/java/com/android/volley/toolbox/JsonRequest.java +++ b/src/main/java/com/android/volley/toolbox/JsonRequest.java @@ -39,7 +39,7 @@ public abstract class JsonRequest extends Request { private static final String PROTOCOL_CONTENT_TYPE = String.format("application/json; charset=%s", PROTOCOL_CHARSET); - private final Listener mListener; + private Listener mListener; private final String mRequestBody; /** @@ -68,6 +68,11 @@ protected void deliverResponse(T response) { } } + @Override + protected void onCanceled() { + mListener = null; + } + @Override abstract protected Response parseNetworkResponse(NetworkResponse response); diff --git a/src/main/java/com/android/volley/toolbox/RequestFuture.java b/src/main/java/com/android/volley/toolbox/RequestFuture.java index 173c44cc..adf9407f 100644 --- a/src/main/java/com/android/volley/toolbox/RequestFuture.java +++ b/src/main/java/com/android/volley/toolbox/RequestFuture.java @@ -20,6 +20,7 @@ import com.android.volley.Response; import com.android.volley.VolleyError; +import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -35,8 +36,8 @@ * * // If you want to be able to cancel the request: * future.setRequest(requestQueue.add(request)); + * // Also be sure not to call Request.setCancelListener. * - * // Otherwise: * requestQueue.add(request); * * try { @@ -51,9 +52,10 @@ * * @param The type of parsed response this future expects. */ -public class RequestFuture implements Future, Response.Listener, - Response.ErrorListener { +public class RequestFuture implements Future, Response.Listener, Response.ErrorListener, + Request.CancelListener { private Request mRequest; + private boolean mRequestCanceled = false; private boolean mResultReceived = false; private T mResult; private VolleyError mException; @@ -66,6 +68,11 @@ private RequestFuture() {} public void setRequest(Request request) { mRequest = request; + if (mRequest.getCancelListener() != null) { + throw new IllegalArgumentException("Provided request already has a cancel listener"); + } + mRequest.setCancelListener(this); + // Not much we can do if a different cancel listener is set after this point. } @Override @@ -107,6 +114,10 @@ private synchronized T doGet(Long timeoutMs) return mResult; } + if (isCancelled()) { + throw new CancellationException(); + } + if (timeoutMs == null) { wait(0); } else if (timeoutMs > 0) { @@ -121,6 +132,10 @@ private synchronized T doGet(Long timeoutMs) throw new TimeoutException(); } + if (isCancelled()) { + throw new CancellationException(); + } + return mResult; } @@ -129,7 +144,7 @@ public boolean isCancelled() { if (mRequest == null) { return false; } - return mRequest.isCanceled(); + return mRequestCanceled; } @Override @@ -149,5 +164,11 @@ public synchronized void onErrorResponse(VolleyError error) { mException = error; notifyAll(); } + + @Override + public synchronized void onRequestCanceled() { + mRequestCanceled = true; + notifyAll(); + } } diff --git a/src/main/java/com/android/volley/toolbox/StringRequest.java b/src/main/java/com/android/volley/toolbox/StringRequest.java index 05a62f60..0d46772b 100644 --- a/src/main/java/com/android/volley/toolbox/StringRequest.java +++ b/src/main/java/com/android/volley/toolbox/StringRequest.java @@ -28,7 +28,7 @@ * A canned request for retrieving the response body at a given URL as a String. */ public class StringRequest extends Request { - private final Listener mListener; + private Listener mListener; /** * Creates a new request with the given method. @@ -62,6 +62,11 @@ protected void deliverResponse(String response) { } } + @Override + protected void onCanceled() { + mListener = null; + } + @Override protected Response parseNetworkResponse(NetworkResponse response) { String parsed; diff --git a/src/test/java/com/android/volley/RequestTest.java b/src/test/java/com/android/volley/RequestTest.java index d5beca5f..84436522 100644 --- a/src/test/java/com/android/volley/RequestTest.java +++ b/src/test/java/com/android/volley/RequestTest.java @@ -21,6 +21,8 @@ import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; +import java.util.concurrent.atomic.AtomicBoolean; + import static org.junit.Assert.*; @RunWith(RobolectricTestRunner.class) @@ -45,6 +47,24 @@ public class RequestTest { assertTrue(immediate.compareTo(high) < 0); } + @Test public void cancelListener() { + TestRequest request = new TestRequest(Priority.NORMAL); + final AtomicBoolean listenerCalled = new AtomicBoolean(false); + request.setCancelListener(new Request.CancelListener() { + @Override + public void onRequestCanceled() { + listenerCalled.set(true); + } + }); + assertFalse(request.isCanceled()); + assertFalse(listenerCalled.get()); + + request.cancel(); + + assertTrue(request.isCanceled()); + assertTrue(listenerCalled.get()); + } + private class TestRequest extends Request { private Priority mPriority = Priority.NORMAL; public TestRequest(Priority priority) { diff --git a/src/test/java/com/android/volley/toolbox/RequestFutureTest.java b/src/test/java/com/android/volley/toolbox/RequestFutureTest.java index c8e23e78..3c62c665 100644 --- a/src/test/java/com/android/volley/toolbox/RequestFutureTest.java +++ b/src/test/java/com/android/volley/toolbox/RequestFutureTest.java @@ -17,10 +17,15 @@ package com.android.volley.toolbox; import com.android.volley.Request; +import com.android.volley.mock.MockRequest; + import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; +import java.util.concurrent.CancellationException; +import java.util.concurrent.TimeUnit; + import static org.junit.Assert.assertNotNull; @RunWith(RobolectricTestRunner.class) @@ -32,4 +37,26 @@ public void publicMethods() throws Exception { assertNotNull(RequestFuture.class.getMethod("newFuture")); assertNotNull(RequestFuture.class.getMethod("setRequest", Request.class)); } + + @Test(expected = CancellationException.class) + public void cancelRequest() throws Exception { + MockRequest request = new MockRequest(); + RequestFuture future = RequestFuture.newFuture(); + future.setRequest(request); + + request.cancel(); + + future.get(5, TimeUnit.SECONDS); + } + + @Test(expected = CancellationException.class) + public void cancelFuture() throws Exception { + MockRequest request = new MockRequest(); + RequestFuture future = RequestFuture.newFuture(); + future.setRequest(request); + + future.cancel(true); + + future.get(5, TimeUnit.SECONDS); + } }