From 53438b90f982d27652c2bae7cd9daf9be8d5a03a Mon Sep 17 00:00:00 2001 From: judds Date: Wed, 20 Feb 2019 12:18:12 -0800 Subject: [PATCH] Require ResourceListener in the constructor of EngineResource ResourceListener is supposed to be effectively final in EngineResource. It was only passed into a separate method to reduce the number of pass throughs. Since we're now loading resources on multiple threads, it's no longer trivially safe to call an additional method to set the listener. Specifically there's a race where the EngineJob exposes the Resource to any new requests for the same image before the Engine has a chance to set the ResourceListener on the EngineResource. The locking to fix this is complicated and would require holding multiple locks simultaneously. Instead this cl moves the ResourceListener into the EngineResource constructor and performs the necessary pass throughs to make that happen. Ideally this is safer and more performant than adjusting the locking. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=234844998 --- .../glide/load/engine/ActiveResources.java | 12 ++- .../bumptech/glide/load/engine/Engine.java | 29 +++--- .../bumptech/glide/load/engine/EngineJob.java | 30 ++++--- .../glide/load/engine/EngineResource.java | 26 +++--- .../load/engine/ActiveResourcesTest.java | 88 ++++++++----------- .../glide/load/engine/EngineJobTest.java | 40 +++++---- .../glide/load/engine/EngineResourceTest.java | 30 +++++-- .../glide/load/engine/EngineTest.java | 23 ++--- 8 files changed, 151 insertions(+), 127 deletions(-) diff --git a/library/src/main/java/com/bumptech/glide/load/engine/ActiveResources.java b/library/src/main/java/com/bumptech/glide/load/engine/ActiveResources.java index 9b25b8a97c..16e548732e 100644 --- a/library/src/main/java/com/bumptech/glide/load/engine/ActiveResources.java +++ b/library/src/main/java/com/bumptech/glide/load/engine/ActiveResources.java @@ -118,8 +118,12 @@ void cleanupActiveReference(@NonNull ResourceWeakReference ref) { return; } EngineResource newResource = - new EngineResource<>(ref.resource, /*isCacheable=*/ true, /*isRecyclable=*/ false); - newResource.setResourceListener(ref.key, listener); + new EngineResource<>( + ref.resource, + /*isMemoryCacheable=*/ true, + /*isRecyclable=*/ false, + ref.key, + listener); listener.onResourceReleased(ref.key, newResource); } } @@ -180,9 +184,9 @@ static final class ResourceWeakReference extends WeakReference super(referent, queue); this.key = Preconditions.checkNotNull(key); this.resource = - referent.isCacheable() && isActiveResourceRetentionAllowed + referent.isMemoryCacheable() && isActiveResourceRetentionAllowed ? Preconditions.checkNotNull(referent.getResource()) : null; - isCacheable = referent.isCacheable(); + isCacheable = referent.isMemoryCacheable(); } void reset() { diff --git a/library/src/main/java/com/bumptech/glide/load/engine/Engine.java b/library/src/main/java/com/bumptech/glide/load/engine/Engine.java index f3050c8b72..e4d88b7fe7 100644 --- a/library/src/main/java/com/bumptech/glide/load/engine/Engine.java +++ b/library/src/main/java/com/bumptech/glide/load/engine/Engine.java @@ -11,6 +11,7 @@ import com.bumptech.glide.load.Key; import com.bumptech.glide.load.Options; import com.bumptech.glide.load.Transformation; +import com.bumptech.glide.load.engine.EngineResource.ResourceListener; import com.bumptech.glide.load.engine.cache.DiskCache; import com.bumptech.glide.load.engine.cache.DiskCacheAdapter; import com.bumptech.glide.load.engine.cache.MemoryCache; @@ -102,7 +103,12 @@ public Engine( if (engineJobFactory == null) { engineJobFactory = new EngineJobFactory( - diskCacheExecutor, sourceExecutor, sourceUnlimitedExecutor, animationExecutor, this); + diskCacheExecutor, + sourceExecutor, + sourceUnlimitedExecutor, + animationExecutor, + /*engineJobListener=*/ this, + /*resourceListener=*/ this); } this.engineJobFactory = engineJobFactory; @@ -276,7 +282,8 @@ private EngineResource getEngineResourceFromCache(Key key) { // Save an object allocation if we've cached an EngineResource (the typical case). result = (EngineResource) cached; } else { - result = new EngineResource<>(cached, true /*isMemoryCacheable*/, true /*isRecyclable*/); + result = new EngineResource<>( + cached, /*isMemoryCacheable=*/ true, /*isRecyclable=*/ true, key, /*listener=*/ this); } return result; } @@ -295,9 +302,7 @@ public synchronized void onEngineJobComplete( EngineJob engineJob, Key key, EngineResource resource) { // A null resource indicates that the load failed, usually due to an exception. if (resource != null) { - resource.setResourceListener(key, this); - - if (resource.isCacheable()) { + if (resource.isMemoryCacheable()) { activeResources.activate(key, resource); } } @@ -318,7 +323,7 @@ public void onResourceRemoved(@NonNull final Resource resource) { @Override public synchronized void onResourceReleased(Key cacheKey, EngineResource resource) { activeResources.deactivate(cacheKey); - if (resource.isCacheable()) { + if (resource.isMemoryCacheable()) { cache.put(cacheKey, resource); } else { resourceRecycler.recycle(resource); @@ -456,7 +461,8 @@ static class EngineJobFactory { @Synthetic final GlideExecutor sourceExecutor; @Synthetic final GlideExecutor sourceUnlimitedExecutor; @Synthetic final GlideExecutor animationExecutor; - @Synthetic final EngineJobListener listener; + @Synthetic final EngineJobListener engineJobListener; + @Synthetic final ResourceListener resourceListener; @Synthetic final Pools.Pool> pool = FactoryPools.threadSafe( JOB_POOL_SIZE, @@ -468,7 +474,8 @@ public EngineJob create() { sourceExecutor, sourceUnlimitedExecutor, animationExecutor, - listener, + engineJobListener, + resourceListener, pool); } }); @@ -478,12 +485,14 @@ public EngineJob create() { GlideExecutor sourceExecutor, GlideExecutor sourceUnlimitedExecutor, GlideExecutor animationExecutor, - EngineJobListener listener) { + EngineJobListener engineJobListener, + ResourceListener resourceListener) { this.diskCacheExecutor = diskCacheExecutor; this.sourceExecutor = sourceExecutor; this.sourceUnlimitedExecutor = sourceUnlimitedExecutor; this.animationExecutor = animationExecutor; - this.listener = listener; + this.engineJobListener = engineJobListener; + this.resourceListener = resourceListener; } @VisibleForTesting diff --git a/library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java b/library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java index c3bd7358e7..0a1846c957 100644 --- a/library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java +++ b/library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java @@ -5,6 +5,7 @@ import android.support.v4.util.Pools; import com.bumptech.glide.load.DataSource; import com.bumptech.glide.load.Key; +import com.bumptech.glide.load.engine.EngineResource.ResourceListener; import com.bumptech.glide.load.engine.executor.GlideExecutor; import com.bumptech.glide.request.ResourceCallback; import com.bumptech.glide.util.Executors; @@ -31,9 +32,10 @@ class EngineJob implements DecodeJob.Callback, final ResourceCallbacksAndExecutors cbs = new ResourceCallbacksAndExecutors(); private final StateVerifier stateVerifier = StateVerifier.newInstance(); + private final ResourceListener resourceListener; private final Pools.Pool> pool; private final EngineResourceFactory engineResourceFactory; - private final EngineJobListener listener; + private final EngineJobListener engineJobListener; private final GlideExecutor diskCacheExecutor; private final GlideExecutor sourceExecutor; private final GlideExecutor sourceUnlimitedExecutor; @@ -73,14 +75,16 @@ class EngineJob implements DecodeJob.Callback, GlideExecutor sourceExecutor, GlideExecutor sourceUnlimitedExecutor, GlideExecutor animationExecutor, - EngineJobListener listener, + EngineJobListener engineJobListener, + ResourceListener resourceListener, Pools.Pool> pool) { this( diskCacheExecutor, sourceExecutor, sourceUnlimitedExecutor, animationExecutor, - listener, + engineJobListener, + resourceListener, pool, DEFAULT_FACTORY); } @@ -91,14 +95,16 @@ class EngineJob implements DecodeJob.Callback, GlideExecutor sourceExecutor, GlideExecutor sourceUnlimitedExecutor, GlideExecutor animationExecutor, - EngineJobListener listener, + EngineJobListener engineJobListener, + ResourceListener resourceListener, Pools.Pool> pool, EngineResourceFactory engineResourceFactory) { this.diskCacheExecutor = diskCacheExecutor; this.sourceExecutor = sourceExecutor; this.sourceUnlimitedExecutor = sourceUnlimitedExecutor; this.animationExecutor = animationExecutor; - this.listener = listener; + this.engineJobListener = engineJobListener; + this.resourceListener = resourceListener; this.pool = pool; this.engineResourceFactory = engineResourceFactory; } @@ -197,7 +203,7 @@ void cancel() { isCancelled = true; decodeJob.cancel(); - listener.onEngineJobCancelled(this, key); + engineJobListener.onEngineJobCancelled(this, key); } // Exposed for testing. @@ -231,7 +237,7 @@ void notifyCallbacksOfResult() { } else if (hasResource) { throw new IllegalStateException("Already have resource"); } - engineResource = engineResourceFactory.build(resource, isCacheable); + engineResource = engineResourceFactory.build(resource, isCacheable, key, resourceListener); // Hold on to resource for duration of our callbacks below so we don't recycle it in the // middle of notifying if it synchronously released by one of the callbacks. Acquire it under // a lock here so that any newly added callback that executes before the next locked section @@ -244,7 +250,7 @@ void notifyCallbacksOfResult() { localResource = engineResource; } - listener.onEngineJobComplete(this, localKey, localResource); + engineJobListener.onEngineJobComplete(this, localKey, localResource); for (final ResourceCallbackAndExecutor entry : copy) { entry.executor.execute(new CallResourceReady(entry.cb)); @@ -347,7 +353,7 @@ void notifyCallbacksOfException() { incrementPendingCallbacks(copy.size() + 1); } - listener.onEngineJobComplete(this, localKey, /*resource=*/ null); + engineJobListener.onEngineJobComplete(this, localKey, /*resource=*/ null); for (ResourceCallbackAndExecutor entry : copy) { entry.executor.execute(new CallLoadFailed(entry.cb)); @@ -480,8 +486,10 @@ public int hashCode() { @VisibleForTesting static class EngineResourceFactory { - public EngineResource build(Resource resource, boolean isMemoryCacheable) { - return new EngineResource<>(resource, isMemoryCacheable, /*isRecyclable=*/ true); + public EngineResource build( + Resource resource, boolean isMemoryCacheable, Key key, ResourceListener listener) { + return new EngineResource<>( + resource, isMemoryCacheable, /*isRecyclable=*/ true, key, listener); } } } diff --git a/library/src/main/java/com/bumptech/glide/load/engine/EngineResource.java b/library/src/main/java/com/bumptech/glide/load/engine/EngineResource.java index 2ff397bdcf..4db3a1db76 100644 --- a/library/src/main/java/com/bumptech/glide/load/engine/EngineResource.java +++ b/library/src/main/java/com/bumptech/glide/load/engine/EngineResource.java @@ -11,12 +11,12 @@ * @param The type of data returned by the wrapped {@link Resource}. */ class EngineResource implements Resource { - private final boolean isCacheable; + private final boolean isMemoryCacheable; private final boolean isRecyclable; private final Resource resource; + private final ResourceListener listener; + private final Key key; - private ResourceListener listener; - private Key key; private int acquired; private boolean isRecycled; @@ -24,23 +24,25 @@ interface ResourceListener { void onResourceReleased(Key key, EngineResource resource); } - EngineResource(Resource toWrap, boolean isCacheable, boolean isRecyclable) { + EngineResource( + Resource toWrap, + boolean isMemoryCacheable, + boolean isRecyclable, + Key key, + ResourceListener listener) { resource = Preconditions.checkNotNull(toWrap); - this.isCacheable = isCacheable; + this.isMemoryCacheable = isMemoryCacheable; this.isRecyclable = isRecyclable; - } - - synchronized void setResourceListener(Key key, ResourceListener listener) { this.key = key; - this.listener = listener; + this.listener = Preconditions.checkNotNull(listener); } Resource getResource() { return resource; } - boolean isCacheable() { - return isCacheable; + boolean isMemoryCacheable() { + return isMemoryCacheable; } @NonNull @@ -119,7 +121,7 @@ void release() { @Override public synchronized String toString() { return "EngineResource{" - + "isCacheable=" + isCacheable + + "isMemoryCacheable=" + isMemoryCacheable + ", listener=" + listener + ", key=" + key + ", acquired=" + acquired diff --git a/library/test/src/test/java/com/bumptech/glide/load/engine/ActiveResourcesTest.java b/library/test/src/test/java/com/bumptech/glide/load/engine/ActiveResourcesTest.java index a467ea7747..acc1b6a305 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/engine/ActiveResourcesTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/engine/ActiveResourcesTest.java @@ -7,6 +7,7 @@ import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; +import android.os.Looper; import android.support.annotation.NonNull; import com.bumptech.glide.load.Key; import com.bumptech.glide.load.engine.ActiveResources.DequeuedResourceCallback; @@ -26,8 +27,8 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.robolectric.RobolectricTestRunner; +import org.robolectric.Shadows; import org.robolectric.annotation.Config; -import org.robolectric.shadows.ShadowLooper; @RunWith(RobolectricTestRunner.class) @Config(shadows = GlideShadowLooper.class) @@ -60,16 +61,14 @@ public void get_withMissingKey_returnsNull() { @Test public void get_withActiveKey_returnsResource() { - EngineResource expected = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource expected = newCacheableEngineResource(); resources.activate(key, expected); assertThat(resources.get(key)).isEqualTo(expected); } @Test public void get_withDeactivatedKey_returnsNull() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); resources.deactivate(key); assertThat(resources.get(key)).isNull(); @@ -82,8 +81,7 @@ public void deactivate_withNotActiveKey_doesNotThrow() { @Test public void get_withActiveAndClearedKey_returnsNull() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); resources.activeEngineResources.get(key).clear(); assertThat(resources.get(key)).isNull(); @@ -91,8 +89,7 @@ public void get_withActiveAndClearedKey_returnsNull() { @Test public void get_withActiveAndClearedKey_andCacheableResource_callsListenerWithWrappedResource() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); resources.activeEngineResources.get(key).clear(); resources.get(key); @@ -106,8 +103,7 @@ public void get_withActiveAndClearedKey_andCacheableResource_callsListenerWithWr @Test public void get_withActiveAndClearedKey_andCacheableResource_callsListenerWithNotRecycleable() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); resources.activeEngineResources.get(key).clear(); resources.get(key); @@ -122,8 +118,7 @@ public void get_withActiveAndClearedKey_andCacheableResource_callsListenerWithNo @Test public void get_withActiveAndClearedKey_andCacheableResource_callsListenerWithCacheable() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); resources.activeEngineResources.get(key).clear(); resources.get(key); @@ -132,13 +127,12 @@ public void get_withActiveAndClearedKey_andCacheableResource_callsListenerWithCa verify(listener).onResourceReleased(eq(key), captor.capture()); - assertThat(captor.getValue().isCacheable()).isTrue(); + assertThat(captor.getValue().isMemoryCacheable()).isTrue(); } @Test public void get_withActiveAndClearedKey_andNotCacheableResource_doesNotCallListener() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ false, /*isRecyclable=*/ true); + EngineResource engineResource = newNonCacheableEngineResource(); resources.activate(key, engineResource); resources.activeEngineResources.get(key).clear(); resources.get(key); @@ -148,8 +142,7 @@ public void get_withActiveAndClearedKey_andNotCacheableResource_doesNotCallListe @Test public void queueIdle_afterResourceRemovedFromActive_doesNotCallListener() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -162,8 +155,7 @@ public void queueIdle_afterResourceRemovedFromActive_doesNotCallListener() { @Test public void queueIdle_withCacheableResourceInActive_callListener() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -175,7 +167,7 @@ public void queueIdle_withCacheableResourceInActive_callListener() { EngineResource released = captor.getValue(); assertThat(released.getResource()).isEqualTo(resource); - assertThat(released.isCacheable()).isTrue(); + assertThat(released.isMemoryCacheable()).isTrue(); released.recycle(); verify(resource, never()).recycle(); @@ -183,8 +175,7 @@ public void queueIdle_withCacheableResourceInActive_callListener() { @Test public void queueIdle_withNotCacheableResourceInActive_doesNotCallListener() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ false, /*isRecyclable=*/ true); + EngineResource engineResource = newNonCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -196,8 +187,7 @@ public void queueIdle_withNotCacheableResourceInActive_doesNotCallListener() { @Test public void queueIdle_withCacheableResourceInActive_removesResourceFromActive() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -208,8 +198,7 @@ public void queueIdle_withCacheableResourceInActive_removesResourceFromActive() @Test public void queueIdle_withNotCacheableResourceInActive_removesResourceFromActive() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ false, /*isRecyclable=*/ true); + EngineResource engineResource = newNonCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -220,8 +209,7 @@ public void queueIdle_withNotCacheableResourceInActive_removesResourceFromActive @Test public void get_withQueuedReference_returnsResource() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -232,8 +220,7 @@ public void get_withQueuedReference_returnsResource() { @Test public void get_withQueuedReference_doesNotNotifyListener() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -244,8 +231,7 @@ public void get_withQueuedReference_doesNotNotifyListener() { @Test public void queueIdle_withQueuedReferenceRetrievedFromGet_notifiesListener() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -261,8 +247,7 @@ public void queueIdle_withQueuedReferenceRetrievedFromGet_notifiesListener() { @Test public void queueIdle_withQueuedReferenceRetrievedFromGetAndNotCacheable_doesNotNotifyListener() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ false, /*isRecyclable=*/ true); + EngineResource engineResource = newNonCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -303,8 +288,7 @@ public void run() { }); resources.setListener(listener); - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -349,16 +333,14 @@ public void run() { }); resources.setListener(listener); - EngineResource first = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource first = newCacheableEngineResource(); resources.activate(key, first); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); CountDownLatch latch = getLatchForClearedRef(); weakRef.enqueue(); - EngineResource second = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource second = newCacheableEngineResource(); resources.activate(key, second); blockExecutor.countDown(); @@ -373,8 +355,7 @@ public void run() { @Test public void activate_withNonCacheableResource_doesNotSaveResource() { - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ false, /*isRecyclable=*/ true); + EngineResource engineResource = newNonCacheableEngineResource(); resources.activate(key, engineResource); assertThat(resources.activeEngineResources.get(key).resource).isNull(); @@ -384,8 +365,7 @@ public void activate_withNonCacheableResource_doesNotSaveResource() { public void get_withActiveClearedKey_cacheableResource_retentionDisabled_doesNotCallListener() { resources = new ActiveResources(/*isActiveResourceRetentionAllowed=*/ false); resources.setListener(listener); - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); resources.activeEngineResources.get(key).clear(); resources.get(key); @@ -397,8 +377,7 @@ public void get_withActiveClearedKey_cacheableResource_retentionDisabled_doesNot public void get_withQueuedReference_retentionDisabled_returnsResource() { resources = new ActiveResources(/*isActiveResourceRetentionAllowed=*/ false); resources.setListener(listener); - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -411,8 +390,7 @@ public void get_withQueuedReference_retentionDisabled_returnsResource() { public void queueIdle_withQueuedReferenceRetrievedFromGet_retentionDisabled_doesNotNotify() { resources = new ActiveResources(/*isActiveResourceRetentionAllowed=*/ false); resources.setListener(listener); - EngineResource engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); + EngineResource engineResource = newCacheableEngineResource(); resources.activate(key, engineResource); ResourceWeakReference weakRef = resources.activeEngineResources.get(key); @@ -438,7 +416,7 @@ private void waitForLatch(CountDownLatch latch) { } catch (InterruptedException e) { throw new RuntimeException(e); } - ShadowLooper.getShadowMainLooper().runToEndOfTasks(); + Shadows.shadowOf(Looper.getMainLooper()).runToEndOfTasks(); } private CountDownLatch getLatchForClearedRef() { @@ -452,6 +430,16 @@ public void onResourceDequeued() { return toWait; } + private EngineResource newCacheableEngineResource() { + return new EngineResource<>( + resource, /*isMemoryCacheable=*/ true, /*isRecyclable=*/ false, key, listener); + } + + private EngineResource newNonCacheableEngineResource() { + return new EngineResource<>( + resource, /*isMemoryCacheable=*/ false, /*isRecyclable=*/ false, key, listener); + } + @SuppressWarnings("unchecked") private static ArgumentCaptor> getEngineResourceCaptor() { return (ArgumentCaptor>) (ArgumentCaptor) diff --git a/library/test/src/test/java/com/bumptech/glide/load/engine/EngineJobTest.java b/library/test/src/test/java/com/bumptech/glide/load/engine/EngineJobTest.java index d6bb580ed7..51368f8a7b 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/engine/EngineJobTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/engine/EngineJobTest.java @@ -21,6 +21,7 @@ import android.support.v4.util.Pools; import com.bumptech.glide.load.DataSource; import com.bumptech.glide.load.Key; +import com.bumptech.glide.load.engine.EngineResource.ResourceListener; import com.bumptech.glide.load.engine.executor.GlideExecutor; import com.bumptech.glide.load.engine.executor.MockGlideExecutor; import com.bumptech.glide.request.ResourceCallback; @@ -66,7 +67,7 @@ public void testListenerNotifiedJobCompleteOnOnResourceReady() { ShadowLooper.runUiThreadTasks(); - verify(harness.listener) + verify(harness.engineJobListener) .onEngineJobComplete(eq(job), eq(harness.key), eq(harness.engineResource)); } @@ -110,7 +111,7 @@ public void testListenerNotifiedJobCompleteOnException() { job.start(harness.decodeJob); job.onLoadFailed(new GlideException("test")); ShadowLooper.runUiThreadTasks(); - verify(harness.listener) + verify(harness.engineJobListener) .onEngineJobComplete(eq(job), eq(harness.key), isNull(EngineResource.class)); } @@ -122,7 +123,9 @@ public void testResourceIsCacheableWhenIsCacheableOnReady() { job.onResourceReady(harness.resource, harness.dataSource); ShadowLooper.runUiThreadTasks(); - verify(harness.factory).build(anyResource(), eq(harness.isCacheable)); + verify(harness.factory) + .build( + anyResource(), eq(harness.isCacheable), eq(harness.key), eq(harness.resourceListener)); } @Test @@ -133,7 +136,9 @@ public void testResourceIsCacheableWhenNotIsCacheableOnReady() { job.onResourceReady(harness.resource, harness.dataSource); ShadowLooper.runUiThreadTasks(); - verify(harness.factory).build(anyResource(), eq(harness.isCacheable)); + verify(harness.factory) + .build( + anyResource(), eq(harness.isCacheable), eq(harness.key), eq(harness.resourceListener)); } @Test @@ -142,7 +147,7 @@ public void testListenerNotifiedOfCancelOnCancel() { job.start(harness.decodeJob); job.cancel(); - verify(harness.listener).onEngineJobCancelled(eq(job), eq(harness.key)); + verify(harness.engineJobListener).onEngineJobCancelled(eq(job), eq(harness.key)); } @Test @@ -205,7 +210,7 @@ public void testDoesNotNotifyCancelledIfCompletes() { job.start(harness.decodeJob); job.onResourceReady(harness.resource, harness.dataSource); - verify(harness.listener, never()).onEngineJobCancelled(eq(job), eq(harness.key)); + verify(harness.engineJobListener, never()).onEngineJobCancelled(eq(job), eq(harness.key)); } @Test @@ -215,7 +220,7 @@ public void testDoesNotNotifyCancelledIfAlreadyCancelled() { job.cancel(); job.cancel(); - verify(harness.listener, times(1)).onEngineJobCancelled(eq(job), eq(harness.key)); + verify(harness.engineJobListener, times(1)).onEngineJobCancelled(eq(job), eq(harness.key)); } @Test @@ -224,9 +229,10 @@ public void testDoesNotNotifyCancelledIfReceivedException() { job.start(harness.decodeJob); job.onLoadFailed(new GlideException("test")); - verify(harness.listener) + verify(harness.engineJobListener) .onEngineJobComplete(eq(job), eq(harness.key), isNull(EngineResource.class)); - verify(harness.listener, never()).onEngineJobCancelled(any(EngineJob.class), any(Key.class)); + verify(harness.engineJobListener, never()) + .onEngineJobCancelled(any(EngineJob.class), any(Key.class)); } @Test @@ -481,7 +487,8 @@ private static class MultiCbHarness { final Key key = mock(Key.class); final Resource resource = mockResource(); final EngineResource engineResource = mock(EngineResource.class); - final EngineJobListener listener = mock(EngineJobListener.class); + final EngineJobListener engineJobListener = mock(EngineJobListener.class); + final ResourceListener resourceListener = mock(ResourceListener.class); final boolean isCacheable = true; final boolean useUnlimitedSourceGeneratorPool = false; final boolean useAnimationPool = false; @@ -499,14 +506,15 @@ private static class MultiCbHarness { final DataSource dataSource = DataSource.LOCAL; public MultiCbHarness() { - when(factory.build(eq(resource), eq(isCacheable))).thenReturn(engineResource); + when(factory.build(resource, isCacheable, key, resourceListener)).thenReturn(engineResource); job = new EngineJob<>( diskCacheService, sourceService, sourceUnlimitedService, animationService, - listener, + engineJobListener, + resourceListener, pool, factory); job.init( @@ -532,7 +540,8 @@ private static class EngineJobHarness { final ResourceCallback cb = mock(ResourceCallback.class); final Resource resource = mockResource(); final EngineResource engineResource = mock(EngineResource.class); - final EngineJobListener listener = mock(EngineJobListener.class); + final EngineJobListener engineJobListener = mock(EngineJobListener.class); + final ResourceListener resourceListener = mock(ResourceListener.class); final GlideExecutor diskCacheService = MockGlideExecutor.newMainThreadExecutor(); final GlideExecutor sourceService = MockGlideExecutor.newMainThreadExecutor(); final GlideExecutor sourceUnlimitedService = MockGlideExecutor.newMainThreadExecutor(); @@ -546,14 +555,15 @@ private static class EngineJobHarness { final DataSource dataSource = DataSource.DATA_DISK_CACHE; EngineJob getJob() { - when(factory.build(eq(resource), eq(isCacheable))).thenReturn(engineResource); + when(factory.build(resource, isCacheable, key, resourceListener)).thenReturn(engineResource); EngineJob result = new EngineJob<>( diskCacheService, sourceService, sourceUnlimitedService, animationService, - listener, + engineJobListener, + resourceListener, pool, factory); result.init( diff --git a/library/test/src/test/java/com/bumptech/glide/load/engine/EngineResourceTest.java b/library/test/src/test/java/com/bumptech/glide/load/engine/EngineResourceTest.java index 3468931eb1..954e4f1b7b 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/engine/EngineResourceTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/engine/EngineResourceTest.java @@ -32,8 +32,8 @@ public class EngineResourceTest { public void setUp() { MockitoAnnotations.initMocks(this); engineResource = - new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ true); - engineResource.setResourceListener(cacheKey, listener); + new EngineResource<>( + resource, /*isMemoryCacheable=*/ true, /*isRecyclable=*/ true, cacheKey, listener); } @Test @@ -143,24 +143,36 @@ public void testThrowsIfReleasedMoreThanAcquired() { @Test(expected = NullPointerException.class) public void testThrowsIfWrappedResourceIsNull() { - new EngineResource<>(/*toWrap=*/ null, /*isCacheable=*/ false, /*isRecyclable=*/ true); + new EngineResource<>( + /*toWrap=*/ null, /*isMemoryCacheable=*/ false, /*isRecyclable=*/ true, cacheKey, listener); } @Test public void testCanSetAndGetIsCacheable() { engineResource = - new EngineResource<>(mockResource(), /*isCacheable=*/ true, /*isRecyclable=*/ true); - assertTrue(engineResource.isCacheable()); + new EngineResource<>( + mockResource(), + /*isMemoryCacheable=*/ true, + /*isRecyclable=*/ true, + cacheKey, + listener); + assertTrue(engineResource.isMemoryCacheable()); engineResource = - new EngineResource<>(mockResource(), /*isCacheable=*/ false, /*isRecyclable=*/ true); - assertFalse(engineResource.isCacheable()); + new EngineResource<>( + mockResource(), + /*isMemoryCacheable=*/ false, + /*isRecyclable=*/ true, + cacheKey, + listener); + assertFalse(engineResource.isMemoryCacheable()); } @Test public void release_whenNotRecycleable_doesNotRecycleResource() { resource = mockResource(); - engineResource = new EngineResource<>(resource, /*isCacheable=*/ true, /*isRecyclable=*/ false); - engineResource.setResourceListener(cacheKey, listener); + engineResource = + new EngineResource<>( + resource, /*isMemoryCacheable=*/ true, /*isRecyclable=*/ false, cacheKey, listener); engineResource.recycle(); verify(listener, never()).onResourceReleased(any(Key.class), any(EngineResource.class)); diff --git a/library/test/src/test/java/com/bumptech/glide/load/engine/EngineTest.java b/library/test/src/test/java/com/bumptech/glide/load/engine/EngineTest.java index 207d7abcc1..172c8b1d71 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/engine/EngineTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/engine/EngineTest.java @@ -267,15 +267,6 @@ public void testRunnerIsRemovedFromRunnersOnEngineNotifiedJobComplete() { assertThat(harness.jobs.getAll()).doesNotContainKey(harness.cacheKey); } - @Test - public void testEngineIsSetAsResourceListenerOnJobComplete() { - harness.doLoad(); - - harness.callOnEngineJobComplete(); - - verify(harness.resource).setResourceListener(eq(harness.cacheKey), eq(harness.getEngine())); - } - @Test public void testEngineIsNotSetAsResourceListenerIfResourceIsNullOnJobComplete() { harness.doLoad(); @@ -285,7 +276,7 @@ public void testEngineIsNotSetAsResourceListenerIfResourceIsNullOnJobComplete() @Test public void testResourceIsAddedToActiveResourcesOnEngineComplete() { - when(harness.resource.isCacheable()).thenReturn(true); + when(harness.resource.isMemoryCacheable()).thenReturn(true); harness.callOnEngineJobComplete(); EngineResource resource = harness.activeResources.get(harness.cacheKey); @@ -300,7 +291,7 @@ public void testDoesNotPutNullResourceInActiveResourcesOnEngineComplete() { @Test public void testDoesNotPutResourceThatIsNotCacheableInActiveResourcesOnEngineComplete() { - when(harness.resource.isCacheable()).thenReturn(false); + when(harness.resource.isMemoryCacheable()).thenReturn(false); harness.callOnEngineJobComplete(); assertThat(harness.activeResources.get(harness.cacheKey)).isNull(); } @@ -326,7 +317,7 @@ public void testJobIsNotRemovedFromJobsIfOldJobIsCancelled() { @Test public void testResourceIsAddedToCacheOnReleased() { final Object expected = new Object(); - when(harness.resource.isCacheable()).thenReturn(true); + when(harness.resource.isMemoryCacheable()).thenReturn(true); when(harness.resource.get()).thenReturn(expected); doAnswer( new Answer() { @@ -347,7 +338,7 @@ public Void answer(InvocationOnMock invocationOnMock) { @Test public void testResourceIsNotAddedToCacheOnReleasedIfNotCacheable() { - when(harness.resource.isCacheable()).thenReturn(false); + when(harness.resource.isMemoryCacheable()).thenReturn(false); harness.getEngine().onResourceReleased(harness.cacheKey, harness.resource); verify(harness.cache, never()).put(eq(harness.cacheKey), eq(harness.resource)); @@ -355,7 +346,7 @@ public void testResourceIsNotAddedToCacheOnReleasedIfNotCacheable() { @Test public void testResourceIsRecycledIfNotCacheableWhenReleased() { - when(harness.resource.isCacheable()).thenReturn(false); + when(harness.resource.isMemoryCacheable()).thenReturn(false); harness.getEngine().onResourceReleased(harness.cacheKey, harness.resource); verify(harness.resourceRecycler).recycle(eq(harness.resource)); } @@ -446,7 +437,7 @@ public void runTest() { @Test public void load_afterResourceIsLoadedInActiveResources_returnsFromMemoryCache() { - when(harness.resource.isCacheable()).thenReturn(true); + when(harness.resource.isMemoryCacheable()).thenReturn(true); doAnswer( new Answer() { @Override @@ -465,7 +456,7 @@ public Object answer(InvocationOnMock invocationOnMock) { @Test public void load_afterResourceIsLoadedAndReleased_returnsFromMemoryCache() { harness.cache = new LruResourceCache(100); - when(harness.resource.isCacheable()).thenReturn(true); + when(harness.resource.isMemoryCacheable()).thenReturn(true); doAnswer( new Answer() { @Override