Fix leak of callbacks on canceled requests.

We clear mErrorListener on cancel(), and mListener in all Request
subclasses in the toolbox package, and synchronize all reads/writes of
these fields. This ensures that we don't hold a reference to the
listeners after a request is canceled. Custom subclasses of Request
will need to do the same if they are concerned about this leak (which
only lasts as long as the network request takes, in the worst case).

Also fix some lingering thread-safety issues for any variable in
Request which can be mutated during the processing of a request and
which is read from multiple threads.

This provides a somewhat stronger guarantee around callback invocation
after cancel than we previously had (although a weaker one than we
incorrectly advertised) - after calling cancel(), we won't deliver the
callback as long as either cancel() was called on the same thread as
the ResponseDelivery being used or if the Request subclass handles
clearing its listener on cancel() correctly.

Fixes #15

src/main/java/com/android/volley/ExecutorDelivery.java

@@ -88,6 +88,13 @@ public ResponseDeliveryRunnable(Request request, Response response, Runnable run
         @SuppressWarnings("unchecked")
         @Override
         public void run() {
+            // NOTE: If cancel() is called off the thread that we're currently running in (by
+            // default, the main thread), we cannot guarantee that deliverResponse()/deliverError()
+            // won't be called, since it may be canceled after we check isCanceled() but before we
+            // deliver the response. Apps concerned about this guarantee must either call cancel()
+            // from the same thread or implement their own guarantee about not invoking their
+            // listener after cancel() has been called.
+
             // If this request has canceled, finish it and don't deliver.
             if (mRequest.isCanceled()) {
                 mRequest.finish("canceled-at-delivery");

src/main/java/com/android/volley/Request.java

@@ -83,8 +83,12 @@ 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;
+    // @GuardedBy("mLock")
+    private Response.ErrorListener mErrorListener;
 
     /** Sequence number of this request, used to enforce FIFO ordering. */
     private Integer mSequence;
@@ -96,9 +100,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,11 +124,9 @@ 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();
-
     /**
      * Creates a new request with the given URL and error listener.  Note that
      * the normal response listener is not provided here as delivery of responses
@@ -320,17 +324,34 @@ 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 either:
+     * <ul>
+     *     <li>This method is called on the same thread as the {@link ResponseDelivery} is running
+     *     on. By default, this is the main thread.
+     *     <li>The request subclass being used overrides cancel() and ensures that it does not
+     *     invoke the listener in {@link #deliverResponse} after cancel() has been called in a
+     *     thread-safe manner.
+     * </ul>
+     *
+     * <p>There are no guarantees if both of these conditions aren't met.
      */
+    // @CallSuper
     public void cancel() {
-        mCanceled = true;
+        synchronized (mLock) {
+            mCanceled = true;
+            mErrorListener = null;
+        }
     }
 
     /**
      * Returns true if this request has been canceled.
      */
     public boolean isCanceled() {
-        return mCanceled;
+        synchronized (mLock) {
+            return mCanceled;
+        }
     }
 
     /**
@@ -549,14 +570,18 @@ public RetryPolicy getRetryPolicy() {
      * later in the request's lifetime for suppressing identical responses.
      */
     public void markDelivered() {
-        mResponseDelivered = true;
+        synchronized (mLock) {
+            mResponseDelivered = true;
+        }
     }
 
     /**
      * Returns true if this request has had a response delivered for it.
      */
     public boolean hasHadResponseDelivered() {
-        return mResponseDelivered;
+        synchronized (mLock) {
+            return mResponseDelivered;
+        }
     }
 
     /**
@@ -597,8 +622,12 @@ protected VolleyError parseNetworkError(VolleyError volleyError) {
      * @param error Error details
      */
     public void deliverError(VolleyError error) {
-        if (mErrorListener != null) {
-            mErrorListener.onErrorResponse(error);
+        Response.ErrorListener listener;
+        synchronized (mLock) {
+            listener = mErrorListener;
+        }
+        if (listener != null) {
+            listener.onErrorResponse(error);
         }
     }
 
@@ -619,10 +648,12 @@ public void deliverError(VolleyError error) {
      * @param response received from the network
      */
     /* package */ void notifyListenerResponseReceived(Response<?> response) {
+        NetworkRequestCompleteListener listener;
         synchronized (mLock) {
-            if (mRequestCompleteListener != null) {
-               mRequestCompleteListener.onResponseReceived(this, response);
-            }
+            listener = mRequestCompleteListener;
+        }
+        if (listener != null) {
+            listener.onResponseReceived(this, response);
         }
     }
 
@@ -631,10 +662,12 @@ public void deliverError(VolleyError error) {
      * a response which can be used for other, waiting requests.
      */
     /* package */ void notifyListenerResponseNotUsable() {
+        NetworkRequestCompleteListener listener;
         synchronized (mLock) {
-            if (mRequestCompleteListener != null) {
-                mRequestCompleteListener.onNoUsableResponseReceived(this);
-            }
+            listener = mRequestCompleteListener;
+        }
+        if (listener != null) {
+            listener.onNoUsableResponseReceived(this);
         }
     }
 

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

@@ -42,7 +42,11 @@ 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;
+    /** Lock to guard mListener as it is cleared on cancel() and read on delivery. */
+    private final Object mLock = new Object();
+
+    // @GuardedBy("mLock")
+    private Response.Listener<Bitmap> mListener;
     private final Config mDecodeConfig;
     private final int mMaxWidth;
     private final int mMaxHeight;
@@ -214,10 +218,22 @@ private Response<Bitmap> doParse(NetworkResponse response) {
         }
     }
 
+    @Override
+    public void cancel() {
+        super.cancel();
+        synchronized (mLock) {
+            mListener = null;
+        }
+    }
+
     @Override
     protected void deliverResponse(Bitmap response) {
-        if (mListener != null) {
-            mListener.onResponse(response);
+        Response.Listener<Bitmap> listener;
+        synchronized (mLock) {
+            listener = mListener;
+        }
+        if (listener != null) {
+            listener.onResponse(response);
         }
     }
 

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

@@ -39,7 +39,11 @@ 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;
+    /** Lock to guard mListener as it is cleared on cancel() and read on delivery. */
+    private final Object mLock = new Object();
+
+    // @GuardedBy("mLock")
+    private Listener<T> mListener;
     private final String mRequestBody;
 
     /**
@@ -61,10 +65,22 @@ public JsonRequest(int method, String url, String requestBody, Listener<T> liste
         mRequestBody = requestBody;
     }
 
+    @Override
+    public void cancel() {
+        super.cancel();
+        synchronized (mLock) {
+            mListener = null;
+        }
+    }
+
     @Override
     protected void deliverResponse(T response) {
-        if (mListener != null) {
-            mListener.onResponse(response);
+        Response.Listener<T> listener;
+        synchronized (mLock) {
+            listener = mListener;
+        }
+        if (listener != null) {
+            listener.onResponse(response);
         }
     }
 

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

@@ -28,7 +28,12 @@
  * 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;
+
+    /** Lock to guard mListener as it is cleared on cancel() and read on delivery. */
+    private final Object mLock = new Object();
+
+    // @GuardedBy("mLock")
+    private Listener<String> mListener;
 
     /**
      * Creates a new request with the given method.
@@ -55,10 +60,22 @@ public StringRequest(String url, Listener<String> listener, ErrorListener errorL
         this(Method.GET, url, listener, errorListener);
     }
 
+    @Override
+    public void cancel() {
+        super.cancel();
+        synchronized (mLock) {
+            mListener = null;
+        }
+    }
+
     @Override
     protected void deliverResponse(String response) {
-        if (mListener != null) {
-            mListener.onResponse(response);
+        Response.Listener<String> listener;
+        synchronized (mLock) {
+            listener = mListener;
+        }
+        if (listener != null) {
+            listener.onResponse(response);
         }
     }