From 6c7cf3f8cae998469a7e7df8c70c459311479a38 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Wed, 20 Dec 2017 11:37:10 -0800 Subject: [PATCH] Fix using a recycled DecodeJob object when ResourceEncoders throw. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s possible that a ResourceEncoder will throw unexpectedly. If they do so, the exception is caught by the DecodeJob’s top level try/catch block. Unfortunately, in a finally block lower down the stack, we were calling onEncodeComplete and returning the DecodeJob to the pool. As a result, the logic in the catch block higher up was running after the DecodeJob was already in the pool, causing an exception. Now we avoid calling onEncodeComplete and returning the DecodeJob to the pool if an exception is thrown, which allows the logic in the top level catch block to function as we expect. --- .../com/bumptech/glide/ErrorHandlingTest.java | 138 ++++++++++++++++++ .../bumptech/glide/load/engine/DecodeJob.java | 4 +- 2 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 instrumentation/src/androidTest/java/com/bumptech/glide/ErrorHandlingTest.java diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/ErrorHandlingTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/ErrorHandlingTest.java new file mode 100644 index 0000000000..ac3ed6a2f7 --- /dev/null +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/ErrorHandlingTest.java @@ -0,0 +1,138 @@ +package com.bumptech.glide; + +import static com.bumptech.glide.test.Matchers.anyDrawable; +import static com.bumptech.glide.test.Matchers.anyDrawableTarget; +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyBoolean; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +import android.content.Context; +import android.graphics.Bitmap; +import android.graphics.drawable.Drawable; +import android.support.annotation.NonNull; +import android.support.annotation.Nullable; +import android.support.test.InstrumentationRegistry; +import android.support.test.runner.AndroidJUnit4; +import com.bumptech.glide.load.DataSource; +import com.bumptech.glide.load.EncodeStrategy; +import com.bumptech.glide.load.Options; +import com.bumptech.glide.load.ResourceEncoder; +import com.bumptech.glide.load.engine.GlideException; +import com.bumptech.glide.load.engine.Resource; +import com.bumptech.glide.load.engine.executor.GlideExecutor; +import com.bumptech.glide.load.engine.executor.GlideExecutor.UncaughtThrowableStrategy; +import com.bumptech.glide.request.RequestListener; +import com.bumptech.glide.test.ConcurrencyHelper; +import com.bumptech.glide.test.ResourceIds; +import com.bumptech.glide.test.TearDownGlide; +import java.io.File; +import java.util.concurrent.CountDownLatch; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +@RunWith(AndroidJUnit4.class) +public class ErrorHandlingTest { + + @Rule public TearDownGlide tearDownGlide = new TearDownGlide(); + @Mock private RequestListener requestListener; + private final ConcurrencyHelper concurrency = new ConcurrencyHelper(); + + private Context context; + + @Before + public void setUp() throws InterruptedException { + MockitoAnnotations.initMocks(this); + context = InstrumentationRegistry.getTargetContext(); + } + + // ResourceEncoders are expected not to throw and to return true or false. If they do throw, it's + // a developer error, so we expect UncaughtThrowableStrategy to be called. + @Test + public void load_whenEncoderFails_callsUncaughtThrowableStrategy() { + WaitForErrorStrategy strategy = new WaitForErrorStrategy(); + Glide.init(context, + new GlideBuilder() + .setAnimationExecutor(GlideExecutor.newAnimationExecutor(/*threadCount=*/ 1, strategy)) + .setSourceExecutor(GlideExecutor.newSourceExecutor(strategy)) + .setDiskCacheExecutor(GlideExecutor.newDiskCacheExecutor(strategy))); + Glide.get(context).getRegistry().prepend(Bitmap.class, new FailEncoder()); + + concurrency.get( + Glide.with(context) + .load(ResourceIds.raw.canonical) + .listener(requestListener) + .submit()); + + // Writing to the disk cache and therefore the exception caused by our FailEncoder may happen + // after the request completes, so we should wait for the expected error explicitly. + ConcurrencyHelper.waitOnLatch(strategy.latch); + assertThat(strategy.error).isEqualTo(FailEncoder.TO_THROW); + + verify(requestListener, never()) + .onLoadFailed(any(GlideException.class), any(), anyDrawableTarget(), anyBoolean()); + } + + @Test + public void load_whenLoadSucceeds_butEncoderFails_doesNotCallOnLoadFailed() + throws InterruptedException { + WaitForErrorStrategy strategy = new WaitForErrorStrategy(); + Glide.init(context, + new GlideBuilder() + .setAnimationExecutor(GlideExecutor.newAnimationExecutor(/*threadCount=*/ 1, strategy)) + .setSourceExecutor(GlideExecutor.newSourceExecutor(strategy)) + .setDiskCacheExecutor(GlideExecutor.newDiskCacheExecutor(strategy))); + Glide.get(context).getRegistry().prepend(Bitmap.class, new FailEncoder()); + + concurrency.get( + Glide.with(context) + .load(ResourceIds.raw.canonical) + .listener(requestListener) + .submit()); + + verify(requestListener) + .onResourceReady( + anyDrawable(), + any(), + anyDrawableTarget(), + any(DataSource.class), + anyBoolean()); + verify(requestListener, never()) + .onLoadFailed(any(GlideException.class), any(), anyDrawableTarget(), anyBoolean()); + } + + private static final class WaitForErrorStrategy implements UncaughtThrowableStrategy { + final CountDownLatch latch = new CountDownLatch(1); + @Nullable Throwable error = null; + + @Override + public void handle(Throwable t) { + if (error != null) { + throw new IllegalArgumentException("Received second error", t); + } + error = t; + latch.countDown(); + } + } + + private static final class FailEncoder implements ResourceEncoder { + + static final RuntimeException TO_THROW = new RuntimeException(); + + @Override + public EncodeStrategy getEncodeStrategy(@NonNull Options options) { + return EncodeStrategy.TRANSFORMED; + } + + @Override + public boolean encode( + @NonNull Resource data, @NonNull File file, @NonNull Options options) { + throw TO_THROW; + } + } +} 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 830ff6864d..a587886ae0 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 @@ -443,8 +443,10 @@ private void notifyEncodeAndRelease(Resource resource, DataSource dataSource) if (lockedResource != null) { lockedResource.unlock(); } - onEncodeComplete(); } + // Call onEncodeComplete outside the finally block so that it's not called if the encode process + // throws. + onEncodeComplete(); } private Resource decodeFromData(DataFetcher fetcher, Data data,