Skip to content

Commit

Permalink
Require ResourceListener in the constructor of EngineResource
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@sjudd
sjudd committed Mar 7, 2019
1 parent c6ffb04 commit 53438b9
Show file tree
Hide file tree
Showing 8 changed files with 151 additions and 127 deletions.
12 changes: 8 additions & 4 deletions library/src/main/java/com/bumptech/glide/load/engine/ActiveResources.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -180,9 +184,9 @@ static final class ResourceWeakReference extends WeakReference<EngineResource<?>
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() {
Expand Down
29 changes: 19 additions & 10 deletions library/src/main/java/com/bumptech/glide/load/engine/Engine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
Expand All @@ -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);
}
}
Expand All @@ -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);
Expand Down Expand Up @@ -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<EngineJob<?>> pool =
FactoryPools.threadSafe(
JOB_POOL_SIZE,
Expand All @@ -468,7 +474,8 @@ public EngineJob<?> create() {
sourceExecutor,
sourceUnlimitedExecutor,
animationExecutor,
listener,
engineJobListener,
resourceListener,
pool);
}
});
Expand All @@ -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
Expand Down
30 changes: 19 additions & 11 deletions library/src/main/java/com/bumptech/glide/load/engine/EngineJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -31,9 +32,10 @@ class EngineJob<R> implements DecodeJob.Callback<R>,
final ResourceCallbacksAndExecutors cbs = new ResourceCallbacksAndExecutors();

private final StateVerifier stateVerifier = StateVerifier.newInstance();
private final ResourceListener resourceListener;
private final Pools.Pool<EngineJob<?>> 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;
Expand Down Expand Up @@ -73,14 +75,16 @@ class EngineJob<R> implements DecodeJob.Callback<R>,
GlideExecutor sourceExecutor,
GlideExecutor sourceUnlimitedExecutor,
GlideExecutor animationExecutor,
EngineJobListener listener,
EngineJobListener engineJobListener,
ResourceListener resourceListener,
Pools.Pool<EngineJob<?>> pool) {
this(
diskCacheExecutor,
sourceExecutor,
sourceUnlimitedExecutor,
animationExecutor,
listener,
engineJobListener,
resourceListener,
pool,
DEFAULT_FACTORY);
}
Expand All @@ -91,14 +95,16 @@ class EngineJob<R> implements DecodeJob.Callback<R>,
GlideExecutor sourceExecutor,
GlideExecutor sourceUnlimitedExecutor,
GlideExecutor animationExecutor,
EngineJobListener listener,
EngineJobListener engineJobListener,
ResourceListener resourceListener,
Pools.Pool<EngineJob<?>> 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;
}
Expand Down Expand Up @@ -197,7 +203,7 @@ void cancel() {

isCancelled = true;
decodeJob.cancel();
listener.onEngineJobCancelled(this, key);
engineJobListener.onEngineJobCancelled(this, key);
}

// Exposed for testing.
Expand Down Expand Up @@ -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
Expand All @@ -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));
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -480,8 +486,10 @@ public int hashCode() {

@VisibleForTesting
static class EngineResourceFactory {
public <R> EngineResource<R> build(Resource<R> resource, boolean isMemoryCacheable) {
return new EngineResource<>(resource, isMemoryCacheable, /*isRecyclable=*/ true);
public <R> EngineResource<R> build(
Resource<R> resource, boolean isMemoryCacheable, Key key, ResourceListener listener) {
return new EngineResource<>(
resource, isMemoryCacheable, /*isRecyclable=*/ true, key, listener);
}
}
}
26 changes: 14 additions & 12 deletions library/src/main/java/com/bumptech/glide/load/engine/EngineResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,38 @@
* @param <Z> The type of data returned by the wrapped {@link Resource}.
*/
class EngineResource<Z> implements Resource<Z> {
private final boolean isCacheable;
private final boolean isMemoryCacheable;
private final boolean isRecyclable;
private final Resource<Z> resource;
private final ResourceListener listener;
private final Key key;

private ResourceListener listener;
private Key key;
private int acquired;
private boolean isRecycled;

interface ResourceListener {
void onResourceReleased(Key key, EngineResource<?> resource);
}

EngineResource(Resource<Z> toWrap, boolean isCacheable, boolean isRecyclable) {
EngineResource(
Resource<Z> 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<Z> getResource() {
return resource;
}

boolean isCacheable() {
return isCacheable;
boolean isMemoryCacheable() {
return isMemoryCacheable;
}

@NonNull
Expand Down Expand Up @@ -119,7 +121,7 @@ void release() {
@Override
public synchronized String toString() {
return "EngineResource{"
+ "isCacheable=" + isCacheable
+ "isMemoryCacheable=" + isMemoryCacheable
+ ", listener=" + listener
+ ", key=" + key
+ ", acquired=" + acquired
Expand Down
Loading

0 comments on commit 53438b9

Please sign in to comment.