Improve request cancelation handling.

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) {