From 65048a4a241b2acc03d2e3586d6e1eec37cb9f5a Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Sun, 22 Oct 2017 00:39:36 -0700 Subject: [PATCH] Pass through exception messages to RequestFutureTarget. We may want to more generically handle multiple RequestListeners in the future, but for now we can at least avoid swallowing exceptions in RequestFutureTarget without allocating new Lists every time a RequestListener is added. --- .../com/bumptech/glide/RequestBuilder.java | 130 ++++++++++++++---- .../bumptech/glide/load/engine/DecodeJob.java | 1 + .../glide/request/RequestFutureTarget.java | 67 +++++++-- .../bumptech/glide/request/SingleRequest.java | 19 ++- .../request/RequestFutureTargetTest.java | 74 ++++++++-- .../glide/request/SingleRequestTest.java | 1 + 6 files changed, 237 insertions(+), 55 deletions(-) diff --git a/library/src/main/java/com/bumptech/glide/RequestBuilder.java b/library/src/main/java/com/bumptech/glide/RequestBuilder.java index e755b07f4..38d9ce454 100644 --- a/library/src/main/java/com/bumptech/glide/RequestBuilder.java +++ b/library/src/main/java/com/bumptech/glide/RequestBuilder.java @@ -476,10 +476,19 @@ public class RequestBuilder implements Cloneable { * @see RequestManager#clear(Target) */ public > Y into(@NonNull Y target) { - return into(target, getMutableOptions()); + return into(target, /*targetListener=*/ null); } - private > Y into(@NonNull Y target, RequestOptions options) { + private > Y into( + @NonNull Y target, + @Nullable RequestListener targetListener) { + return into(target, targetListener, getMutableOptions()); + } + + private > Y into( + @NonNull Y target, + @Nullable RequestListener targetListener, + RequestOptions options) { Util.assertMainThread(); Preconditions.checkNotNull(target); if (!isModelSet) { @@ -487,7 +496,7 @@ public class RequestBuilder implements Cloneable { } options = options.autoClone(); - Request request = buildRequest(target, options); + Request request = buildRequest(target, targetListener, options); Request previous = target.getRequest(); if (request.isEquivalentTo(previous)) { @@ -557,7 +566,10 @@ public class RequestBuilder implements Cloneable { } } - return into(glideContext.buildImageViewTarget(view, transcodeClass), requestOptions); + return into( + glideContext.buildImageViewTarget(view, transcodeClass), + /*targetListener=*/ null, + requestOptions); } /** @@ -617,12 +629,12 @@ public class RequestBuilder implements Cloneable { @Override public void run() { if (!target.isCancelled()) { - into(target); + into(target, target); } } }); } else { - into(target); + into(target, target); } return target; @@ -717,15 +729,30 @@ public class RequestBuilder implements Cloneable { } } - private Request buildRequest(Target target, RequestOptions requestOptions) { - return buildRequestRecursive(target, null, transitionOptions, requestOptions.getPriority(), - requestOptions.getOverrideWidth(), requestOptions.getOverrideHeight(), requestOptions); - } - - private Request buildRequestRecursive(Target target, + private Request buildRequest( + Target target, + @Nullable RequestListener targetListener, + RequestOptions requestOptions) { + return buildRequestRecursive( + target, + targetListener, + /*requestCoordinator=*/ null, + transitionOptions, + requestOptions.getPriority(), + requestOptions.getOverrideWidth(), + requestOptions.getOverrideHeight(), + requestOptions); + } + + private Request buildRequestRecursive( + Target target, + @Nullable RequestListener targetListener, @Nullable RequestCoordinator parentCoordinator, TransitionOptions transitionOptions, - Priority priority, int overrideWidth, int overrideHeight, RequestOptions requestOptions) { + Priority priority, + int overrideWidth, + int overrideHeight, + RequestOptions requestOptions) { // Build the ErrorRequestCoordinator first if necessary so we can update parentCoordinator. ErrorRequestCoordinator errorRequestCoordinator = null; @@ -737,6 +764,7 @@ public class RequestBuilder implements Cloneable { Request mainRequest = buildThumbnailRequestRecursive( target, + targetListener, parentCoordinator, transitionOptions, priority, @@ -758,6 +786,7 @@ public class RequestBuilder implements Cloneable { Request errorRequest = errorBuilder.buildRequestRecursive( target, + targetListener, errorRequestCoordinator, errorBuilder.transitionOptions, errorBuilder.requestOptions.getPriority(), @@ -768,10 +797,15 @@ public class RequestBuilder implements Cloneable { return errorRequestCoordinator; } - private Request buildThumbnailRequestRecursive(Target target, - @Nullable RequestCoordinator parentCoordinator, - TransitionOptions transitionOptions, - Priority priority, int overrideWidth, int overrideHeight, RequestOptions requestOptions) { + private Request buildThumbnailRequestRecursive( + Target target, + RequestListener targetListener, + @Nullable RequestCoordinator parentCoordinator, + TransitionOptions transitionOptions, + Priority priority, + int overrideWidth, + int overrideHeight, + RequestOptions requestOptions) { if (thumbnailBuilder != null) { // Recursive case: contains a potentially recursive thumbnail request builder. if (isThumbnailBuilt) { @@ -800,13 +834,22 @@ public class RequestBuilder implements Cloneable { } ThumbnailRequestCoordinator coordinator = new ThumbnailRequestCoordinator(parentCoordinator); - Request fullRequest = obtainRequest(target, requestOptions, coordinator, - transitionOptions, priority, overrideWidth, overrideHeight); + Request fullRequest = + obtainRequest( + target, + targetListener, + requestOptions, + coordinator, + transitionOptions, + priority, + overrideWidth, + overrideHeight); isThumbnailBuilt = true; // Recursively generate thumbnail requests. Request thumbRequest = thumbnailBuilder.buildRequestRecursive( target, + targetListener, coordinator, thumbTransitionOptions, thumbPriority, @@ -819,27 +862,55 @@ public class RequestBuilder implements Cloneable { } else if (thumbSizeMultiplier != null) { // Base case: thumbnail multiplier generates a thumbnail request, but cannot recurse. ThumbnailRequestCoordinator coordinator = new ThumbnailRequestCoordinator(parentCoordinator); - Request fullRequest = obtainRequest(target, requestOptions, coordinator, transitionOptions, - priority, overrideWidth, overrideHeight); + Request fullRequest = + obtainRequest( + target, + targetListener, + requestOptions, + coordinator, + transitionOptions, + priority, + overrideWidth, + overrideHeight); RequestOptions thumbnailOptions = requestOptions.clone() .sizeMultiplier(thumbSizeMultiplier); - Request thumbnailRequest = obtainRequest(target, thumbnailOptions, coordinator, - transitionOptions, getThumbnailPriority(priority), overrideWidth, overrideHeight); + Request thumbnailRequest = + obtainRequest( + target, + targetListener, + thumbnailOptions, + coordinator, + transitionOptions, + getThumbnailPriority(priority), + overrideWidth, + overrideHeight); coordinator.setRequests(fullRequest, thumbnailRequest); return coordinator; } else { // Base case: no thumbnail. - return obtainRequest(target, requestOptions, parentCoordinator, transitionOptions, priority, - overrideWidth, overrideHeight); + return obtainRequest( + target, + targetListener, + requestOptions, + parentCoordinator, + transitionOptions, + priority, + overrideWidth, + overrideHeight); } } - private Request obtainRequest(Target target, - RequestOptions requestOptions, RequestCoordinator requestCoordinator, - TransitionOptions transitionOptions, Priority priority, - int overrideWidth, int overrideHeight) { + private Request obtainRequest( + Target target, + RequestListener targetListener, + RequestOptions requestOptions, + RequestCoordinator requestCoordinator, + TransitionOptions transitionOptions, + Priority priority, + int overrideWidth, + int overrideHeight) { return SingleRequest.obtain( context, glideContext, @@ -850,6 +921,7 @@ public class RequestBuilder implements Cloneable { overrideHeight, priority, target, + targetListener, requestListener, requestCoordinator, glideContext.getEngine(), diff --git a/library/src/main/java/com/bumptech/glide/load/engine/DecodeJob.java b/library/src/main/java/com/bumptech/glide/load/engine/DecodeJob.java index 045100156..9bc35e27b 100644 --- a/library/src/main/java/com/bumptech/glide/load/engine/DecodeJob.java +++ b/library/src/main/java/com/bumptech/glide/load/engine/DecodeJob.java @@ -235,6 +235,7 @@ class DecodeJob implements DataFetcherGenerator.FetcherReadyCallback, } // When we're encoding we've already notified our callback and it isn't safe to do so again. if (stage != Stage.ENCODE) { + exceptions.add(e); notifyFailed(); } if (!isCancelled) { diff --git a/library/src/main/java/com/bumptech/glide/request/RequestFutureTarget.java b/library/src/main/java/com/bumptech/glide/request/RequestFutureTarget.java index 2c6125674..7bc94ab03 100644 --- a/library/src/main/java/com/bumptech/glide/request/RequestFutureTarget.java +++ b/library/src/main/java/com/bumptech/glide/request/RequestFutureTarget.java @@ -3,9 +3,14 @@ package com.bumptech.glide.request; import android.graphics.drawable.Drawable; import android.os.Handler; import android.support.annotation.Nullable; +import com.bumptech.glide.load.DataSource; +import com.bumptech.glide.load.engine.GlideException; 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.Util; +import java.io.PrintStream; +import java.io.PrintWriter; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -47,6 +52,7 @@ import java.util.concurrent.TimeoutException; * @param The type of the resource that will be loaded. */ public class RequestFutureTarget implements FutureTarget, + RequestListener, Runnable { private static final Waiter DEFAULT_WAITER = new Waiter(); @@ -62,6 +68,7 @@ public class RequestFutureTarget implements FutureTarget, private boolean isCancelled; private boolean resultReceived; private boolean loadFailed; + @Nullable private GlideException exception; /** * Constructor for a RequestFutureTarget. Should not be used directly. @@ -162,8 +169,7 @@ public class RequestFutureTarget implements FutureTarget, */ @Override public synchronized void onLoadFailed(Drawable errorDrawable) { - loadFailed = true; - waiter.notifyAll(this); + // Ignored, synchronized for backwards compatibility. } /** @@ -171,10 +177,7 @@ public class RequestFutureTarget implements FutureTarget, */ @Override public synchronized void onResourceReady(R resource, Transition transition) { - // We might get a null result. - resultReceived = true; - this.resource = resource; - waiter.notifyAll(this); + // Ignored, synchronized for backwards compatibility. } private synchronized R doGet(Long timeoutMillis) @@ -186,7 +189,7 @@ public class RequestFutureTarget implements FutureTarget, if (isCancelled) { throw new CancellationException(); } else if (loadFailed) { - throw new ExecutionException(new IllegalStateException("Load failed")); + throw new ExecutionException(exception); } else if (resultReceived) { return resource; } @@ -200,7 +203,7 @@ public class RequestFutureTarget implements FutureTarget, if (Thread.interrupted()) { throw new InterruptedException(); } else if (loadFailed) { - throw new ExecutionException(new IllegalStateException("Load failed")); + throw new GlideExecutionException(exception); } else if (isCancelled) { throw new CancellationException(); } else if (!resultReceived) { @@ -240,6 +243,25 @@ public class RequestFutureTarget implements FutureTarget, // Do nothing. } + @Override + public synchronized boolean onLoadFailed( + @Nullable GlideException e, Object model, Target target, boolean isFirstResource) { + loadFailed = true; + exception = e; + waiter.notifyAll(this); + return false; + } + + @Override + public synchronized boolean onResourceReady( + R resource, Object model, Target target, DataSource dataSource, boolean isFirstResource) { + // We might get a null result. + resultReceived = true; + this.resource = resource; + waiter.notifyAll(this); + return false; + } + // Visible for testing. static class Waiter { @@ -251,4 +273,33 @@ public class RequestFutureTarget implements FutureTarget, toNotify.notifyAll(); } } + + private static class GlideExecutionException extends ExecutionException { + + private final GlideException cause; + + GlideExecutionException(GlideException cause) { + super(); + this.cause = cause; + } + + @Override + public void printStackTrace() { + printStackTrace(System.err); + } + + @Override + public void printStackTrace(PrintStream s) { + super.printStackTrace(s); + s.print("Caused by: "); + cause.printStackTrace(s); + } + + @Override + public void printStackTrace(PrintWriter s) { + super.printStackTrace(s); + s.print("Caused by: "); + cause.printStackTrace(s); + } + } } 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 49a6e988a..f33cc4cf7 100644 --- a/library/src/main/java/com/bumptech/glide/request/SingleRequest.java +++ b/library/src/main/java/com/bumptech/glide/request/SingleRequest.java @@ -85,6 +85,8 @@ public final class SingleRequest implements Request, private final String tag = String.valueOf(super.hashCode()); private final StateVerifier stateVerifier = StateVerifier.newInstance(); + @Nullable + private RequestListener targetListener; private RequestCoordinator requestCoordinator; private Context context; private GlideContext glideContext; @@ -119,6 +121,7 @@ public final class SingleRequest implements Request, int overrideHeight, Priority priority, Target target, + RequestListener targetListener, RequestListener requestListener, RequestCoordinator requestCoordinator, Engine engine, @@ -138,6 +141,7 @@ public final class SingleRequest implements Request, overrideHeight, priority, target, + targetListener, requestListener, requestCoordinator, engine, @@ -160,6 +164,7 @@ public final class SingleRequest implements Request, int overrideHeight, Priority priority, Target target, + RequestListener targetListener, RequestListener requestListener, RequestCoordinator requestCoordinator, Engine engine, @@ -173,6 +178,7 @@ public final class SingleRequest implements Request, this.overrideHeight = overrideHeight; this.priority = priority; this.target = target; + this.targetListener = targetListener; this.requestListener = requestListener; this.requestCoordinator = requestCoordinator; this.engine = engine; @@ -197,6 +203,7 @@ public final class SingleRequest implements Request, overrideHeight = -1; target = null; requestListener = null; + targetListener = null; requestCoordinator = null; animationFactory = null; loadStatus = null; @@ -544,8 +551,10 @@ public final class SingleRequest implements Request, isCallingCallbacks = true; try { - if (requestListener == null - || !requestListener.onResourceReady(result, model, target, dataSource, isFirstResource)) { + if ((requestListener == null + || !requestListener.onResourceReady(result, model, target, dataSource, isFirstResource)) + && (targetListener == null + || !targetListener.onResourceReady(result, model, target, dataSource, isFirstResource))) { Transition animation = animationFactory.build(dataSource, isFirstResource); target.onResourceReady(result, animation); @@ -581,8 +590,10 @@ public final class SingleRequest implements Request, isCallingCallbacks = true; try { //TODO: what if this is a thumbnail request? - if (requestListener == null - || !requestListener.onLoadFailed(e, model, target, isFirstReadyResource())) { + if ((requestListener == null + || !requestListener.onLoadFailed(e, model, target, isFirstReadyResource())) + && (targetListener == null + || !targetListener.onLoadFailed(e, model, target, isFirstReadyResource()))) { setErrorPlaceholder(); } } finally { diff --git a/library/src/test/java/com/bumptech/glide/request/RequestFutureTargetTest.java b/library/src/test/java/com/bumptech/glide/request/RequestFutureTargetTest.java index cc3c11cd0..23df8a88f 100644 --- a/library/src/test/java/com/bumptech/glide/request/RequestFutureTargetTest.java +++ b/library/src/test/java/com/bumptech/glide/request/RequestFutureTargetTest.java @@ -13,6 +13,7 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import android.os.Handler; +import com.bumptech.glide.load.DataSource; import com.bumptech.glide.request.target.SizeReadyCallback; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; @@ -61,7 +62,12 @@ public class RequestFutureTargetTest { @Test public void testReturnsTrueFromIsDoneIfDone() { - future.onResourceReady(new Object(), null); + future.onResourceReady( + /*resource=*/ new Object(), + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); assertTrue(future.isDone()); } @@ -106,7 +112,12 @@ public class RequestFutureTargetTest { @Test public void testDoesNotClearRequestIfCancelledAfterDone() { - future.onResourceReady(new Object(), null); + future.onResourceReady( + /*resource=*/ new Object(), + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); future.cancel(true); verify(request, never()).clear(); @@ -120,7 +131,12 @@ public class RequestFutureTargetTest { @Test public void testReturnsFalseFromIsCancelledIfCancelledAfterDone() { - future.onResourceReady(new Object(), null); + future.onResourceReady( + /*resource=*/ new Object(), + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); future.cancel(true); assertFalse(future.isCancelled()); @@ -134,7 +150,12 @@ public class RequestFutureTargetTest { @Test public void testReturnsFalseFromCancelIfDone() { - future.onResourceReady(new Object(), null); + future.onResourceReady( + /*resource=*/ new Object(), + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); assertFalse(future.cancel(true)); } @@ -142,7 +163,12 @@ public class RequestFutureTargetTest { public void testReturnsResourceOnGetIfAlreadyDone() throws ExecutionException, InterruptedException { Object expected = new Object(); - future.onResourceReady(expected, null); + future.onResourceReady( + /*resource=*/ expected, + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); assertEquals(expected, future.get()); } @@ -151,7 +177,12 @@ public class RequestFutureTargetTest { public void testReturnsResourceOnGetWithTimeoutIfAlreadyDone() throws InterruptedException, ExecutionException, TimeoutException { Object expected = new Object(); - future.onResourceReady(expected, null); + future.onResourceReady( + /*resource=*/ expected, + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); assertEquals(expected, future.get(100, TimeUnit.MILLISECONDS)); } @@ -173,21 +204,21 @@ public class RequestFutureTargetTest { @Test(expected = ExecutionException.class) public void testThrowsExecutionExceptionOnGetIfExceptionBeforeGet() throws ExecutionException, InterruptedException { - future.onLoadFailed(null); + future.onLoadFailed(/*e=*/ null, /*model=*/ null, future, /*isFirstResource=*/ true); future.get(); } @Test(expected = ExecutionException.class) public void testThrowsExecutionExceptionOnGetIfExceptionWithNullValueBeforeGet() throws ExecutionException, InterruptedException, TimeoutException { - future.onLoadFailed(null); + future.onLoadFailed(/*e=*/ null, /*model=*/ null, future, /*isFirstResource=*/ true); future.get(100, TimeUnit.MILLISECONDS); } @Test(expected = ExecutionException.class) public void testThrowsExecutionExceptionOnGetIfExceptionBeforeGetWithTimeout() throws ExecutionException, InterruptedException, TimeoutException { - future.onLoadFailed(null); + future.onLoadFailed(/*e=*/ null, /*model=*/ null, future, /*isFirstResource=*/ true); future.get(100, TimeUnit.MILLISECONDS); } @@ -208,7 +239,12 @@ public class RequestFutureTargetTest { public void testGetSucceedsOnMainThreadIfDone() throws ExecutionException, InterruptedException { future = new RequestFutureTarget<>(handler, width, height, true, waiter); - future.onResourceReady(new Object(), null); + future.onResourceReady( + /*resource=*/ new Object(), + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); future.get(); } @@ -232,7 +268,7 @@ public class RequestFutureTargetTest { doAnswer(new Answer() { @Override public Void answer(InvocationOnMock invocationOnMock) throws Throwable { - future.onLoadFailed(null); + future.onLoadFailed(/*e=*/ null, /*model=*/ null, future, /*isFirstResource=*/ true); return null; } }).when(waiter).waitForTimeout(eq(future), anyLong()); @@ -266,13 +302,18 @@ public class RequestFutureTargetTest { @Test public void testNotifiesAllWhenLoadFails() { - future.onLoadFailed(null); + future.onLoadFailed(/*e=*/ null, /*model=*/ null, future, /*isFirstResource=*/ true); verify(waiter).notifyAll(eq(future)); } @Test public void testNotifiesAllWhenResourceReady() { - future.onResourceReady(null, null); + future.onResourceReady( + /*resource=*/ new Object(), + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); verify(waiter).notifyAll(eq(future)); } @@ -297,7 +338,12 @@ public class RequestFutureTargetTest { doAnswer(new Answer() { @Override public Void answer(InvocationOnMock invocationOnMock) throws Throwable { - future.onResourceReady(expected, null); + future.onResourceReady( + /*resource=*/ expected, + /*model=*/ null, + /*target=*/future, + DataSource.DATA_DISK_CACHE, + true /*isFirstResource*/); return null; } }).when(waiter).waitForTimeout(eq(future), anyLong()); diff --git a/library/src/test/java/com/bumptech/glide/request/SingleRequestTest.java b/library/src/test/java/com/bumptech/glide/request/SingleRequestTest.java index 86c73fa82..0002b5288 100644 --- a/library/src/test/java/com/bumptech/glide/request/SingleRequestTest.java +++ b/library/src/test/java/com/bumptech/glide/request/SingleRequestTest.java @@ -1002,6 +1002,7 @@ public class SingleRequestTest { overrideHeight, priority, target, + /*targetListener=*/ null, requestListener, requestCoordinator, engine, -- GitLab