Skip to content

Commit

Permalink
Fix concurrent modification errors in ImageContainer.
Browse files Browse the repository at this point in the history
ImageContainer#cancelRequest modifies data structures that are
mutated without a lock on the main thread. Its parent class,
ImageLoader, is documented as requiring that all access happen
on the main thread, so strictly enforce that cancelRequest be
called from the main thread as well. Outside of cancelRequest,
the only uncontrolled caller is NetworkImageView#setImageUrl,
which we also document/enforce as needing to be called from the
main thread.

Add @mainthread annotations and Javadoc clarifications as
appropriate. Clarify that behavior on custom ResponseDeliveries
is undefined.

Fixes google#132
  • Loading branch information
jpd236 committed May 5, 2018
1 parent 62c1901 commit ecb42b9
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 9 deletions.
27 changes: 18 additions & 9 deletions src/main/java/com/android/volley/toolbox/ImageLoader.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@
import android.graphics.Bitmap.Config;
import android.os.Handler;
import android.os.Looper;
import android.support.annotation.MainThread;
import android.widget.ImageView;
import android.widget.ImageView.ScaleType;
import com.android.volley.Request;
import com.android.volley.RequestQueue;
import com.android.volley.Response.ErrorListener;
import com.android.volley.Response.Listener;
import com.android.volley.ResponseDelivery;
import com.android.volley.VolleyError;
import java.util.ArrayList;
import java.util.HashMap;
Expand All @@ -33,8 +35,9 @@
*
* <p>The simple way to use this class is to call {@link ImageLoader#get(String, ImageListener)} and
* to pass in the default image listener provided by {@link ImageLoader#getImageListener(ImageView,
* int, int)}. Note that all function calls to this class must be made from the main thead, and all
* responses will be delivered to the main thread as well.
* int, int)}. Note that all function calls to this class must be made from the main thread, and all
* responses will be delivered to the main thread as well. Custom {@link ResponseDelivery}s which
* don't use the main thread are not supported.
*/
public class ImageLoader {
/** RequestQueue for dispatching ImageRequests onto. */
Expand Down Expand Up @@ -152,14 +155,17 @@ public boolean isCached(String requestUrl, int maxWidth, int maxHeight) {
/**
* Checks if the item is available in the cache.
*
* <p>Must be called from the main thread.
*
* @param requestUrl The url of the remote image
* @param maxWidth The maximum width of the returned image.
* @param maxHeight The maximum height of the returned image.
* @param scaleType The scaleType of the imageView.
* @return True if the item exists in cache, false otherwise.
*/
@MainThread
public boolean isCached(String requestUrl, int maxWidth, int maxHeight, ScaleType scaleType) {
throwIfNotOnMainThread();
Threads.throwIfNotOnMainThread();

String cacheKey = getCacheKey(requestUrl, maxWidth, maxHeight, scaleType);
return mCache.getBitmap(cacheKey) != null;
Expand Down Expand Up @@ -192,6 +198,8 @@ public ImageContainer get(
* returns a bitmap container that contains all of the data relating to the request (as well as
* the default image if the requested image is not available).
*
* <p>Must be called from the main thread.
*
* @param requestUrl The url of the remote image
* @param imageListener The listener to call when the remote image is loaded
* @param maxWidth The maximum width of the returned image.
Expand All @@ -200,6 +208,7 @@ public ImageContainer get(
* @return A container object that contains all of the properties of the request, as well as the
* currently available image (default if remote is not loaded).
*/
@MainThread
public ImageContainer get(
String requestUrl,
ImageListener imageListener,
Expand All @@ -208,7 +217,7 @@ public ImageContainer get(
ScaleType scaleType) {

// only fulfill requests that were initiated from the main thread.
throwIfNotOnMainThread();
Threads.throwIfNotOnMainThread();

final String cacheKey = getCacheKey(requestUrl, maxWidth, maxHeight, scaleType);

Expand Down Expand Up @@ -358,8 +367,13 @@ public ImageContainer(

/**
* Releases interest in the in-flight request (and cancels it if no one else is listening).
*
* <p>Must be called from the main thread.
*/
@MainThread
public void cancelRequest() {
Threads.throwIfNotOnMainThread();

if (mListener == null) {
return;
}
Expand Down Expand Up @@ -499,11 +513,6 @@ public void run() {
}
}

private void throwIfNotOnMainThread() {
if (Looper.myLooper() != Looper.getMainLooper()) {
throw new IllegalStateException("ImageLoader must be invoked from the main thread.");
}
}
/**
* Creates a cache key for use with the L1 cache.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.android.volley.toolbox;

import android.content.Context;
import android.support.annotation.MainThread;
import android.text.TextUtils;
import android.util.AttributeSet;
import android.view.ViewGroup.LayoutParams;
Expand Down Expand Up @@ -59,10 +60,14 @@ public NetworkImageView(Context context, AttributeSet attrs, int defStyle) {
* <p>NOTE: If applicable, {@link NetworkImageView#setDefaultImageResId(int)} and {@link
* NetworkImageView#setErrorImageResId(int)} should be called prior to calling this function.
*
* <p>Must be called from the main thread.
*
* @param url The URL that should be loaded into this ImageView.
* @param imageLoader ImageLoader that will be used to make the request.
*/
@MainThread
public void setImageUrl(String url, ImageLoader imageLoader) {
Threads.throwIfNotOnMainThread();
mUrl = url;
mImageLoader = imageLoader;
// The URL has potentially changed. See if we need to load it.
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/com/android/volley/toolbox/Threads.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.android.volley.toolbox;

import android.os.Looper;

final class Threads {
private Threads() {}

static void throwIfNotOnMainThread() {
if (Looper.myLooper() != Looper.getMainLooper()) {
throw new IllegalStateException("Must be invoked from the main thread.");
}
}
}

0 comments on commit ecb42b9

Please sign in to comment.