From 3dc1d18da75b44d8d97f4161fe0d330e4993e537 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Wed, 6 Dec 2017 21:58:43 -0800 Subject: [PATCH] Avoid re-using completed previous requests if skipMemoryCache is true. Fixes #2663. --- .../java/com/bumptech/glide/CachingTest.java | 83 +++++++++++++++++++ .../com/bumptech/glide/RequestBuilder.java | 16 +++- 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/CachingTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/CachingTest.java index b17001dd69..4c6ec95365 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/CachingTest.java +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/CachingTest.java @@ -9,6 +9,7 @@ import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyBoolean; import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; import android.content.Context; @@ -18,6 +19,8 @@ import android.os.Looper; import android.support.test.InstrumentationRegistry; import android.support.test.runner.AndroidJUnit4; +import android.view.ViewGroup.LayoutParams; +import android.widget.ImageView; import com.bumptech.glide.load.DataSource; import com.bumptech.glide.load.engine.DiskCacheStrategy; import com.bumptech.glide.load.engine.cache.LruResourceCache; @@ -43,6 +46,7 @@ import org.junit.Test; import org.junit.function.ThrowingRunnable; import org.junit.runner.RunWith; +import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -356,6 +360,85 @@ public void run() throws Throwable { }); } + + @Test + public void loadIntoView_withoutSkipMemoryCache_loadsFromMemoryCacheIfPresent() { + final ImageView imageView = new ImageView(context); + imageView.setLayoutParams(new LayoutParams(100, 100)); + + concurrency.loadOnMainThread( + GlideApp.with(context) + .load(ResourceIds.raw.canonical) + .listener(requestListener) + .dontTransform(), + imageView); + + // Casting avoids a varags array warning. + reset((RequestListener) requestListener); + + // Run on the main thread, since this is already cached, we shouldn't need to try to wait. If we + // do end up re-using the old Target, our wait will always timeout anyway if we use + // loadOnMainThread. If the load doesn't complete in time, it will be caught by the listener + // below, which expects to be called synchronously. + concurrency.runOnMainThread( + new Runnable() { + @Override + public void run() { + GlideApp.with(context) + .load(ResourceIds.raw.canonical) + .listener(requestListener) + .dontTransform() + .into(imageView); + } + }); + + verify(requestListener) + .onResourceReady( + anyDrawable(), + ArgumentMatchers.any(), + anyDrawableTarget(), + eq(DataSource.MEMORY_CACHE), + anyBoolean()); + } + + @Test + public void loadIntoView_withSkipMemoryCache_doesNotLoadFromMemoryCacheIfPresent() { + final ImageView imageView = new ImageView(context); + imageView.setLayoutParams(new LayoutParams(100, 100)); + + concurrency.loadOnMainThread( + GlideApp.with(context) + .load(ResourceIds.raw.canonical) + .listener(requestListener) + .dontTransform() + .skipMemoryCache(true), + imageView); + + // Casting avoids a varags array warning. + reset((RequestListener) requestListener); + + // If this test fails due to a timeout, it's because we re-used the Target from the previous + // request, which breaks the logic in loadOnMainThread that expects a new Target's + // onResourceReady callback to be called. This can be confirmed by changing this to + // runOnMainThread and verifying that the RequestListener assertion below fails because + // the DataSource was from the memory cache. + concurrency.loadOnMainThread( + GlideApp.with(context) + .load(ResourceIds.raw.canonical) + .listener(requestListener) + .dontTransform() + .skipMemoryCache(true), + imageView); + + verify(requestListener) + .onResourceReady( + anyDrawable(), + ArgumentMatchers.any(), + anyDrawableTarget(), + not(eq(DataSource.MEMORY_CACHE)), + anyBoolean()); + } + private void clearMemoryCacheOnMainThread() throws InterruptedException { concurrency.runOnMainThread(new Runnable() { @Override diff --git a/library/src/main/java/com/bumptech/glide/RequestBuilder.java b/library/src/main/java/com/bumptech/glide/RequestBuilder.java index d59061f90f..37ab7dd7cf 100644 --- a/library/src/main/java/com/bumptech/glide/RequestBuilder.java +++ b/library/src/main/java/com/bumptech/glide/RequestBuilder.java @@ -570,7 +570,8 @@ private > Y into( Request request = buildRequest(target, targetListener, options); Request previous = target.getRequest(); - if (request.isEquivalentTo(previous)) { + if (request.isEquivalentTo(previous) + && !isSkipMemoryCacheWithCompletePreviousRequest(options, previous)) { request.recycle(); // If the request is completed, beginning again will ensure the result is re-delivered, // triggering RequestListeners and Targets. If the request is failed, beginning again will @@ -578,8 +579,8 @@ private > Y into( // running, we can let it continue running without interruption. if (!Preconditions.checkNotNull(previous).isRunning()) { // Use the previous request rather than the new one to allow for optimizations like skipping - // setting placeholders, tracking and untracking Targets, and obtaining View dimensions that - // are done in the individual Request. + // setting placeholders, tracking and un-tracking Targets, and obtaining View dimensions + // that are done in the individual Request. previous.begin(); } return target; @@ -592,6 +593,15 @@ private > Y into( return target; } + // If the caller is using skipMemoryCache and the previous request is finished, calling begin on + // the previous request will complete from memory because it will just use the resource that had + // already been loaded. If the previous request isn't complete, we can wait for it to finish + // because the previous request must also be using skipMemoryCache for the requests to be + // equivalent. See #2663 for additional context. + private boolean isSkipMemoryCacheWithCompletePreviousRequest( + RequestOptions options, Request previous) { + return options.isSkipMemoryCacheSet() && previous.isComplete(); + } /** * Sets the {@link ImageView} the resource will be loaded into, cancels any existing loads into