diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/test/ConcurrencyHelper.java b/instrumentation/src/androidTest/java/com/bumptech/glide/test/ConcurrencyHelper.java index ed6b13cf8e..754bc973de 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/test/ConcurrencyHelper.java +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/test/ConcurrencyHelper.java @@ -16,6 +16,7 @@ import com.bumptech.glide.request.target.SizeReadyCallback; import com.bumptech.glide.request.target.Target; import com.bumptech.glide.request.transition.Transition; +import com.bumptech.glide.util.Executors; import com.bumptech.glide.util.Preconditions; import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; @@ -197,9 +198,7 @@ public void onDestroy() { public void onResourceReady( @NonNull T resource, @Nullable Transition transition) { target.onResourceReady(resource, transition); - if (!Preconditions.checkNotNull(getRequest()).isRunning()) { - latch.countDown(); - } + checkRequestAndMaybeReleaseLatch(); } @Override @@ -215,9 +214,7 @@ public void onLoadStarted(@Nullable Drawable placeholder) { @Override public void onLoadFailed(@Nullable Drawable errorDrawable) { target.onLoadFailed(errorDrawable); - if (!Preconditions.checkNotNull(getRequest()).isRunning()) { - latch.countDown(); - } + checkRequestAndMaybeReleaseLatch(); } @Override @@ -240,6 +237,22 @@ public void setRequest(@Nullable Request request) { public Request getRequest() { return target.getRequest(); } + + // We can't guarantee the ordering of when this callback is called and when the + // request's state is updated, so it's safer to post the check back to the UI + // thread. + private void checkRequestAndMaybeReleaseLatch() { + Executors.mainThreadExecutor() + .execute( + new Runnable() { + @Override + public void run() { + if (!Preconditions.checkNotNull(getRequest()).isRunning()) { + latch.countDown(); + } + } + }); + } }); return target; } diff --git a/library/src/main/java/com/bumptech/glide/RequestBuilder.java b/library/src/main/java/com/bumptech/glide/RequestBuilder.java index 8bf798ea43..f97a96b867 100644 --- a/library/src/main/java/com/bumptech/glide/RequestBuilder.java +++ b/library/src/main/java/com/bumptech/glide/RequestBuilder.java @@ -849,6 +849,7 @@ private Request buildRequest( BaseRequestOptions requestOptions, Executor callbackExecutor) { return buildRequestRecursive( + /*requestLock=*/ new Object(), target, targetListener, /*parentCoordinator=*/ null, @@ -861,6 +862,7 @@ private Request buildRequest( } private Request buildRequestRecursive( + Object requestLock, Target target, @Nullable RequestListener targetListener, @Nullable RequestCoordinator parentCoordinator, @@ -874,12 +876,13 @@ private Request buildRequestRecursive( // Build the ErrorRequestCoordinator first if necessary so we can update parentCoordinator. ErrorRequestCoordinator errorRequestCoordinator = null; if (errorBuilder != null) { - errorRequestCoordinator = new ErrorRequestCoordinator(parentCoordinator); + errorRequestCoordinator = new ErrorRequestCoordinator(requestLock, parentCoordinator); parentCoordinator = errorRequestCoordinator; } Request mainRequest = buildThumbnailRequestRecursive( + requestLock, target, targetListener, parentCoordinator, @@ -903,6 +906,7 @@ private Request buildRequestRecursive( Request errorRequest = errorBuilder.buildRequestRecursive( + requestLock, target, targetListener, errorRequestCoordinator, @@ -917,6 +921,7 @@ private Request buildRequestRecursive( } private Request buildThumbnailRequestRecursive( + Object requestLock, Target target, RequestListener targetListener, @Nullable RequestCoordinator parentCoordinator, @@ -956,9 +961,11 @@ private Request buildThumbnailRequestRecursive( thumbOverrideHeight = requestOptions.getOverrideHeight(); } - ThumbnailRequestCoordinator coordinator = new ThumbnailRequestCoordinator(parentCoordinator); + ThumbnailRequestCoordinator coordinator = + new ThumbnailRequestCoordinator(requestLock, parentCoordinator); Request fullRequest = obtainRequest( + requestLock, target, targetListener, requestOptions, @@ -972,6 +979,7 @@ private Request buildThumbnailRequestRecursive( // Recursively generate thumbnail requests. Request thumbRequest = thumbnailBuilder.buildRequestRecursive( + requestLock, target, targetListener, coordinator, @@ -986,9 +994,11 @@ private Request buildThumbnailRequestRecursive( return coordinator; } else if (thumbSizeMultiplier != null) { // Base case: thumbnail multiplier generates a thumbnail request, but cannot recurse. - ThumbnailRequestCoordinator coordinator = new ThumbnailRequestCoordinator(parentCoordinator); + ThumbnailRequestCoordinator coordinator = + new ThumbnailRequestCoordinator(requestLock, parentCoordinator); Request fullRequest = obtainRequest( + requestLock, target, targetListener, requestOptions, @@ -1003,6 +1013,7 @@ private Request buildThumbnailRequestRecursive( Request thumbnailRequest = obtainRequest( + requestLock, target, targetListener, thumbnailOptions, @@ -1018,6 +1029,7 @@ private Request buildThumbnailRequestRecursive( } else { // Base case: no thumbnail. return obtainRequest( + requestLock, target, targetListener, requestOptions, @@ -1031,6 +1043,7 @@ private Request buildThumbnailRequestRecursive( } private Request obtainRequest( + Object requestLock, Target target, RequestListener targetListener, BaseRequestOptions requestOptions, @@ -1043,6 +1056,7 @@ private Request obtainRequest( return SingleRequest.obtain( context, glideContext, + requestLock, model, transcodeClass, requestOptions, diff --git a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java index 5cf58ef942..36212e9b9f 100644 --- a/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java +++ b/library/src/main/java/com/bumptech/glide/request/ErrorRequestCoordinator.java @@ -1,5 +1,6 @@ package com.bumptech.glide.request; +import androidx.annotation.GuardedBy; import androidx.annotation.Nullable; /** @@ -8,11 +9,20 @@ */ public final class ErrorRequestCoordinator implements RequestCoordinator, Request { + private final Object requestLock; @Nullable private final RequestCoordinator parent; - private Request primary; - private Request error; - public ErrorRequestCoordinator(@Nullable RequestCoordinator parent) { + private volatile Request primary; + private volatile Request error; + + @GuardedBy("requestLock") + private RequestState primaryState = RequestState.CLEARED; + + @GuardedBy("requestLock") + private RequestState errorState = RequestState.CLEARED; + + public ErrorRequestCoordinator(Object requestLock, @Nullable RequestCoordinator parent) { + this.requestLock = requestLock; this.parent = parent; } @@ -23,56 +33,69 @@ public void setRequests(Request primary, Request error) { @Override public void begin() { - if (!primary.isRunning()) { - primary.begin(); + synchronized (requestLock) { + if (primaryState != RequestState.RUNNING) { + primaryState = RequestState.RUNNING; + primary.begin(); + } } } @Override public void clear() { - primary.clear(); - // Don't check primary.isFailed() here because it will have been reset by the clear call - // immediately before this. - if (error.isRunning()) { - error.clear(); + synchronized (requestLock) { + primaryState = RequestState.CLEARED; + primary.clear(); + // Don't check primary's failed state here because it will have been reset by the clear call + // immediately before this. + if (errorState != RequestState.CLEARED) { + errorState = RequestState.CLEARED; + error.clear(); + } } } @Override public void pause() { - primary.pause(); - error.pause(); + synchronized (requestLock) { + if (primaryState == RequestState.RUNNING) { + primaryState = RequestState.PAUSED; + primary.pause(); + } + if (errorState == RequestState.RUNNING) { + errorState = RequestState.PAUSED; + error.pause(); + } + } } @Override public boolean isRunning() { - return primary.isFailed() ? error.isRunning() : primary.isRunning(); + synchronized (requestLock) { + return primaryState == RequestState.RUNNING || errorState == RequestState.RUNNING; + } } @Override public boolean isComplete() { - return primary.isFailed() ? error.isComplete() : primary.isComplete(); - } - - @Override - public boolean isResourceSet() { - return primary.isFailed() ? error.isResourceSet() : primary.isResourceSet(); + synchronized (requestLock) { + return primaryState == RequestState.SUCCESS || errorState == RequestState.SUCCESS; + } } @Override public boolean isCleared() { - return primary.isFailed() ? error.isCleared() : primary.isCleared(); - } - - @Override - public boolean isFailed() { - return primary.isFailed() && error.isFailed(); + synchronized (requestLock) { + return primaryState == RequestState.CLEARED && errorState == RequestState.CLEARED; + } } @Override public void recycle() { - primary.recycle(); - error.recycle(); + synchronized (requestLock) { + primary.recycle(); + error.recycle(); + } } @Override @@ -86,62 +109,89 @@ public boolean isEquivalentTo(Request o) { @Override public boolean canSetImage(Request request) { - return parentCanSetImage() && isValidRequest(request); + synchronized (requestLock) { + return parentCanSetImage() && isValidRequest(request); + } } + @GuardedBy("requestLock") private boolean parentCanSetImage() { return parent == null || parent.canSetImage(this); } @Override public boolean canNotifyStatusChanged(Request request) { - return parentCanNotifyStatusChanged() && isValidRequest(request); + synchronized (requestLock) { + return parentCanNotifyStatusChanged() && isValidRequest(request); + } } @Override public boolean canNotifyCleared(Request request) { - return parentCanNotifyCleared() && isValidRequest(request); + synchronized (requestLock) { + return parentCanNotifyCleared() && isValidRequest(request); + } } + @GuardedBy("requestLock") private boolean parentCanNotifyCleared() { return parent == null || parent.canNotifyCleared(this); } + @GuardedBy("requestLock") private boolean parentCanNotifyStatusChanged() { return parent == null || parent.canNotifyStatusChanged(this); } + @GuardedBy("requestLock") private boolean isValidRequest(Request request) { - return request.equals(primary) || (primary.isFailed() && request.equals(error)); + return request.equals(primary) + || (primaryState == RequestState.FAILED && request.equals(error)); } @Override public boolean isAnyResourceSet() { - return parentIsAnyResourceSet() || isResourceSet(); + synchronized (requestLock) { + return parentIsAnyResourceSet() || isComplete(); + } } + @GuardedBy("requestLock") private boolean parentIsAnyResourceSet() { return parent != null && parent.isAnyResourceSet(); } @Override public void onRequestSuccess(Request request) { - if (parent != null) { - parent.onRequestSuccess(this); + synchronized (requestLock) { + if (request.equals(primary)) { + primaryState = RequestState.SUCCESS; + } else if (request.equals(error)) { + errorState = RequestState.SUCCESS; + } + if (parent != null) { + parent.onRequestSuccess(this); + } } } @Override public void onRequestFailed(Request request) { - if (!request.equals(error)) { - if (!error.isRunning()) { - error.begin(); + synchronized (requestLock) { + if (!request.equals(error)) { + primaryState = RequestState.FAILED; + if (errorState != RequestState.RUNNING) { + errorState = RequestState.RUNNING; + error.begin(); + } + return; } - return; - } - if (parent != null) { - parent.onRequestFailed(this); + errorState = RequestState.FAILED; + + if (parent != null) { + parent.onRequestFailed(this); + } } } } diff --git a/library/src/main/java/com/bumptech/glide/request/Request.java b/library/src/main/java/com/bumptech/glide/request/Request.java index 77dcb888e9..e6d5e2da14 100644 --- a/library/src/main/java/com/bumptech/glide/request/Request.java +++ b/library/src/main/java/com/bumptech/glide/request/Request.java @@ -29,18 +29,9 @@ public interface Request { /** Returns true if the request has completed successfully. */ boolean isComplete(); - /** - * Returns true if a non-placeholder resource is put. For Requests that load more than one - * resource, isResourceSet may return true even if {@link #isComplete()}} returns false. - */ - boolean isResourceSet(); - /** Returns true if the request has been cleared. */ boolean isCleared(); - /** Returns true if the request has failed. */ - boolean isFailed(); - /** Recycles the request object and releases its resources. */ void recycle(); diff --git a/library/src/main/java/com/bumptech/glide/request/RequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/RequestCoordinator.java index 8a13b53f9c..c71ee5639c 100644 --- a/library/src/main/java/com/bumptech/glide/request/RequestCoordinator.java +++ b/library/src/main/java/com/bumptech/glide/request/RequestCoordinator.java @@ -3,6 +3,11 @@ /** * An interface for coordinating multiple requests with the same {@link * com.bumptech.glide.request.target.Target}. + * + *

To avoid deadlock, implemenations must not call into individual {@link Request}s to + * determine their state (ie do not call {@link Request#isCleared()} or {@link Request#isRunning()} + * etc). Instead use {@link RequestState} and the various methods available on this interface and + * {@link Request} to track states manually. */ public interface RequestCoordinator { @@ -38,4 +43,24 @@ public interface RequestCoordinator { /** Must be called when a {@link Request} coordinated by this object fails. */ void onRequestFailed(Request request); + + /** A simple state enum to keep track of the states of individual subrequests. */ + enum RequestState { + RUNNING(false), + PAUSED(false), + CLEARED(false), + SUCCESS(true), + FAILED(true); + + private final boolean isComplete; + + RequestState(boolean isComplete) { + + this.isComplete = isComplete; + } + + boolean isComplete() { + return isComplete; + } + } } diff --git a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java index 44b6b8397d..f5fb9f2d45 100644 --- a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java +++ b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java @@ -34,6 +34,7 @@ * * @param The type of the resource that will be transcoded from the loaded resource. */ +@SuppressWarnings("SynchronizeOnNonFinalField") public final class SingleRequest implements Request, SizeReadyCallback, ResourceCallback, FactoryPools.Poolable { /** Tag for logging internal events, not generally suitable for public use. */ @@ -50,7 +51,6 @@ public SingleRequest create() { return new SingleRequest(); } }); - private boolean isCallingCallbacks; private static final boolean IS_VERBOSE_LOGGABLE = Log.isLoggable(TAG, Log.VERBOSE); @@ -74,38 +74,95 @@ private enum Status { private final StateVerifier stateVerifier = StateVerifier.newInstance(); - @Nullable private RequestListener targetListener; + /* Variables mutated only when a request is initialized or returned to the object pool. */ + private volatile Object requestLock; + + @GuardedBy("requestLock") + @Nullable + private RequestListener targetListener; + + @GuardedBy("requestLock") private RequestCoordinator requestCoordinator; + + @GuardedBy("requestLock") private Context context; + + @GuardedBy("requestLock") private GlideContext glideContext; - @Nullable private Object model; + + @GuardedBy("requestLock") + @Nullable + private Object model; + + @GuardedBy("requestLock") private Class transcodeClass; + + @GuardedBy("requestLock") private BaseRequestOptions requestOptions; + + @GuardedBy("requestLock") private int overrideWidth; + + @GuardedBy("requestLock") private int overrideHeight; + + @GuardedBy("requestLock") private Priority priority; + + @GuardedBy("requestLock") private Target target; - @Nullable private List> requestListeners; - private Engine engine; + + @GuardedBy("requestLock") + @Nullable + private List> requestListeners; + + @GuardedBy("requestLock") private TransitionFactory animationFactory; + + @GuardedBy("requestLock") private Executor callbackExecutor; + + @GuardedBy("requestLock") private Resource resource; + + @GuardedBy("requestLock") private Engine.LoadStatus loadStatus; + + @GuardedBy("requestLock") private long startTime; - @GuardedBy("this") + // Volatile because it's accessed outside of a lock and nullable, even though in practice it will + // always be non-null unless the request is in the object pool. + private volatile Engine engine; + + /* Variables mutated during a request. */ + @GuardedBy("requestLock") private Status status; + @GuardedBy("requestLock") private Drawable errorDrawable; + + @GuardedBy("requestLock") private Drawable placeholderDrawable; + + @GuardedBy("requestLock") private Drawable fallbackDrawable; + + @GuardedBy("requestLock") private int width; + + @GuardedBy("requestLock") private int height; + + @GuardedBy("requestLock") + private boolean isCallingCallbacks; + @Nullable private RuntimeException requestOrigin; public static SingleRequest obtain( Context context, GlideContext glideContext, + @Nullable Object requestLock, Object model, Class transcodeClass, BaseRequestOptions requestOptions, @@ -124,9 +181,13 @@ public static SingleRequest obtain( if (request == null) { request = new SingleRequest<>(); } + if (requestLock == null) { + requestLock = request; + } request.init( context, glideContext, + requestLock, model, transcodeClass, requestOptions, @@ -149,9 +210,12 @@ public static SingleRequest obtain( // just create, instances are reused with recycle/init } - private synchronized void init( + // We are in fact locking on the same lock that will be used for all subsequent method calls. + @SuppressWarnings("GuardedBy") + private void init( Context context, GlideContext glideContext, + @NonNull Object requestLock, Object model, Class transcodeClass, BaseRequestOptions requestOptions, @@ -165,25 +229,28 @@ private synchronized void init( Engine engine, TransitionFactory animationFactory, Executor callbackExecutor) { - this.context = context; - this.glideContext = glideContext; - this.model = model; - this.transcodeClass = transcodeClass; - this.requestOptions = requestOptions; - this.overrideWidth = overrideWidth; - this.overrideHeight = overrideHeight; - this.priority = priority; - this.target = target; - this.targetListener = targetListener; - this.requestListeners = requestListeners; - this.requestCoordinator = requestCoordinator; - this.engine = engine; - this.animationFactory = animationFactory; - this.callbackExecutor = callbackExecutor; - status = Status.PENDING; - - if (requestOrigin == null && glideContext.isLoggingRequestOriginsEnabled()) { - requestOrigin = new RuntimeException("Glide request origin trace"); + this.requestLock = requestLock; + synchronized (this.requestLock) { + this.context = context; + this.glideContext = glideContext; + this.model = model; + this.transcodeClass = transcodeClass; + this.requestOptions = requestOptions; + this.overrideWidth = overrideWidth; + this.overrideHeight = overrideHeight; + this.priority = priority; + this.target = target; + this.targetListener = targetListener; + this.requestListeners = requestListeners; + this.requestCoordinator = requestCoordinator; + this.engine = engine; + this.animationFactory = animationFactory; + this.callbackExecutor = callbackExecutor; + status = Status.PENDING; + + if (requestOrigin == null && glideContext.isLoggingRequestOriginsEnabled()) { + requestOrigin = new RuntimeException("Glide request origin trace"); + } } } @@ -194,78 +261,83 @@ public StateVerifier getVerifier() { } @Override - public synchronized void recycle() { - assertNotCallingCallbacks(); - context = null; - glideContext = null; - model = null; - transcodeClass = null; - requestOptions = null; - overrideWidth = -1; - overrideHeight = -1; - target = null; - requestListeners = null; - targetListener = null; - requestCoordinator = null; - animationFactory = null; - loadStatus = null; - errorDrawable = null; - placeholderDrawable = null; - fallbackDrawable = null; - width = -1; - height = -1; - requestOrigin = null; - POOL.release(this); + public void recycle() { + synchronized (requestLock) { + assertNotCallingCallbacks(); + context = null; + glideContext = null; + model = null; + transcodeClass = null; + requestOptions = null; + overrideWidth = -1; + overrideHeight = -1; + target = null; + requestListeners = null; + targetListener = null; + requestCoordinator = null; + animationFactory = null; + loadStatus = null; + errorDrawable = null; + placeholderDrawable = null; + fallbackDrawable = null; + width = -1; + height = -1; + requestOrigin = null; + POOL.release(this); + } + requestLock = null; } @Override - public synchronized void begin() { - assertNotCallingCallbacks(); - stateVerifier.throwIfRecycled(); - startTime = LogTime.getLogTime(); - if (model == null) { - if (Util.isValidDimensions(overrideWidth, overrideHeight)) { - width = overrideWidth; - height = overrideHeight; + public void begin() { + synchronized (requestLock) { + assertNotCallingCallbacks(); + stateVerifier.throwIfRecycled(); + startTime = LogTime.getLogTime(); + if (model == null) { + if (Util.isValidDimensions(overrideWidth, overrideHeight)) { + width = overrideWidth; + height = overrideHeight; + } + // Only log at more verbose log levels if the user has set a fallback drawable, because + // fallback Drawables indicate the user expects null models occasionally. + int logLevel = getFallbackDrawable() == null ? Log.WARN : Log.DEBUG; + onLoadFailed(new GlideException("Received null model"), logLevel); + return; } - // Only log at more verbose log levels if the user has set a fallback drawable, because - // fallback Drawables indicate the user expects null models occasionally. - int logLevel = getFallbackDrawable() == null ? Log.WARN : Log.DEBUG; - onLoadFailed(new GlideException("Received null model"), logLevel); - return; - } - if (status == Status.RUNNING) { - throw new IllegalArgumentException("Cannot restart a running request"); - } + if (status == Status.RUNNING) { + throw new IllegalArgumentException("Cannot restart a running request"); + } - // If we're restarted after we're complete (usually via something like a notifyDataSetChanged - // that starts an identical request into the same Target or View), we can simply use the - // resource and size we retrieved the last time around and skip obtaining a new size, starting a - // new load etc. This does mean that users who want to restart a load because they expect that - // the view size has changed will need to explicitly clear the View or Target before starting - // the new load. - if (status == Status.COMPLETE) { - onResourceReady(resource, DataSource.MEMORY_CACHE); - return; - } + // If we're restarted after we're complete (usually via something like a notifyDataSetChanged + // that starts an identical request into the same Target or View), we can simply use the + // resource and size we retrieved the last time around and skip obtaining a new size, starting + // a new load etc. This does mean that users who want to restart a load because they expect + // that the view size has changed will need to explicitly clear the View or Target before + // starting the new load. + if (status == Status.COMPLETE) { + onResourceReady(resource, DataSource.MEMORY_CACHE); + return; + } - // Restarts for requests that are neither complete nor running can be treated as new requests - // and can run again from the beginning. + // Restarts for requests that are neither complete nor running can be treated as new requests + // and can run again from the beginning. - status = Status.WAITING_FOR_SIZE; - if (Util.isValidDimensions(overrideWidth, overrideHeight)) { - onSizeReady(overrideWidth, overrideHeight); - } else { - target.getSize(this); - } + status = Status.WAITING_FOR_SIZE; + if (Util.isValidDimensions(overrideWidth, overrideHeight)) { + onSizeReady(overrideWidth, overrideHeight); + } else { + target.getSize(this); + } - if ((status == Status.RUNNING || status == Status.WAITING_FOR_SIZE) - && canNotifyStatusChanged()) { - target.onLoadStarted(getPlaceholderDrawable()); - } - if (IS_VERBOSE_LOGGABLE) { - logV("finished run method in " + LogTime.getElapsedMillis(startTime)); + if ((status == Status.RUNNING || status == Status.WAITING_FOR_SIZE) + && canNotifyStatusChanged()) { + target.onLoadStarted(getPlaceholderDrawable()); + } + if (IS_VERBOSE_LOGGABLE) { + logV("finished run method in " + LogTime.getElapsedMillis(startTime)); + } } } @@ -277,6 +349,7 @@ && canNotifyStatusChanged()) { * * @see #clear() */ + @GuardedBy("requestLock") private void cancel() { assertNotCallingCallbacks(); stateVerifier.throwIfRecycled(); @@ -288,6 +361,7 @@ private void cancel() { } // Avoids difficult to understand errors like #2413. + @GuardedBy("requestLock") private void assertNotCallingCallbacks() { if (isCallingCallbacks) { throw new IllegalStateException( @@ -309,7 +383,7 @@ private void assertNotCallingCallbacks() { @Override public void clear() { Resource toRelease = null; - synchronized (this) { + synchronized (requestLock) { assertNotCallingCallbacks(); stateVerifier.throwIfRecycled(); if (status == Status.CLEARED) { @@ -334,37 +408,36 @@ public void clear() { } @Override - public synchronized void pause() { - if (isRunning()) { - clear(); + public void pause() { + synchronized (requestLock) { + if (isRunning()) { + clear(); + } } } @Override - public synchronized boolean isRunning() { - return status == Status.RUNNING || status == Status.WAITING_FOR_SIZE; - } - - @Override - public synchronized boolean isComplete() { - return status == Status.COMPLETE; - } - - @Override - public synchronized boolean isResourceSet() { - return isComplete(); + public boolean isRunning() { + synchronized (requestLock) { + return status == Status.RUNNING || status == Status.WAITING_FOR_SIZE; + } } @Override - public synchronized boolean isCleared() { - return status == Status.CLEARED; + public boolean isComplete() { + synchronized (requestLock) { + return status == Status.COMPLETE; + } } @Override - public synchronized boolean isFailed() { - return status == Status.FAILED; + public boolean isCleared() { + synchronized (requestLock) { + return status == Status.CLEARED; + } } + @GuardedBy("requestLock") private Drawable getErrorDrawable() { if (errorDrawable == null) { errorDrawable = requestOptions.getErrorPlaceholder(); @@ -375,6 +448,7 @@ private Drawable getErrorDrawable() { return errorDrawable; } + @GuardedBy("requestLock") private Drawable getPlaceholderDrawable() { if (placeholderDrawable == null) { placeholderDrawable = requestOptions.getPlaceholderDrawable(); @@ -385,6 +459,7 @@ private Drawable getPlaceholderDrawable() { return placeholderDrawable; } + @GuardedBy("requestLock") private Drawable getFallbackDrawable() { if (fallbackDrawable == null) { fallbackDrawable = requestOptions.getFallbackDrawable(); @@ -395,13 +470,15 @@ private Drawable getFallbackDrawable() { return fallbackDrawable; } + @GuardedBy("requestLock") private Drawable loadDrawable(@DrawableRes int resourceId) { Theme theme = requestOptions.getTheme() != null ? requestOptions.getTheme() : context.getTheme(); return DrawableDecoderCompat.getDrawable(glideContext, resourceId, theme); } - private synchronized void setErrorPlaceholder() { + @GuardedBy("requestLock") + private void setErrorPlaceholder() { if (!canNotifyStatusChanged()) { return; } @@ -423,53 +500,55 @@ private synchronized void setErrorPlaceholder() { /** A callback method that should never be invoked directly. */ @Override - public synchronized void onSizeReady(int width, int height) { + public void onSizeReady(int width, int height) { stateVerifier.throwIfRecycled(); - if (IS_VERBOSE_LOGGABLE) { - logV("Got onSizeReady in " + LogTime.getElapsedMillis(startTime)); - } - if (status != Status.WAITING_FOR_SIZE) { - return; - } - status = Status.RUNNING; + synchronized (requestLock) { + if (IS_VERBOSE_LOGGABLE) { + logV("Got onSizeReady in " + LogTime.getElapsedMillis(startTime)); + } + if (status != Status.WAITING_FOR_SIZE) { + return; + } + status = Status.RUNNING; - float sizeMultiplier = requestOptions.getSizeMultiplier(); - this.width = maybeApplySizeMultiplier(width, sizeMultiplier); - this.height = maybeApplySizeMultiplier(height, sizeMultiplier); + float sizeMultiplier = requestOptions.getSizeMultiplier(); + this.width = maybeApplySizeMultiplier(width, sizeMultiplier); + this.height = maybeApplySizeMultiplier(height, sizeMultiplier); - if (IS_VERBOSE_LOGGABLE) { - logV("finished setup for calling load in " + LogTime.getElapsedMillis(startTime)); - } - loadStatus = - engine.load( - glideContext, - model, - requestOptions.getSignature(), - this.width, - this.height, - requestOptions.getResourceClass(), - transcodeClass, - priority, - requestOptions.getDiskCacheStrategy(), - requestOptions.getTransformations(), - requestOptions.isTransformationRequired(), - requestOptions.isScaleOnlyOrNoTransform(), - requestOptions.getOptions(), - requestOptions.isMemoryCacheable(), - requestOptions.getUseUnlimitedSourceGeneratorsPool(), - requestOptions.getUseAnimationPool(), - requestOptions.getOnlyRetrieveFromCache(), - this, - callbackExecutor); - - // This is a hack that's only useful for testing right now where loads complete synchronously - // even though under any executor running on any thread but the main thread, the load would - // have completed asynchronously. - if (status != Status.RUNNING) { - loadStatus = null; - } - if (IS_VERBOSE_LOGGABLE) { - logV("finished onSizeReady in " + LogTime.getElapsedMillis(startTime)); + if (IS_VERBOSE_LOGGABLE) { + logV("finished setup for calling load in " + LogTime.getElapsedMillis(startTime)); + } + loadStatus = + engine.load( + glideContext, + model, + requestOptions.getSignature(), + this.width, + this.height, + requestOptions.getResourceClass(), + transcodeClass, + priority, + requestOptions.getDiskCacheStrategy(), + requestOptions.getTransformations(), + requestOptions.isTransformationRequired(), + requestOptions.isScaleOnlyOrNoTransform(), + requestOptions.getOptions(), + requestOptions.isMemoryCacheable(), + requestOptions.getUseUnlimitedSourceGeneratorsPool(), + requestOptions.getUseAnimationPool(), + requestOptions.getOnlyRetrieveFromCache(), + this, + callbackExecutor); + + // This is a hack that's only useful for testing right now where loads complete synchronously + // even though under any executor running on any thread but the main thread, the load would + // have completed asynchronously. + if (status != Status.RUNNING) { + loadStatus = null; + } + if (IS_VERBOSE_LOGGABLE) { + logV("finished onSizeReady in " + LogTime.getElapsedMillis(startTime)); + } } } @@ -477,28 +556,34 @@ private static int maybeApplySizeMultiplier(int size, float sizeMultiplier) { return size == Target.SIZE_ORIGINAL ? size : Math.round(sizeMultiplier * size); } + @GuardedBy("requestLock") private boolean canSetResource() { return requestCoordinator == null || requestCoordinator.canSetImage(this); } + @GuardedBy("requestLock") private boolean canNotifyCleared() { return requestCoordinator == null || requestCoordinator.canNotifyCleared(this); } + @GuardedBy("requestLock") private boolean canNotifyStatusChanged() { return requestCoordinator == null || requestCoordinator.canNotifyStatusChanged(this); } + @GuardedBy("requestLock") private boolean isFirstReadyResource() { return requestCoordinator == null || !requestCoordinator.isAnyResourceSet(); } + @GuardedBy("requestLock") private void notifyLoadSuccess() { if (requestCoordinator != null) { requestCoordinator.onRequestSuccess(this); } } + @GuardedBy("requestLock") private void notifyLoadFailed() { if (requestCoordinator != null) { requestCoordinator.onRequestFailed(this); @@ -508,11 +593,11 @@ private void notifyLoadFailed() { /** A callback method that should never be invoked directly. */ @SuppressWarnings("unchecked") @Override - public synchronized void onResourceReady(Resource resource, DataSource dataSource) { + public void onResourceReady(Resource resource, DataSource dataSource) { + stateVerifier.throwIfRecycled(); Resource toRelease = null; try { - synchronized (this) { - stateVerifier.throwIfRecycled(); + synchronized (requestLock) { loadStatus = null; if (resource == null) { GlideException exception = @@ -576,7 +661,8 @@ public synchronized void onResourceReady(Resource resource, DataSource dataSo * @param result object returned by {@link Resource#get()}, checked for type and never null * */ - private synchronized void onResourceReady(Resource resource, R result, DataSource dataSource) { + @GuardedBy("requestLock") + private void onResourceReady(Resource resource, R result, DataSource dataSource) { // We must call isFirstReadyResource before setting status. boolean isFirstResource = isFirstReadyResource(); status = Status.COMPLETE; @@ -626,76 +712,105 @@ private synchronized void onResourceReady(Resource resource, R result, DataSo /** A callback method that should never be invoked directly. */ @Override - public synchronized void onLoadFailed(GlideException e) { + public void onLoadFailed(GlideException e) { onLoadFailed(e, Log.WARN); } - private synchronized void onLoadFailed(GlideException e, int maxLogLevel) { + private void onLoadFailed(GlideException e, int maxLogLevel) { stateVerifier.throwIfRecycled(); - e.setOrigin(requestOrigin); - int logLevel = glideContext.getLogLevel(); - if (logLevel <= maxLogLevel) { - Log.w(GLIDE_TAG, "Load failed for " + model + " with size [" + width + "x" + height + "]", e); - if (logLevel <= Log.INFO) { - e.logRootCauses(GLIDE_TAG); + synchronized (requestLock) { + e.setOrigin(requestOrigin); + int logLevel = glideContext.getLogLevel(); + if (logLevel <= maxLogLevel) { + Log.w( + GLIDE_TAG, "Load failed for " + model + " with size [" + width + "x" + height + "]", e); + if (logLevel <= Log.INFO) { + e.logRootCauses(GLIDE_TAG); + } } - } - loadStatus = null; - status = Status.FAILED; + loadStatus = null; + status = Status.FAILED; + + isCallingCallbacks = true; + try { + // TODO: what if this is a thumbnail request? + boolean anyListenerHandledUpdatingTarget = false; + if (requestListeners != null) { + for (RequestListener listener : requestListeners) { + anyListenerHandledUpdatingTarget |= + listener.onLoadFailed(e, model, target, isFirstReadyResource()); + } + } + anyListenerHandledUpdatingTarget |= + targetListener != null + && targetListener.onLoadFailed(e, model, target, isFirstReadyResource()); - isCallingCallbacks = true; - try { - // TODO: what if this is a thumbnail request? - boolean anyListenerHandledUpdatingTarget = false; - if (requestListeners != null) { - for (RequestListener listener : requestListeners) { - anyListenerHandledUpdatingTarget |= - listener.onLoadFailed(e, model, target, isFirstReadyResource()); + if (!anyListenerHandledUpdatingTarget) { + setErrorPlaceholder(); } + } finally { + isCallingCallbacks = false; } - anyListenerHandledUpdatingTarget |= - targetListener != null - && targetListener.onLoadFailed(e, model, target, isFirstReadyResource()); - if (!anyListenerHandledUpdatingTarget) { - setErrorPlaceholder(); - } - } finally { - isCallingCallbacks = false; + notifyLoadFailed(); } - - notifyLoadFailed(); } - @SuppressWarnings("SynchronizationOnLocalVariableOrMethodParameter") @Override - public synchronized boolean isEquivalentTo(Request o) { - if (o instanceof SingleRequest) { - SingleRequest that = (SingleRequest) o; - synchronized (that) { - return overrideWidth == that.overrideWidth - && overrideHeight == that.overrideHeight - && Util.bothModelsNullEquivalentOrEquals(model, that.model) - && transcodeClass.equals(that.transcodeClass) - && requestOptions.equals(that.requestOptions) - && priority == that.priority - // We do not want to require that RequestListeners implement equals/hashcode, so we - // don't compare them using equals(). We can however, at least assert that the same - // amount of request listeners are present in both requests. - && listenerCountEquals(that); - } - } - return false; - } - - @SuppressWarnings("SynchronizationOnLocalVariableOrMethodParameter") - private synchronized boolean listenerCountEquals(SingleRequest other) { - synchronized (other) { - int firstListenerCount = requestListeners == null ? 0 : requestListeners.size(); - int secondListenerCount = other.requestListeners == null ? 0 : other.requestListeners.size(); - return firstListenerCount == secondListenerCount; - } + public boolean isEquivalentTo(Request o) { + if (!(o instanceof SingleRequest)) { + return false; + } + + int localOverrideWidth; + int localOverrideHeight; + Object localModel; + Class localTransocdeClass; + BaseRequestOptions localRequestOptions; + Priority localPriority; + int localListenerCount; + synchronized (requestLock) { + localOverrideWidth = overrideWidth; + localOverrideHeight = overrideHeight; + localModel = model; + localTransocdeClass = transcodeClass; + localRequestOptions = requestOptions; + localPriority = priority; + localListenerCount = requestListeners != null ? requestListeners.size() : 0; + } + + SingleRequest other = (SingleRequest) o; + int otherLocalOverrideWidth; + int otherLocalOverrideHeight; + Object otherLocalModel; + Class otherLocalTransocdeClass; + BaseRequestOptions otherLocalRequestOptions; + Priority otherLocalPriority; + int otherLocalListenerCount; + synchronized (other.requestLock) { + otherLocalOverrideWidth = other.overrideWidth; + otherLocalOverrideHeight = other.overrideHeight; + otherLocalModel = other.model; + otherLocalTransocdeClass = other.transcodeClass; + otherLocalRequestOptions = other.requestOptions; + otherLocalPriority = other.priority; + otherLocalListenerCount = other.requestListeners != null ? other.requestListeners.size() : 0; + } + + // If there's ever a case where synchronization matters for these values, something else has + // gone wrong. It indicates that we'er comparing at least one recycled object, which has to be + // protected against via other means. None of these values changes aside from object re-use. + return localOverrideWidth == otherLocalOverrideWidth + && localOverrideHeight == otherLocalOverrideHeight + && Util.bothModelsNullEquivalentOrEquals(localModel, otherLocalModel) + && localTransocdeClass.equals(otherLocalTransocdeClass) + && localRequestOptions.equals(otherLocalRequestOptions) + && localPriority == otherLocalPriority + // We do not want to require that RequestListeners implement equals/hashcode, so we + // don't compare them using equals(). We can however, at least assert that the same + // amount of request listeners are present in both requests. + && localListenerCount == otherLocalListenerCount; } private void logV(String message) { diff --git a/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java b/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java index a7e3747382..4782b0221c 100644 --- a/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java +++ b/library/src/main/java/com/bumptech/glide/request/ThumbnailRequestCoordinator.java @@ -1,7 +1,7 @@ package com.bumptech.glide.request; +import androidx.annotation.GuardedBy; import androidx.annotation.Nullable; -import androidx.annotation.VisibleForTesting; /** * A coordinator that coordinates two individual {@link Request}s that load a small thumbnail @@ -9,18 +9,22 @@ */ public class ThumbnailRequestCoordinator implements RequestCoordinator, Request { @Nullable private final RequestCoordinator parent; + private final Object requestLock; - private Request full; - private Request thumb; - private boolean isRunning; - private boolean isPaused; + private volatile Request full; + private volatile Request thumb; - @VisibleForTesting - ThumbnailRequestCoordinator() { - this(/*parent=*/ null); - } + @GuardedBy("requestLock") + private RequestState fullState = RequestState.CLEARED; + + @GuardedBy("requestLock") + private RequestState thumbState = RequestState.CLEARED; + // Only used to check if the full request is cleared by the thumbnail request. + @GuardedBy("requestLock") + private boolean isRunningDuringBegin; - public ThumbnailRequestCoordinator(@Nullable RequestCoordinator parent) { + public ThumbnailRequestCoordinator(Object requestLock, @Nullable RequestCoordinator parent) { + this.requestLock = requestLock; this.parent = parent; } @@ -37,9 +41,12 @@ public void setRequests(Request full, Request thumb) { */ @Override public boolean canSetImage(Request request) { - return parentCanSetImage() && (request.equals(full) || !full.isResourceSet()); + synchronized (requestLock) { + return parentCanSetImage() && (request.equals(full) || fullState != RequestState.SUCCESS); + } } + @GuardedBy("requestLock") private boolean parentCanSetImage() { return parent == null || parent.canSetImage(this); } @@ -52,54 +59,71 @@ private boolean parentCanSetImage() { */ @Override public boolean canNotifyStatusChanged(Request request) { - return parentCanNotifyStatusChanged() && request.equals(full) && !isAnyResourceSet(); + synchronized (requestLock) { + return parentCanNotifyStatusChanged() && request.equals(full) && !isResourceSet(); + } } @Override public boolean canNotifyCleared(Request request) { - return parentCanNotifyCleared() && request.equals(full) && !isPaused; + synchronized (requestLock) { + return parentCanNotifyCleared() && request.equals(full) && fullState != RequestState.PAUSED; + } } + @GuardedBy("requestLock") private boolean parentCanNotifyCleared() { return parent == null || parent.canNotifyCleared(this); } + @GuardedBy("requestLock") private boolean parentCanNotifyStatusChanged() { return parent == null || parent.canNotifyStatusChanged(this); } @Override public boolean isAnyResourceSet() { - return parentIsAnyResourceSet() || isResourceSet(); + synchronized (requestLock) { + return parentIsAnyResourceSet() || isResourceSet(); + } } @Override public void onRequestSuccess(Request request) { - if (request.equals(thumb)) { - return; - } - if (parent != null) { - parent.onRequestSuccess(this); - } - // Clearing the thumb is not necessarily safe if the thumb is being displayed in the Target, - // as a layer in a cross fade for example. The only way we know the thumb is not being - // displayed and is therefore safe to clear is if the thumb request has not yet completed. - if (!thumb.isComplete()) { - thumb.clear(); + synchronized (requestLock) { + if (request.equals(thumb)) { + thumbState = RequestState.SUCCESS; + return; + } + fullState = RequestState.SUCCESS; + if (parent != null) { + parent.onRequestSuccess(this); + } + // Clearing the thumb is not necessarily safe if the thumb is being displayed in the Target, + // as a layer in a cross fade for example. The only way we know the thumb is not being + // displayed and is therefore safe to clear is if the thumb request has not yet completed. + if (!thumbState.isComplete()) { + thumb.clear(); + } } } @Override public void onRequestFailed(Request request) { - if (!request.equals(full)) { - return; - } - - if (parent != null) { - parent.onRequestFailed(this); + synchronized (requestLock) { + if (!request.equals(full)) { + thumbState = RequestState.FAILED; + return; + } + fullState = RequestState.FAILED; + + if (parent != null) { + parent.onRequestFailed(this); + } } } + @GuardedBy("requestLock") private boolean parentIsAnyResourceSet() { return parent != null && parent.isAnyResourceSet(); } @@ -107,66 +131,83 @@ private boolean parentIsAnyResourceSet() { /** Starts first the thumb request and then the full request. */ @Override public void begin() { - isPaused = false; - isRunning = true; - // If the request has completed previously, there's no need to restart both the full and the - // thumb, we can just restart the full. - if (!full.isComplete() && !thumb.isRunning()) { - thumb.begin(); - } - if (isRunning && !full.isRunning()) { - full.begin(); + synchronized (requestLock) { + isRunningDuringBegin = true; + try { + // If the request has completed previously, there's no need to restart both the full and the + // thumb, we can just restart the full. + if (fullState != RequestState.SUCCESS && thumbState != RequestState.RUNNING) { + thumbState = RequestState.RUNNING; + thumb.begin(); + } + if (isRunningDuringBegin && fullState != RequestState.RUNNING) { + fullState = RequestState.RUNNING; + full.begin(); + } + } finally { + isRunningDuringBegin = false; + } } } @Override public void clear() { - isPaused = false; - isRunning = false; - thumb.clear(); - full.clear(); + synchronized (requestLock) { + isRunningDuringBegin = false; + fullState = RequestState.CLEARED; + thumbState = RequestState.CLEARED; + thumb.clear(); + full.clear(); + } } @Override public void pause() { - isPaused = true; - isRunning = false; - thumb.pause(); - full.pause(); + synchronized (requestLock) { + if (!thumbState.isComplete()) { + thumbState = RequestState.PAUSED; + thumb.pause(); + } + if (!fullState.isComplete()) { + fullState = RequestState.PAUSED; + full.pause(); + } + } } - /** Returns true if the full request is still running. */ @Override public boolean isRunning() { - return full.isRunning(); + synchronized (requestLock) { + return fullState == RequestState.RUNNING; + } } - /** Returns true if the full request is complete. */ @Override public boolean isComplete() { - return full.isComplete(); + synchronized (requestLock) { + return fullState == RequestState.SUCCESS; + } } - @Override - public boolean isResourceSet() { - return full.isResourceSet() || thumb.isResourceSet(); + private boolean isResourceSet() { + synchronized (requestLock) { + return fullState == RequestState.SUCCESS || thumbState == RequestState.SUCCESS; + } } @Override public boolean isCleared() { - return full.isCleared(); - } - - /** Returns true if the full request has failed. */ - @Override - public boolean isFailed() { - return full.isFailed(); + synchronized (requestLock) { + return fullState == RequestState.CLEARED; + } } @Override public void recycle() { - full.recycle(); - thumb.recycle(); + synchronized (requestLock) { + full.recycle(); + thumb.recycle(); + } } @Override diff --git a/library/test/src/test/java/com/bumptech/glide/GlideTest.java b/library/test/src/test/java/com/bumptech/glide/GlideTest.java index afb047e86c..ff5d015682 100644 --- a/library/test/src/test/java/com/bumptech/glide/GlideTest.java +++ b/library/test/src/test/java/com/bumptech/glide/GlideTest.java @@ -61,7 +61,6 @@ import com.bumptech.glide.tests.TearDownGlide; import com.bumptech.glide.tests.Util; import com.bumptech.glide.testutil.TestResourceUtil; -import com.bumptech.glide.util.Preconditions; import java.io.ByteArrayInputStream; import java.io.File; import java.io.InputStream; @@ -689,13 +688,10 @@ public void onLoadCleared(@Nullable Drawable placeholder) { } }); - Request request = Preconditions.checkNotNull(target.getRequest()); - requestManager.onDestroy(); requestManager.clear(target); assertThat(target.getRequest()).isNull(); - assertThat(request.isCleared()).isTrue(); } @Test diff --git a/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java b/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java index c1868420c1..d0b9708a18 100644 --- a/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java +++ b/library/test/src/test/java/com/bumptech/glide/manager/RequestTrackerTest.java @@ -131,17 +131,6 @@ public void pauseRequests_withCompletedRequest_doesNotPauseRequest() { assertThat(request.isPaused()).isFalse(); } - @Test - public void pauseRequests_withFailedRequest_doesNotPauseRequest() { - FakeRequest request = new FakeRequest(); - tracker.addRequest(request); - - request.setIsFailed(); - tracker.pauseRequests(); - - assertThat(request.isPaused()).isFalse(); - } - @Test public void pauseRequests_withClearedRequest_doesNotPauseRequest() { FakeRequest request = new FakeRequest(); @@ -189,17 +178,6 @@ public void runRequest_afterPausingAndResuming_startsRequest() { assertThat(request.isRunning()).isTrue(); } - @Test - public void pauseRequests_withFailedRequest_doesNotClearRequest() { - FakeRequest request = new FakeRequest(); - request.setIsFailed(); - tracker.addRequest(request); - - tracker.pauseRequests(); - - assertThat(request.isCleared()).isFalse(); - } - @Test public void resumeRequests_withRequestAddedWhilePaused_startsRequest() { FakeRequest request = new FakeRequest(); @@ -221,17 +199,6 @@ public void resumeRequests_withCompletedRequest_doesNotRestartCompletedRequest() assertThat(request.isRunning()).isFalse(); } - @Test - public void resumeRequests_withFailedRequest_restartsRequest() { - FakeRequest request = new FakeRequest(); - request.setIsFailed(); - tracker.addRequest(request); - - tracker.resumeRequests(); - - assertThat(request.isRunning()).isTrue(); - } - @Test public void addRequest_withRunningRequest_doesNotRestartRequest() { FakeRequest request = new FakeRequest(); @@ -300,17 +267,6 @@ public void restartRequests_withRequestThatClearsAnother_avoidsConcurrentModific tracker.restartRequests(); } - @Test - public void restartRequests_withFailedRequest_restartsRequest() { - FakeRequest request = new FakeRequest(); - request.setIsFailed(); - tracker.addRequest(request); - - tracker.restartRequests(); - - assertThat(request.isRunning()).isTrue(); - } - @Test public void restartRequests_withIncompleteRequest_restartsRequest() { FakeRequest request = new FakeRequest(); @@ -324,7 +280,7 @@ public void restartRequests_withIncompleteRequest_restartsRequest() { @Test public void restartRequests_whenPaused_doesNotRestartRequests() { FakeRequest request = new FakeRequest(); - request.setIsFailed(); + request.setIsComplete(); tracker.pauseRequests(); tracker.addRequest(request); @@ -333,18 +289,6 @@ public void restartRequests_whenPaused_doesNotRestartRequests() { assertThat(request.isRunning()).isFalse(); } - @Test - public void restartRequests_withFailedRequestAddedWhilePaused_clearsFailedRequest() { - FakeRequest request = new FakeRequest(); - request.setIsFailed(); - - tracker.pauseRequests(); - tracker.addRequest(request); - - tracker.restartRequests(); - assertThat(request.isCleared()).isTrue(); - } - @Test public void restartRequests_withIncompleteRequestAddedWhilePaused_doesNotRestartRequest() { FakeRequest request = new FakeRequest(); @@ -436,7 +380,6 @@ public void pause() { } private boolean isRunning; - private boolean isFailed; private boolean isCleared; private boolean isComplete; private boolean isRecycled; @@ -449,10 +392,6 @@ void setIsComplete(boolean isComplete) { this.isComplete = isComplete; } - void setIsFailed() { - isFailed = true; - } - void setIsRunning() { isRunning = true; } @@ -479,7 +418,6 @@ public void clear() { throw new IllegalStateException(); } isRunning = false; - isFailed = false; isCleared = true; } @@ -493,21 +431,11 @@ public boolean isComplete() { return isComplete; } - @Override - public boolean isResourceSet() { - throw new UnsupportedOperationException(); - } - @Override public boolean isCleared() { return isCleared; } - @Override - public boolean isFailed() { - return isFailed; - } - @Override public void recycle() { if (isRecycled) { diff --git a/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java b/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java index 8587d1b559..cc2c4ad313 100644 --- a/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java +++ b/library/test/src/test/java/com/bumptech/glide/request/ErrorRequestCoordinatorTest.java @@ -3,9 +3,11 @@ import static com.google.common.truth.Truth.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import androidx.annotation.Nullable; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -24,7 +26,7 @@ public class ErrorRequestCoordinatorTest { @Before public void setUp() { MockitoAnnotations.initMocks(this); - coordinator = new ErrorRequestCoordinator(/*parent=*/ null); + coordinator = newCoordinator(); coordinator.setRequests(primary, error); } @@ -36,9 +38,9 @@ public void begin_startsPrimary() { @Test public void begin_whenPrimaryIsAlreadyRunning_doesNotStartPrimaryAgain() { - when(primary.isRunning()).thenReturn(true); coordinator.begin(); - verify(primary, never()).begin(); + coordinator.begin(); + verify(primary, times(1)).begin(); } @Test @@ -55,22 +57,21 @@ public void clear_whenPrimaryHasNotFailed_doesNotClearError() { @Test public void clear_whenPrimaryHasFailed_errorIsRunning_clearsError() { - when(primary.isFailed()).thenReturn(true); - when(error.isRunning()).thenReturn(true); + coordinator.onRequestFailed(primary); coordinator.clear(); verify(error).clear(); } @Test public void clear_whenPrimaryHasFailed_clearsPrimary() { - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); coordinator.clear(); verify(primary).clear(); } @Test public void clear_whenErrorIsRunning_clearsError() { - when(error.isRunning()).thenReturn(true); + coordinator.onRequestFailed(primary); coordinator.clear(); verify(error).clear(); @@ -78,65 +79,54 @@ public void clear_whenErrorIsRunning_clearsError() { @Test public void pause_whenPrimaryIsRunning_pausesPrimary() { - when(primary.isRunning()).thenReturn(true); + coordinator.begin(); coordinator.pause(); verify(primary).pause(); } - // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient - // because we don't need an additional lock. @Test public void pause_whenPrimaryIsComplete_doesNotPausePrimary() { - when(primary.isComplete()).thenReturn(true); + coordinator.onRequestSuccess(primary); coordinator.pause(); - verify(primary).pause(); + verify(primary, never()).pause(); } - // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient - // because we don't need an additional lock. @Test - public void pause_whenPrimaryIsFailed_pausesPrimary() { - when(primary.isFailed()).thenReturn(true); + public void pause_whenPrimaryIsFailed_doesNotPausePrimary() { + coordinator.onRequestFailed(primary); coordinator.pause(); - verify(primary).pause(); + verify(primary, never()).pause(); } - // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient - // because we don't need an additional lock. @Test - public void pause_whenErrorIsNotRunning_pausesError() { - when(error.isRunning()).thenReturn(false); + public void pause_whenErrorIsNotRunning_doesNotPauseError() { coordinator.pause(); - verify(error).pause(); + verify(error, never()).pause(); } - // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient - // because we don't need an additional lock. @Test - public void pause_whenErrorIsComplete_pausesError() { - when(error.isComplete()).thenReturn(true); + public void pause_whenErrorIsComplete_doesNotPauseError() { + coordinator.onRequestSuccess(error); coordinator.pause(); - verify(error).pause(); + verify(error, never()).pause(); } - // Rely on the underlying implementation to ignore the pause call. It's somewhat more efficient - // because we don't need an additional lock. @Test - public void pause_whenErrorIsFailed_pausesError() { - when(error.isFailed()).thenReturn(true); + public void pause_whenErrorIsFailed_doesNotPauseError() { + coordinator.onRequestFailed(error); coordinator.pause(); - verify(error).pause(); + verify(error, never()).pause(); } @Test public void pause_whenErrorIsRunning_pausesError() { - when(error.isRunning()).thenReturn(true); + coordinator.onRequestFailed(primary); coordinator.pause(); verify(error).pause(); @@ -149,20 +139,14 @@ public void isRunning_primaryNotFailed_primaryNotRunning_returnsFalse() { @Test public void isRunning_primaryNotFailed_primaryRunning_returnsTrue() { - when(primary.isRunning()).thenReturn(true); + coordinator.begin(); assertThat(coordinator.isRunning()).isTrue(); } @Test - public void isRunning_primaryFailed_errorNotRunning_returnsFalse() { - when(primary.isFailed()).thenReturn(true); - assertThat(coordinator.isRunning()).isFalse(); - } - - @Test - public void isRunning_primaryFailed_errorRunning_returnsTrue() { - when(primary.isFailed()).thenReturn(true); - when(error.isRunning()).thenReturn(true); + public void isRunning_primaryFailed_returnsTrue() { + coordinator.onRequestFailed(primary); + // A failed primary request starts the error request. assertThat(coordinator.isRunning()).isTrue(); } @@ -173,95 +157,49 @@ public void isComplete_primaryNotFailed_primaryNotComplete_returnsFalse() { @Test public void isComplete_primaryNotFailed_primaryComplete_returnsTrue() { - when(primary.isComplete()).thenReturn(true); + coordinator.onRequestSuccess(primary); assertThat(coordinator.isComplete()).isTrue(); } @Test public void isComplete_primaryFailed_errorNotComplete_returnsFalse() { - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.isComplete()).isFalse(); } @Test public void isComplete_primaryFailed_errorComplete_returnsTrue() { - when(primary.isFailed()).thenReturn(true); - when(error.isComplete()).thenReturn(true); + coordinator.onRequestFailed(primary); + coordinator.onRequestSuccess(error); assertThat(coordinator.isComplete()).isTrue(); } @Test - public void isResourceSet_primaryNotFailed_primaryNotResourceSet_returnsFalse() { - assertThat(coordinator.isResourceSet()).isFalse(); - } - - @Test - public void isResourceSet_primaryNotFailed_primaryResourceSet_returnsTrue() { - when(primary.isResourceSet()).thenReturn(true); - assertThat(coordinator.isResourceSet()).isTrue(); - } - - @Test - public void isResourceSet_primaryFailed_errorNotResourceSet_returnsFalse() { - when(primary.isFailed()).thenReturn(true); - assertThat(coordinator.isResourceSet()).isFalse(); - } - - @Test - public void isResourceSet_primaryFailed_errorResourceSet_returnsTrue() { - when(primary.isFailed()).thenReturn(true); - when(error.isResourceSet()).thenReturn(true); - assertThat(coordinator.isResourceSet()).isTrue(); - } - - @Test - public void isCancelled_primaryNotFailed_primaryNotCancelled_returnsFalse() { + public void isCleared_primaryNotFailed_primaryNotCancelled_returnsFalse() { + coordinator.begin(); assertThat(coordinator.isCleared()).isFalse(); } @Test - public void isCancelled_primaryNotFailed_primaryCancelled_returnsTrue() { - when(primary.isCleared()).thenReturn(true); + public void isCleared_primaryNotFailed_primaryCancelled_returnsTrue() { + coordinator.begin(); + coordinator.clear(); assertThat(coordinator.isCleared()).isTrue(); } @Test - public void isCancelled_primaryFailed_errorNotCancelled_returnsFalse() { - when(primary.isFailed()).thenReturn(true); + public void isCleared_primaryFailed_errorNotCancelled_returnsFalse() { + coordinator.onRequestFailed(primary); assertThat(coordinator.isCleared()).isFalse(); } @Test - public void isCancelled_primaryFailed_errorCancelled_returnsTrue() { - when(primary.isFailed()).thenReturn(true); - when(error.isCleared()).thenReturn(true); + public void isCleared_primaryFailed_errorCancelled_returnsTrue() { + coordinator.onRequestFailed(primary); + coordinator.clear(); assertThat(coordinator.isCleared()).isTrue(); } - @Test - public void isFailed_primaryNotFailed_errorNotFailed_returnsFalse() { - assertThat(coordinator.isFailed()).isFalse(); - } - - @Test - public void isFailed_primaryFailed_errorNotFailed_returnsFalse() { - when(primary.isFailed()).thenReturn(true); - assertThat(coordinator.isFailed()).isFalse(); - } - - @Test - public void isFailed_primaryNotFailed_errorFailed_returnsFalse() { - when(error.isFailed()).thenReturn(true); - assertThat(coordinator.isFailed()).isFalse(); - } - - @Test - public void isFailed_primaryFailed_andErrorFailed_returnsTrue() { - when(primary.isFailed()).thenReturn(true); - when(error.isFailed()).thenReturn(true); - assertThat(coordinator.isFailed()).isTrue(); - } - @Test public void recycle_recyclesPrimaryAndError() { coordinator.recycle(); @@ -273,7 +211,7 @@ public void recycle_recyclesPrimaryAndError() { public void isEquivalentTo() { assertThat(coordinator.isEquivalentTo(primary)).isFalse(); - ErrorRequestCoordinator other = new ErrorRequestCoordinator(/*parent=*/ null); + ErrorRequestCoordinator other = newCoordinator(/*parent=*/ null); assertThat(coordinator.isEquivalentTo(other)).isFalse(); other.setRequests(primary, primary); @@ -287,13 +225,13 @@ public void isEquivalentTo() { other.setRequests(primary, error); assertThat(coordinator.isEquivalentTo(other)).isTrue(); - other = new ErrorRequestCoordinator(parent); + other = newCoordinator(parent); other.setRequests(primary, error); assertThat(coordinator.isEquivalentTo(other)).isTrue(); - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - other = new ErrorRequestCoordinator(parent); + other = newCoordinator(parent); other.setRequests(primary, error); assertThat(coordinator.isEquivalentTo(other)).isTrue(); } @@ -310,7 +248,7 @@ public void canSetImage_withError_andNullParent_andNotFailedPrimary_returnsFalse @Test public void canSetImage_withNotFailedPrimary_parentCanSetImage_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.canSetImage(coordinator)).thenReturn(true); @@ -319,7 +257,7 @@ public void canSetImage_withNotFailedPrimary_parentCanSetImage_returnsTrue() { @Test public void canSetImage_withNotFailedPrimary_parentCanNotSetImage_returnsFalse() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); assertThat(coordinator.canSetImage(primary)).isFalse(); @@ -327,25 +265,25 @@ public void canSetImage_withNotFailedPrimary_parentCanNotSetImage_returnsFalse() @Test public void canSetImage_withError_andFailedPrimary_nullParent_returnsTrue() { - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canSetImage(error)).isTrue(); } @Test public void canSetImage_withError_andFailedPrimary_nonNullParentCanSetImage_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.canSetImage(coordinator)).thenReturn(true); - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canSetImage(error)).isTrue(); } @Test public void canSetImage_withError_andFailedPrimary_nonNullParentCanNotSetImage_returnsFalse() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canSetImage(error)).isFalse(); } @@ -357,7 +295,7 @@ public void canNotifyStatusChanged_withNotFailedPrimary_nullParent_returnsTrue() @Test public void canNotifyStatusChanged_withNotFailedPrimary_nonNullParentCantNotify_returnsFalse() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); assertThat(coordinator.canNotifyStatusChanged(primary)).isFalse(); @@ -365,7 +303,7 @@ public void canNotifyStatusChanged_withNotFailedPrimary_nonNullParentCantNotify_ @Test public void canNotifyStatusChanged_withNotFailedPrimary_nonNullParentCanNotify_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.canNotifyStatusChanged(coordinator)).thenReturn(true); @@ -379,25 +317,25 @@ public void canNotifyStatusChanged_withError_notFailedPrimary_nullParent_returns @Test public void canNotifyStatusChanged_withError_failedPrimary_nullParent_returnsTrue() { - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canNotifyStatusChanged(error)).isTrue(); } @Test public void canNotifyStatusChanged_withError_failedPrimary_nonNullParentCantNotify_false() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canNotifyStatusChanged(error)).isFalse(); } @Test public void canNotifyStatusChanged_withError_failedPrimary_nonNullParentCanNotify_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); when(parent.canNotifyStatusChanged(coordinator)).thenReturn(true); assertThat(coordinator.canNotifyStatusChanged(primary)).isTrue(); @@ -410,24 +348,24 @@ public void isAnyResourceSet_primaryNotSet_nullParent_returnsFalse() { @Test public void isAnyResourceSet_primarySet_nullParent_returnsTrue() { - when(primary.isResourceSet()).thenReturn(true); + coordinator.onRequestSuccess(primary); assertThat(coordinator.isAnyResourceSet()).isTrue(); } @Test public void isAnyResourceSet_primarySet_parentResourceNotSet_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - when(primary.isResourceSet()).thenReturn(true); + coordinator.onRequestSuccess(primary); assertThat(coordinator.isAnyResourceSet()).isTrue(); } @Test public void isAnyResourceSet_primarySet_parentSet_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - when(primary.isResourceSet()).thenReturn(true); + coordinator.onRequestSuccess(primary); when(parent.isAnyResourceSet()).thenReturn(true); assertThat(coordinator.isAnyResourceSet()).isTrue(); @@ -435,58 +373,43 @@ public void isAnyResourceSet_primarySet_parentSet_returnsTrue() { @Test public void isAnyResourceSet_parentSet_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.isAnyResourceSet()).thenReturn(true); assertThat(coordinator.isAnyResourceSet()).isTrue(); } - @Test - public void isAnyResourceSet_errorSet_notFailedPrimary_nullParent_returnsFalse() { - when(error.isResourceSet()).thenReturn(true); - assertThat(coordinator.isAnyResourceSet()).isFalse(); - } - @Test public void isAnyResourceSet_errorSet_failedPrimary_nullParent_returnsTrue() { - when(error.isResourceSet()).thenReturn(true); - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); + coordinator.onRequestSuccess(error); assertThat(coordinator.isAnyResourceSet()).isTrue(); } - @Test - public void isAnyResourceSet_errorSet_notFailedPrimary_nonNullParentNotSet_returnsFalse() { - coordinator = new ErrorRequestCoordinator(parent); - coordinator.setRequests(primary, error); - when(error.isResourceSet()).thenReturn(true); - - assertThat(coordinator.isAnyResourceSet()).isFalse(); - } - @Test public void isAnyResourceSet_errorSet_failedPrimary_nonNullParentNotSet_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - when(primary.isFailed()).thenReturn(true); - when(error.isResourceSet()).thenReturn(true); + coordinator.onRequestFailed(primary); + coordinator.onRequestSuccess(error); assertThat(coordinator.isAnyResourceSet()).isTrue(); } @Test public void isAnyResourceSet_errorSet_nonNullParentSet_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.isAnyResourceSet()).thenReturn(true); - when(error.isResourceSet()).thenReturn(true); + coordinator.onRequestSuccess(error); assertThat(coordinator.isAnyResourceSet()).isTrue(); } @Test public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentNotSet_returnsFalse() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); assertThat(coordinator.isAnyResourceSet()).isFalse(); @@ -494,7 +417,7 @@ public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentNotSet_retur @Test public void isAnyResourceSet_primaryNotSet_errorNotSet_nonNullParentSet_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.isAnyResourceSet()).thenReturn(true); @@ -509,7 +432,7 @@ public void onRequestSuccess_nullParent_doesNotThrow() { @Test public void onRequestSuccess_nonNullParent_callsParent() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.onRequestSuccess(primary); verify(parent).onRequestSuccess(coordinator); } @@ -520,14 +443,6 @@ public void onRequestFailed_primaryRequest_notRunningError_beingsError() { verify(error).begin(); } - @Test - public void onRequestFailed_primaryRequest_runningError_doesNotBeginError() { - when(error.isRunning()).thenReturn(true); - coordinator.onRequestFailed(primary); - - verify(error, never()).begin(); - } - @Test public void onRequestFailed_errorRequest_doesNotBeginError() { coordinator.onRequestFailed(error); @@ -536,7 +451,7 @@ public void onRequestFailed_errorRequest_doesNotBeginError() { @Test public void onRequestFailed_primaryRequest_notRunningError_nonNullParent_doesNotNotifyParent() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); coordinator.onRequestFailed(primary); @@ -545,7 +460,7 @@ public void onRequestFailed_primaryRequest_notRunningError_nonNullParent_doesNot @Test public void onRequestFailed_errorRequest_nonNullParent_notifiesParent() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); coordinator.onRequestFailed(error); @@ -555,9 +470,9 @@ public void onRequestFailed_errorRequest_nonNullParent_notifiesParent() { @Test public void onRequestFailed_primaryRequest_runningError_nonNullParent_doesNotNotifyParent() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - when(error.isRunning()).thenReturn(true); + coordinator.onRequestFailed(primary); coordinator.onRequestFailed(primary); @@ -571,7 +486,7 @@ public void canNotifyCleared_primaryRequest_nullParent_returnsTrue() { @Test public void canNotifyCleared_primaryRequest_parentCanNotNotify_returnsFalse() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); assertThat(coordinator.canNotifyCleared(primary)).isFalse(); @@ -579,7 +494,7 @@ public void canNotifyCleared_primaryRequest_parentCanNotNotify_returnsFalse() { @Test public void canNotifyCleared_primaryRequest_parentCanNotify_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.canNotifyCleared(coordinator)).thenReturn(true); @@ -588,26 +503,26 @@ public void canNotifyCleared_primaryRequest_parentCanNotify_returnsTrue() { @Test public void canNotifyCleared_primaryRequestFailed_parentCanNotify_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.canNotifyCleared(coordinator)).thenReturn(true); - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canNotifyCleared(primary)).isTrue(); } @Test public void canNotifyCleared_primaryRequestFailed_parentCanNotNotify_returnsFalse() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); - when(primary.isFailed()).thenReturn(false); + coordinator.onRequestFailed(primary); assertThat(coordinator.canNotifyCleared(primary)).isFalse(); } @Test public void canNotifyCleared_primaryRequestFailed_nullParent_returnsTrue() { - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canNotifyCleared(primary)).isTrue(); } @@ -619,27 +534,35 @@ public void canNotifyCleared_errorRequest_nullParent_returnsFalse() { @Test public void canNotifyCleared_errorRequest_primaryFailed_nullParent_returnsTrue() { - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canNotifyCleared(error)).isTrue(); } @Test public void canNotifyCleared_errorRequest_primaryFailed_nonNullParentCanNotNotify_returnsFalse() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.canNotifyCleared(coordinator)).thenReturn(false); - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canNotifyCleared(error)).isFalse(); } @Test public void canNotifyCleared_errorRequest_primaryFailed_nonNullParentCanNotify_returnsTrue() { - coordinator = new ErrorRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(primary, error); when(parent.canNotifyCleared(coordinator)).thenReturn(true); - when(primary.isFailed()).thenReturn(true); + coordinator.onRequestFailed(primary); assertThat(coordinator.canNotifyCleared(error)).isTrue(); } + + private static ErrorRequestCoordinator newCoordinator() { + return newCoordinator(/*parent=*/ null); + } + + private static ErrorRequestCoordinator newCoordinator(@Nullable RequestCoordinator parent) { + return new ErrorRequestCoordinator(/*requestLock=*/ new Object(), parent); + } } diff --git a/library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java b/library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java index f35559a1de..9bfdbfd2c3 100644 --- a/library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java +++ b/library/test/src/test/java/com/bumptech/glide/request/SingleRequestTest.java @@ -83,7 +83,6 @@ public void testCanHandleNullResources() { request.onResourceReady(null, DataSource.LOCAL); - assertTrue(request.isFailed()); verify(listener1) .onLoadFailed(isAGlideException(), isA(Number.class), eq(builder.target), anyBoolean()); } @@ -95,7 +94,6 @@ public void testCanHandleEmptyResources() { request.onResourceReady(builder.resource, DataSource.REMOTE); - assertTrue(request.isFailed()); verify(builder.engine).release(eq(builder.resource)); verify(listener1) .onLoadFailed(isAGlideException(), any(Number.class), eq(builder.target), anyBoolean()); @@ -109,32 +107,11 @@ public void testCanHandleNonConformingResources() { request.onResourceReady(builder.resource, DataSource.DATA_DISK_CACHE); - assertTrue(request.isFailed()); verify(builder.engine).release(eq(builder.resource)); verify(listener1) .onLoadFailed(isAGlideException(), any(Number.class), eq(builder.target), anyBoolean()); } - @Test - public void testIsNotFailedAfterClear() { - SingleRequest request = builder.build(); - - request.onResourceReady(null, DataSource.DATA_DISK_CACHE); - request.clear(); - - assertFalse(request.isFailed()); - } - - @Test - public void testIsNotFailedAfterBegin() { - SingleRequest request = builder.build(); - - request.onResourceReady(null, DataSource.DATA_DISK_CACHE); - request.begin(); - - assertFalse(request.isFailed()); - } - @Test public void testIsCompleteAfterReceivingResource() { SingleRequest request = builder.build(); @@ -209,21 +186,6 @@ public Object answer(InvocationOnMock invocation) { verify(requestCoordinator).canSetImage(eq(request)); } - @Test - public void testIsNotFailedWithoutException() { - SingleRequest request = builder.build(); - - assertFalse(request.isFailed()); - } - - @Test - public void testIsFailedAfterException() { - SingleRequest request = builder.build(); - - request.onLoadFailed(new GlideException("test")); - assertTrue(request.isFailed()); - } - @Test public void pause_whenRequestIsWaitingForASize_clearsRequest() { SingleRequest request = builder.build(); @@ -254,15 +216,6 @@ public void pause_whenComplete_doesNotClearRequest() { assertThat(request.isComplete()).isTrue(); } - @Test - public void pause_whenFailed_doesNotClearRequest() { - SingleRequest request = builder.build(); - - request.onLoadFailed(new GlideException("test")); - request.pause(); - assertThat(request.isFailed()).isTrue(); - } - @Test public void pause_whenCleared_doesNotClearRequest() { SingleRequest request = builder.build(); @@ -303,14 +256,6 @@ public void testIgnoresOnSizeReadyIfNotWaitingForSize() { anyExecutor()); } - @Test - public void testIsFailedAfterNoResultAndNullException() { - SingleRequest request = builder.build(); - - request.onLoadFailed(new GlideException("test")); - assertTrue(request.isFailed()); - } - @Test public void testEngineLoadCancelledOnCancel() { Engine.LoadStatus loadStatus = mock(Engine.LoadStatus.class); @@ -1048,6 +993,7 @@ SingleRequest build() { return SingleRequest.obtain( /*context=*/ glideContext, /*glideContext=*/ glideContext, + /*requestLock=*/ new Object(), model, transcodeClass, requestOptions, diff --git a/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java b/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java index 18b9ac4491..74abef7c1f 100644 --- a/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java +++ b/library/test/src/test/java/com/bumptech/glide/request/ThumbnailRequestCoordinatorTest.java @@ -8,6 +8,7 @@ import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -31,7 +32,7 @@ public class ThumbnailRequestCoordinatorTest { @Before public void setUp() { MockitoAnnotations.initMocks(this); - coordinator = new ThumbnailRequestCoordinator(); + coordinator = newCoordinator(); coordinator.setRequests(full, thumb); } @@ -41,21 +42,27 @@ public void testIsRunningIsFalseIfNeitherRequestIsRunning() { } @Test - public void testIsRunningIsTrueIfFullIsRunning() { - when(full.isRunning()).thenReturn(true); + public void isRunning_withThumbAndFullRunning_isTrue() { + coordinator.begin(); + assertTrue(coordinator.isRunning()); + } + + @Test + public void isRunning_withFullRunning_isTrue() { + coordinator.begin(); + coordinator.onRequestSuccess(thumb); assertTrue(coordinator.isRunning()); } @Test - public void testIsNotRunningIfFullIsNotRunningButThumbIs() { - when(full.isRunning()).thenReturn(false); - when(thumb.isRunning()).thenReturn(true); + public void isRunning_withThumbRunning_fullComplete_isFalse() { + coordinator.begin(); + coordinator.onRequestSuccess(full); assertFalse(coordinator.isRunning()); } @Test public void testStartsFullOnRunIfNotRunning() { - when(full.isRunning()).thenReturn(false); coordinator.begin(); verify(full).begin(); @@ -63,7 +70,6 @@ public void testStartsFullOnRunIfNotRunning() { @Test public void testStartsThumbOnRunIfNotRunning() { - when(thumb.isRunning()).thenReturn(false); coordinator.begin(); verify(thumb).begin(); @@ -71,23 +77,23 @@ public void testStartsThumbOnRunIfNotRunning() { @Test public void testDoesNotStartFullOnRunIfRunning() { - when(full.isRunning()).thenReturn(true); + coordinator.begin(); coordinator.begin(); - verify(full, never()).begin(); + verify(full, times(1)).begin(); } @Test public void testDoesNotStartThumbOnRunIfRunning() { - when(thumb.isRunning()).thenReturn(true); + coordinator.begin(); coordinator.begin(); - verify(thumb, never()).begin(); + verify(thumb, times(1)).begin(); } @Test public void begin_whenFullIsComplete_startsFull() { - when(full.isComplete()).thenReturn(true); + coordinator.onRequestSuccess(full); coordinator.begin(); @@ -96,22 +102,13 @@ public void begin_whenFullIsComplete_startsFull() { @Test public void begin_whenFullIsComplete_doesNotBeginThumb() { - when(full.isComplete()).thenReturn(true); + coordinator.onRequestSuccess(full); coordinator.begin(); verify(thumb, never()).begin(); } - @Test - public void begin_whenFullIsComplete_doesNotSetRunning() { - when(full.isComplete()).thenReturn(true); - - coordinator.begin(); - - assertThat(coordinator.isRunning()).isFalse(); - } - @Test public void testDoesNotStartFullIfClearedByThumb() { doAnswer( @@ -141,6 +138,7 @@ public void testCallsClearOnRequestsWhenCleared() { @Test public void pause_pausesThumbAndFullInOrder() { + coordinator.begin(); coordinator.pause(); InOrder order = inOrder(thumb, full); order.verify(thumb).pause(); @@ -156,14 +154,14 @@ public void testRecyclesRequestsWhenRecycled() { @Test public void testCanSetImageReturnsTrueForFullRequestIfCoordinatorIsNull() { - coordinator = new ThumbnailRequestCoordinator(); + coordinator = newCoordinator(); coordinator.setRequests(full, thumb); assertTrue(coordinator.canSetImage(full)); } @Test public void testCanSetImageReturnsTrueForFullRequestIfParentAllowsSetImage() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.canSetImage(eq(coordinator))).thenReturn(true); assertTrue(coordinator.canSetImage(full)); @@ -171,31 +169,37 @@ public void testCanSetImageReturnsTrueForFullRequestIfParentAllowsSetImage() { @Test public void testCanSetImageReturnsFalseForFullRequestIfParentDoesNotAllowSetImage() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.canSetImage(eq(coordinator))).thenReturn(false); assertFalse(coordinator.canSetImage(full)); } @Test - public void - testCanSetImageReturnsTrueForThumbRequestIfParentIsNullAndFullDoesNotHaveResourceSet() { - when(full.isResourceSet()).thenReturn(false); + public void canSetImage_forThumb_withNullParent_fullNotComplete_returnsTrue() { assertTrue(coordinator.canSetImage(thumb)); } @Test - public void testCanSetImageReturnsFalseForThumbRequestIfParentIsNullAndFullHasResourceSet() { - when(full.isResourceSet()).thenReturn(true); + public void canSetImage_forThumb_withNullParent_fullComplete_returnsFalse() { + coordinator.onRequestSuccess(full); + assertFalse(coordinator.canSetImage(thumb)); + } + + @Test + public void canSetImage_forThumb_whenDisallowedByParent_fullNotComplete_returnsFalse() { + coordinator = newCoordinator(parent); + coordinator.setRequests(full, thumb); + when(parent.canSetImage(eq(coordinator))).thenReturn(false); assertFalse(coordinator.canSetImage(thumb)); } @Test - public void testCanNotSetImageForThumbIfNotAllowedByParentAndFullDoesNotHaveResourceSet() { - coordinator = new ThumbnailRequestCoordinator(parent); + public void canSetImage_forThumb_whenDisallowedByParent_fullComplete_returnsFalse() { + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.canSetImage(eq(coordinator))).thenReturn(false); - when(full.isResourceSet()).thenReturn(false); + coordinator.onRequestSuccess(full); assertFalse(coordinator.canSetImage(thumb)); } @@ -210,20 +214,20 @@ public void testCanNotNotifyStatusChangedIfThumb() { } @Test - public void testCanNotNotifyStatusChangedIfFullHasResourceSet() { - when(full.isResourceSet()).thenReturn(true); + public void canNotNotifyStatusChanged_forFull_whenFullComplete_isFalse() { + coordinator.onRequestSuccess(full); assertFalse(coordinator.canNotifyStatusChanged(full)); } @Test - public void testCanNotNotifyStatusChangedIfThumbHasResourceSet() { - when(thumb.isResourceSet()).thenReturn(true); + public void canNotNotifyStatusChanged_forFull_whenIfThumbComplete_isFalse() { + coordinator.onRequestSuccess(thumb); assertFalse(coordinator.canNotifyStatusChanged(full)); } @Test public void testCanNotNotifyStatusChangedIfParentHasResourceSet() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.isAnyResourceSet()).thenReturn(true); assertFalse(coordinator.canNotifyStatusChanged(full)); @@ -231,7 +235,7 @@ public void testCanNotNotifyStatusChangedIfParentHasResourceSet() { @Test public void testCanNotifyStatusChangedIfParentAllowsNotify() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.canNotifyStatusChanged(eq(coordinator))).thenReturn(true); assertTrue(coordinator.canNotifyStatusChanged(full)); @@ -239,41 +243,35 @@ public void testCanNotifyStatusChangedIfParentAllowsNotify() { @Test public void testCanNotNotifyStatusChangedIfParentDoesNotAllowNotify() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.canNotifyStatusChanged(eq(coordinator))).thenReturn(false); assertFalse(coordinator.canNotifyStatusChanged(full)); } @Test - public void testIsAnyResourceSetIsFalseIfNeitherRequestHasResourceSet() { - when(full.isResourceSet()).thenReturn(false); - when(thumb.isResourceSet()).thenReturn(false); + public void isAnyResourceSet_withIncompleteThumbAndFull_isFalse() { assertFalse(coordinator.isAnyResourceSet()); } @Test - public void testIsAnyResourceSetIsTrueIfFullHasResourceSet() { - when(full.isResourceSet()).thenReturn(true); - when(thumb.isResourceSet()).thenReturn(false); + public void isAnyResourceSet_withCompleteFull_isTrue() { + coordinator.onRequestSuccess(full); assertTrue(coordinator.isAnyResourceSet()); } @Test - public void testIsAnyResourceSetIsTrueIfThumbHasResourceSet() { - when(full.isResourceSet()).thenReturn(false); - when(thumb.isResourceSet()).thenReturn(true); + public void isAnyResourceSet_withCompleteThumb_isTrue() { + coordinator.onRequestSuccess(thumb); assertTrue(coordinator.isAnyResourceSet()); } @Test - public void testIsAnyResourceSetIsTrueIfParentIsNonNullAndParentHasResourceSet() { - coordinator = new ThumbnailRequestCoordinator(parent); + public void isAnyResourceSet_withParentResourceSet_isTrue() { + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.isAnyResourceSet()).thenReturn(true); - when(full.isResourceSet()).thenReturn(false); - when(thumb.isResourceSet()).thenReturn(false); assertTrue(coordinator.isAnyResourceSet()); } @@ -284,34 +282,17 @@ public void testIsNotCompleteIfNeitherRequestIsComplete() { } @Test - public void testIsCompleteIfFullIsComplete() { - when(full.isComplete()).thenReturn(true); + public void isComplete_withFullComplete_isTrue() { + coordinator.onRequestSuccess(full); assertTrue(coordinator.isComplete()); } @Test public void isComplete_withOnlyThumbComplete_returnsFalse() { - when(thumb.isComplete()).thenReturn(true); + coordinator.onRequestSuccess(thumb); assertThat(coordinator.isComplete()).isFalse(); } - @Test - public void testIsResourceSetIsFalseIfNeitherRequestHasResourceSet() { - assertFalse(coordinator.isResourceSet()); - } - - @Test - public void testIsResourceSetIsTrueIfFullRequestHasResourceSet() { - when(full.isResourceSet()).thenReturn(true); - assertTrue(coordinator.isResourceSet()); - } - - @Test - public void testIsResourceSetIsTrueIfThumbRequestHasResourceSet() { - when(thumb.isResourceSet()).thenReturn(true); - assertTrue(coordinator.isResourceSet()); - } - @Test public void testClearsThumbRequestOnFullRequestComplete_withNullParent() { coordinator.onRequestSuccess(full); @@ -320,7 +301,7 @@ public void testClearsThumbRequestOnFullRequestComplete_withNullParent() { @Test public void testNotifiesParentOnFullRequestComplete_withNonNullParent() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); coordinator.onRequestSuccess(full); verify(parent).onRequestSuccess(eq(coordinator)); @@ -328,7 +309,7 @@ public void testNotifiesParentOnFullRequestComplete_withNonNullParent() { @Test public void testClearsThumbRequestOnFullRequestComplete_withNonNullParent() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); coordinator.onRequestSuccess(full); verify(thumb).clear(); @@ -342,14 +323,14 @@ public void testDoesNotClearThumbOnThumbRequestComplete() { @Test public void testDoesNotClearThumbOnFullComplete_whenThumbIsComplete() { - when(thumb.isComplete()).thenReturn(true); + coordinator.onRequestSuccess(thumb); coordinator.onRequestSuccess(full); verify(thumb, never()).clear(); } @Test public void testDoesNotNotifyParentOnThumbRequestComplete() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); coordinator.onRequestSuccess(thumb); @@ -368,7 +349,7 @@ public void canNotifyCleared_withFullRequest_andNullParent_returnsTrue() { @Test public void canNotifyCleared_withFullRequest_nonNullParent_parentCanClear_returnsTrue() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.canNotifyCleared(coordinator)).thenReturn(true); assertThat(coordinator.canNotifyCleared(full)).isTrue(); @@ -376,7 +357,7 @@ public void canNotifyCleared_withFullRequest_nonNullParent_parentCanClear_return @Test public void canNotifyCleared_withFullRequest_nonNullParent_parentCanNotClear_returnsFalse() { - coordinator = new ThumbnailRequestCoordinator(parent); + coordinator = newCoordinator(parent); coordinator.setRequests(full, thumb); when(parent.canNotifyCleared(coordinator)).thenReturn(false); assertThat(coordinator.canNotifyCleared(full)).isFalse(); @@ -404,22 +385,30 @@ public void canNotifyCleared_withFullRequest_afterPauseAndClear_returnsTrue() { @Test public void testIsEquivalentTo() { - ThumbnailRequestCoordinator first = new ThumbnailRequestCoordinator(); + ThumbnailRequestCoordinator first = newCoordinator(); when(full.isEquivalentTo(full)).thenReturn(true); when(thumb.isEquivalentTo(thumb)).thenReturn(true); first.setRequests(full, thumb); assertTrue(first.isEquivalentTo(first)); - ThumbnailRequestCoordinator second = new ThumbnailRequestCoordinator(); + ThumbnailRequestCoordinator second = newCoordinator(); second.setRequests(full, full); assertTrue(second.isEquivalentTo(second)); assertFalse(second.isEquivalentTo(first)); assertFalse(first.isEquivalentTo(second)); - ThumbnailRequestCoordinator third = new ThumbnailRequestCoordinator(); + ThumbnailRequestCoordinator third = newCoordinator(); third.setRequests(thumb, thumb); assertTrue(third.isEquivalentTo(third)); assertFalse(third.isEquivalentTo(first)); assertFalse(first.isEquivalentTo(third)); } + + private static ThumbnailRequestCoordinator newCoordinator() { + return newCoordinator(/*parent=*/ null); + } + + private static ThumbnailRequestCoordinator newCoordinator(RequestCoordinator parent) { + return new ThumbnailRequestCoordinator(/*requestLock=*/ new Object(), parent); + } }