Keep onlyRetrieveFromCache and normal requests running independently.

This prevents cases where using onlyRetrieveFromCache might block a
request on an RPC if a request was already pending without
onlyRetrieveFromCache being set.

However, this change does now mean that it's possible a resource will be
loaded from the disk cache twice simultaneously because
onlyRetrieveFromCache requests are no longer deduped with normal
requests. We expect this case to be rare, and the consequences are an
efficiency issue rather than a correctness problem, so we're still
probably better off with this change.

Future work might include attempting to cancel and notify any normal
requests if an onlyRetrieveFromCache requests completes with the same
key will the normal request is running.

Fixes #2428.

instrumentation/src/androidTest/java/com/bumptech/glide/CachingTest.java

@@ -2,6 +2,7 @@
 
 import static com.bumptech.glide.test.Matchers.anyDrawable;
 import static com.bumptech.glide.test.Matchers.anyTarget;
+import static com.google.common.truth.Truth.assertThat;
 import static org.mockito.AdditionalMatchers.not;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyBoolean;
@@ -11,6 +12,8 @@
 import android.content.Context;
 import android.graphics.Bitmap;
 import android.graphics.drawable.Drawable;
+import android.os.Handler;
+import android.os.Looper;
 import android.support.test.InstrumentationRegistry;
 import android.support.test.runner.AndroidJUnit4;
 import com.bumptech.glide.load.DataSource;
@@ -25,7 +28,11 @@
 import com.bumptech.glide.test.ResourceIds;
 import com.bumptech.glide.test.ResourceIds.raw;
 import com.bumptech.glide.test.TearDownGlide;
+import com.bumptech.glide.test.WaitModelLoader;
+import com.bumptech.glide.test.WaitModelLoader.WaitModel;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import org.junit.Before;
 import org.junit.Rule;
@@ -271,6 +278,63 @@ public void clearDiskCache_doesNotPreventFutureLoads()
             anyBoolean());
   }
 
+  // Tests #2428.
+  @Test
+  public void onlyRetrieveFromCache_withPreviousRequestLoadingFromSource_doesNotBlock() {
+    final WaitModel<Integer> waitModel = WaitModelLoader.Factory.waitOn(ResourceIds.raw.canonical);
+
+    GlideApp.with(context)
+        .load(waitModel)
+        .submit();
+
+    FutureTarget<Drawable> onlyFromCacheFuture = GlideApp.with(context)
+        .load(waitModel)
+        .onlyRetrieveFromCache(true)
+        .submit();
+    try {
+      onlyFromCacheFuture.get(1000, TimeUnit.MILLISECONDS);
+      throw new IllegalStateException();
+    } catch (InterruptedException | TimeoutException e) {
+      throw new RuntimeException(e);
+    } catch (ExecutionException e) {
+      // Expected.
+    }
+    waitModel.countDown();
+  }
+
+  // Tests #2428.
+  @Test
+  public void submit_withRequestLoadingWithOnlyRetrieveFromCache_andNotInCache_doesNotFail() {
+    // Block the main thread so that we know that both requests will be queued but not started at
+    // the same time.
+    final CountDownLatch blockMainThread = new CountDownLatch(1);
+    new Handler(Looper.getMainLooper()).post(new Runnable() {
+      @Override
+      public void run() {
+         try {
+          blockMainThread.await();
+        } catch (InterruptedException e) {
+          throw new RuntimeException(e);
+        }
+      }
+    });
+
+    // Queue the retrieve from cache request first.
+    GlideApp.with(context)
+        .load(ResourceIds.raw.canonical)
+        .onlyRetrieveFromCache(true)
+        .submit();
+
+    // Then queue the normal request.
+    FutureTarget<Drawable> expectedFuture =
+        GlideApp.with(context).load(ResourceIds.raw.canonical).submit();
+
+    // Run the requests.
+    blockMainThread.countDown();
+
+    // Verify that the request that didn't have retrieve from cache succeeds
+    assertThat(concurrency.get(expectedFuture)).isNotNull();
+  }
 
   private void clearMemoryCacheOnMainThread() throws InterruptedException {
     concurrency.runOnMainThread(new Runnable() {

library/src/main/java/com/bumptech/glide/load/engine/Engine.java

@@ -1,5 +1,6 @@
 package com.bumptech.glide.load.engine;
 
+import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.support.annotation.VisibleForTesting;
 import android.support.v4.util.Pools;
@@ -20,7 +21,6 @@
 import com.bumptech.glide.util.Synthetic;
 import com.bumptech.glide.util.Util;
 import com.bumptech.glide.util.pool.FactoryPools;
-import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.TimeUnit;
@@ -33,7 +33,7 @@ public class Engine implements EngineJobListener,
     EngineResource.ResourceListener {
   private static final String TAG = "Engine";
   private static final int JOB_POOL_SIZE = 150;
-  private final Map<Key, EngineJob<?>> jobs;
+  private final Jobs jobs;
   private final EngineKeyFactory keyFactory;
   private final MemoryCache cache;
   private final EngineJobFactory engineJobFactory;
@@ -70,7 +70,7 @@ public Engine(MemoryCache memoryCache,
       GlideExecutor sourceExecutor,
       GlideExecutor sourceUnlimitedExecutor,
       GlideExecutor animationExecutor,
-      Map<Key, EngineJob<?>> jobs,
+      Jobs jobs,
       EngineKeyFactory keyFactory,
       ActiveResources activeResources,
       EngineJobFactory engineJobFactory,
@@ -91,7 +91,7 @@ public Engine(MemoryCache memoryCache,
     this.keyFactory = keyFactory;
 
     if (jobs == null) {
-      jobs = new HashMap<>();
+      jobs = new Jobs();
     }
     this.jobs = jobs;
 
@@ -177,7 +177,7 @@ public <R> LoadStatus load(
       return null;
     }
 
-    EngineJob<?> current = jobs.get(key);
+    EngineJob<?> current = jobs.get(key, onlyRetrieveFromCache);
     if (current != null) {
       current.addCallback(cb);
       if (Log.isLoggable(TAG, Log.VERBOSE)) {
@@ -186,26 +186,35 @@ public <R> LoadStatus load(
       return new LoadStatus(cb, current);
     }
 
-    EngineJob<R> engineJob = engineJobFactory.build(key, isMemoryCacheable,
-        useUnlimitedSourceExecutorPool, useAnimationPool);
-    DecodeJob<R> decodeJob = decodeJobFactory.build(
-        glideContext,
-        model,
-        key,
-        signature,
-        width,
-        height,
-        resourceClass,
-        transcodeClass,
-        priority,
-        diskCacheStrategy,
-        transformations,
-        isTransformationRequired,
-        isScaleOnlyOrNoTransform,
-        onlyRetrieveFromCache,
-        options,
-        engineJob);
+    EngineJob<R> engineJob =
+        engineJobFactory.build(
+            key,
+            isMemoryCacheable,
+            useUnlimitedSourceExecutorPool,
+            useAnimationPool,
+            onlyRetrieveFromCache);
+
+    DecodeJob<R> decodeJob =
+        decodeJobFactory.build(
+            glideContext,
+            model,
+            key,
+            signature,
+            width,
+            height,
+            resourceClass,
+            transcodeClass,
+            priority,
+            diskCacheStrategy,
+            transformations,
+            isTransformationRequired,
+            isScaleOnlyOrNoTransform,
+            onlyRetrieveFromCache,
+            options,
+            engineJob);
+
     jobs.put(key, engineJob);
+
     engineJob.addCallback(cb);
     engineJob.start(decodeJob);
 
@@ -232,7 +241,6 @@ private EngineResource<?> loadFromActiveResources(Key key, boolean isMemoryCache
     return active;
   }
 
-
   private EngineResource<?> loadFromCache(Key key, boolean isMemoryCacheable) {
     if (!isMemoryCacheable) {
       return null;
@@ -273,7 +281,7 @@ public void release(Resource<?> resource) {
 
   @SuppressWarnings("unchecked")
   @Override
-  public void onEngineJobComplete(Key key, EngineResource<?> resource) {
+  public void onEngineJobComplete(EngineJob<?> engineJob, Key key, EngineResource<?> resource) {
     Util.assertMainThread();
     // A null resource indicates that the load failed, usually due to an exception.
     if (resource != null) {
@@ -283,21 +291,19 @@ public void onEngineJobComplete(Key key, EngineResource<?> resource) {
         activeResources.activate(key, resource);
       }
     }
-    // TODO: should this check that the engine job is still current?
-    jobs.remove(key);
+
+    jobs.removeIfCurrent(key, engineJob);
   }
 
   @Override
   public void onEngineJobCancelled(EngineJob<?> engineJob, Key key) {
     Util.assertMainThread();
-    EngineJob<?> current = jobs.get(key);
-    if (engineJob.equals(current)) {
-      jobs.remove(key);
-    }
+
+    jobs.removeIfCurrent(key, engineJob);
   }
 
   @Override
-  public void onResourceRemoved(final Resource<?> resource) {
+  public void onResourceRemoved(@NonNull final Resource<?> resource) {
     Util.assertMainThread();
     resourceRecycler.recycle(resource);
   }
@@ -471,10 +477,19 @@ void shutdown() {
     }
 
     @SuppressWarnings("unchecked")
-    <R> EngineJob<R> build(Key key, boolean isMemoryCacheable,
-        boolean useUnlimitedSourceGeneratorPool, boolean useAnimationPool) {
+    <R> EngineJob<R> build(
+        Key key,
+        boolean isMemoryCacheable,
+        boolean useUnlimitedSourceGeneratorPool,
+        boolean useAnimationPool,
+        boolean onlyRetrieveFromCache) {
       EngineJob<R> result = Preconditions.checkNotNull((EngineJob<R>) pool.acquire());
-      return result.init(key, isMemoryCacheable, useUnlimitedSourceGeneratorPool, useAnimationPool);
+      return result.init(
+          key,
+          isMemoryCacheable,
+          useUnlimitedSourceGeneratorPool,
+          useAnimationPool,
+          onlyRetrieveFromCache);
     }
 
     private static void shutdownAndAwaitTermination(ExecutorService pool) {

library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java

@@ -46,6 +46,7 @@ class EngineJob<R> implements DecodeJob.Callback<R>,
   private boolean isCacheable;
   private boolean useUnlimitedSourceGeneratorPool;
   private boolean useAnimationPool;
+  private boolean onlyRetrieveFromCache;
   private Resource<?> resource;
   private DataSource dataSource;
   private boolean hasResource;
@@ -100,11 +101,13 @@ EngineJob<R> init(
       Key key,
       boolean isCacheable,
       boolean useUnlimitedSourceGeneratorPool,
-      boolean useAnimationPool) {
+      boolean useAnimationPool,
+      boolean onlyRetrieveFromCache) {
     this.key = key;
     this.isCacheable = isCacheable;
     this.useUnlimitedSourceGeneratorPool = useUnlimitedSourceGeneratorPool;
     this.useAnimationPool = useAnimationPool;
+    this.onlyRetrieveFromCache = onlyRetrieveFromCache;
     return this;
   }
 
@@ -128,7 +131,7 @@ void addCallback(ResourceCallback cb) {
     }
   }
 
-  public void removeCallback(ResourceCallback cb) {
+  void removeCallback(ResourceCallback cb) {
     Util.assertMainThread();
     stateVerifier.throwIfRecycled();
     if (hasResource || hasLoadFailed) {
@@ -141,6 +144,10 @@ public void removeCallback(ResourceCallback cb) {
     }
   }
 
+  boolean onlyRetrieveFromCache() {
+    return onlyRetrieveFromCache;
+  }
+
   private GlideExecutor getActiveSourceExecutor() {
     return useUnlimitedSourceGeneratorPool
         ? sourceUnlimitedExecutor : (useAnimationPool ? animationExecutor : sourceExecutor);
@@ -200,7 +207,7 @@ void handleResultOnMainThread() {
     // Hold on to resource for duration of request so we don't recycle it in the middle of
     // notifying if it synchronously released by one of the callbacks.
     engineResource.acquire();
-    listener.onEngineJobComplete(key, engineResource);
+    listener.onEngineJobComplete(this, key, engineResource);
 
     int size = cbs.size();
     for (int i = 0; i < size; i++) {
@@ -278,7 +285,7 @@ void handleExceptionOnMainThread() {
     }
     hasLoadFailed = true;
 
-    listener.onEngineJobComplete(key, null);
+    listener.onEngineJobComplete(this, key, null);
 
     for (ResourceCallback cb : cbs) {
       if (!isInIgnoredCallbacks(cb)) {

library/src/main/java/com/bumptech/glide/load/engine/EngineJobListener.java

@@ -4,7 +4,7 @@
 
 interface EngineJobListener {
 
-  void onEngineJobComplete(Key key, EngineResource<?> resource);
+  void onEngineJobComplete(EngineJob<?> engineJob, Key key, EngineResource<?> resource);
 
   void onEngineJobCancelled(EngineJob<?> engineJob, Key key);
 }

library/src/main/java/com/bumptech/glide/load/engine/EngineKey.java

@@ -21,9 +21,15 @@ class EngineKey implements Key {
   private final Options options;
   private int hashCode;
 
-  EngineKey(Object model, Key signature, int width, int height,
-      Map<Class<?>, Transformation<?>> transformations, Class<?> resourceClass,
-      Class<?> transcodeClass, Options options) {
+  EngineKey(
+      Object model,
+      Key signature,
+      int width,
+      int height,
+      Map<Class<?>, Transformation<?>> transformations,
+      Class<?> resourceClass,
+      Class<?> transcodeClass,
+      Options options) {
     this.model = Preconditions.checkNotNull(model);
     this.signature = Preconditions.checkNotNull(signature, "Signature must not be null");
     this.width = width;

library/src/main/java/com/bumptech/glide/load/engine/Jobs.java

@@ -0,0 +1,36 @@
+package com.bumptech.glide.load.engine;
+
+import android.support.annotation.VisibleForTesting;
+import com.bumptech.glide.load.Key;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+final class Jobs {
+  private final Map<Key, EngineJob<?>> jobs = new HashMap<>();
+  private final Map<Key, EngineJob<?>> onlyCacheJobs = new HashMap<>();
+
+  @VisibleForTesting
+  Map<Key, EngineJob<?>> getAll() {
+    return Collections.unmodifiableMap(jobs);
+  }
+
+  EngineJob<?> get(Key key, boolean onlyRetrieveFromCache) {
+    return getJobMap(onlyRetrieveFromCache).get(key);
+  }
+
+  void put(Key key, EngineJob<?> job) {
+    getJobMap(job.onlyRetrieveFromCache()).put(key, job);
+  }
+
+  void removeIfCurrent(Key key, EngineJob<?> expected) {
+    Map<Key, EngineJob<?>> jobMap = getJobMap(expected.onlyRetrieveFromCache());
+    if (expected.equals(jobMap.get(key))) {
+      jobMap.remove(key);
+    }
+  }
+
+  private Map<Key, EngineJob<?>> getJobMap(boolean onlyRetrieveFromCache) {
+    return onlyRetrieveFromCache ? onlyCacheJobs : jobs;
+  }
+}