Skip to content

Commit

Permalink
Avoid re-using completed previous requests if skipMemoryCache is true.
Browse files Browse the repository at this point in the history
Fixes #2663.
  • Loading branch information
sjudd committed Dec 7, 2017
1 parent 07ae4f7 commit 3dc1d18
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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
Expand Down
16 changes: 13 additions & 3 deletions library/src/main/java/com/bumptech/glide/RequestBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -570,16 +570,17 @@ private <Y extends Target<TranscodeType>> 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
// restart the request, giving it another chance to complete. If the request is already
// 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;
Expand All @@ -592,6 +593,15 @@ private <Y extends Target<TranscodeType>> 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
Expand Down

0 comments on commit 3dc1d18

Please sign in to comment.