diff options
author | Anonymous <no-reply@google.com> | 2017-10-09 10:53:24 -0700 |
---|---|---|
committer | Jeff Davidson <jpd@google.com> | 2017-10-11 12:28:08 -0700 |
commit | 82e4a43fe78d4f665985b4e4726ff2baee449fe8 (patch) | |
tree | 19e615b27d9cb80945f86cadc0a7ccca09cc1dee /src/main | |
parent | 2021ca6a6c3fa80646220cb97746f0b1cd3ae103 (diff) | |
download | volley-82e4a43fe78d4f665985b4e4726ff2baee449fe8.tar.gz |
Import of Volley from GitHub to AOSP.
- 95f64de9bfa2ee44d6cf93a6dc4b980e5ec56f6a Fix leak of callbacks on canceled requests. by Jeff Davidson <jpd236@cornell.edu>
GitOrigin-RevId: 95f64de9bfa2ee44d6cf93a6dc4b980e5ec56f6a
Change-Id: I9537a9937f9c0e7078dfa8f8fdfcd8deb36417a7
Diffstat (limited to 'src/main')
5 files changed, 115 insertions, 26 deletions
diff --git a/src/main/java/com/android/volley/ExecutorDelivery.java b/src/main/java/com/android/volley/ExecutorDelivery.java index 1babfcd..f36fb75 100644 --- a/src/main/java/com/android/volley/ExecutorDelivery.java +++ b/src/main/java/com/android/volley/ExecutorDelivery.java @@ -88,6 +88,13 @@ public class ExecutorDelivery implements ResponseDelivery { @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"); diff --git a/src/main/java/com/android/volley/Request.java b/src/main/java/com/android/volley/Request.java index a98277e..358a327 100644 --- a/src/main/java/com/android/volley/Request.java +++ b/src/main/java/com/android/volley/Request.java @@ -83,8 +83,12 @@ public abstract class Request<T> implements Comparable<Request<T>> { /** 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 abstract class Request<T> implements Comparable<Request<T>> { 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 abstract class Request<T> implements Comparable<Request<T>> { 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 abstract class Request<T> implements Comparable<Request<T>> { } /** - * 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 abstract class Request<T> implements Comparable<Request<T>> { * 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 @@ public abstract class Request<T> implements Comparable<Request<T>> { * @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 abstract class Request<T> implements Comparable<Request<T>> { * @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 abstract class Request<T> implements Comparable<Request<T>> { * 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); } } diff --git a/src/main/java/com/android/volley/toolbox/ImageRequest.java b/src/main/java/com/android/volley/toolbox/ImageRequest.java index 0f33cd8..1db7e35 100644 --- a/src/main/java/com/android/volley/toolbox/ImageRequest.java +++ b/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; @@ -215,9 +219,21 @@ public class ImageRequest extends Request<Bitmap> { } @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); } } diff --git a/src/main/java/com/android/volley/toolbox/JsonRequest.java b/src/main/java/com/android/volley/toolbox/JsonRequest.java index 40877b1..f291076 100644 --- a/src/main/java/com/android/volley/toolbox/JsonRequest.java +++ b/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; /** @@ -62,9 +66,21 @@ public abstract class JsonRequest<T> extends Request<T> { } @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); } } diff --git a/src/main/java/com/android/volley/toolbox/StringRequest.java b/src/main/java/com/android/volley/toolbox/StringRequest.java index 05a62f6..9deba1d 100644 --- a/src/main/java/com/android/volley/toolbox/StringRequest.java +++ b/src/main/java/com/android/volley/toolbox/StringRequest.java @@ -28,7 +28,12 @@ import java.io.UnsupportedEncodingException; * 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. @@ -56,9 +61,21 @@ public class StringRequest extends Request<String> { } @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); } } |