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 google#15
Fixes google#85

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);
        }
     }
 }

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.
+     *
+     * <p>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.
+     *
+     * <p>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.
+     *
+     * <p>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<T> 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();
+            }
+        }
     }
 
     /**

src/main/java/com/android/volley/toolbox/ImageRequest.java

@@ -42,7 +42,7 @@ public class ImageRequest extends Request<Bitmap> {
     /** Default backoff multiplier for image requests */
     public static final float DEFAULT_IMAGE_BACKOFF_MULT = 2f;
 
-    private final Response.Listener<Bitmap> mListener;
+    private Response.Listener<Bitmap> 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.

src/main/java/com/android/volley/toolbox/JsonRequest.java

@@ -39,7 +39,7 @@ public abstract class JsonRequest<T> extends Request<T> {
     private static final String PROTOCOL_CONTENT_TYPE =
         String.format("application/json; charset=%s", PROTOCOL_CHARSET);
 
-    private final Listener<T> mListener;
+    private Listener<T> mListener;
     private final String mRequestBody;
 
     /**
@@ -68,6 +68,11 @@ protected void deliverResponse(T response) {
         }
     }
 
+    @Override
+    protected void onCanceled() {
+        mListener = null;
+    }
+
     @Override
     abstract protected Response<T> parseNetworkResponse(NetworkResponse response);
 

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 <T> The type of parsed response this future expects.
  */
-public class RequestFuture<T> implements Future<T>, Response.Listener<T>,
-       Response.ErrorListener {
+public class RequestFuture<T> implements Future<T>, Response.Listener<T>, 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();
+    }
 }
 

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<String> {
-    private final Listener<String> mListener;
+    private Listener<String> 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<String> parseNetworkResponse(NetworkResponse response) {
         String parsed;

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<Object> {
         private Priority mPriority = Priority.NORMAL;
         public TestRequest(Priority priority) {