Skip to content

Commit

Permalink
Avoid holding the Engine lock while calling callbacks for in memory r…
Browse files Browse the repository at this point in the history
…esources.

Doing so can make it easy for callers to deadlock.

PiperOrigin-RevId: 255250190
  • Loading branch information
sjudd authored and glide-copybara-robot committed Jun 26, 2019
1 parent 8907122 commit b96b000
Showing 1 changed file with 84 additions and 24 deletions.
108 changes: 84 additions & 24 deletions library/src/main/java/com/bumptech/glide/load/engine/Engine.java
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public Engine(
* @param height The target height in pixels of the desired resource.
* @param cb The callback that will be called when the load completes.
*/
public synchronized <R> LoadStatus load(
public <R> LoadStatus load(
GlideContext glideContext,
Object model,
Key signature,
Expand Down Expand Up @@ -185,24 +185,65 @@ public synchronized <R> LoadStatus load(
transcodeClass,
options);

EngineResource<?> active = loadFromActiveResources(key, isMemoryCacheable);
if (active != null) {
cb.onResourceReady(active, DataSource.MEMORY_CACHE);
if (VERBOSE_IS_LOGGABLE) {
logWithTimeAndKey("Loaded resource from active resources", startTime, key);
}
return null;
}
EngineResource<?> memoryResource;
synchronized (this) {
memoryResource = loadFromMemory(key, isMemoryCacheable, startTime);

EngineResource<?> cached = loadFromCache(key, isMemoryCacheable);
if (cached != null) {
cb.onResourceReady(cached, DataSource.MEMORY_CACHE);
if (VERBOSE_IS_LOGGABLE) {
logWithTimeAndKey("Loaded resource from cache", startTime, key);
if (memoryResource == null) {
return waitForExistingOrStartNewJob(
glideContext,
model,
signature,
width,
height,
resourceClass,
transcodeClass,
priority,
diskCacheStrategy,
transformations,
isTransformationRequired,
isScaleOnlyOrNoTransform,
options,
isMemoryCacheable,
useUnlimitedSourceExecutorPool,
useAnimationPool,
onlyRetrieveFromCache,
cb,
callbackExecutor,
key,
startTime);
}
return null;
}

// Avoid calling back while holding the engine lock, doing so makes it easier for callers to
// deadlock.
cb.onResourceReady(memoryResource, DataSource.MEMORY_CACHE);
return null;
}

private <R> LoadStatus waitForExistingOrStartNewJob(
GlideContext glideContext,
Object model,
Key signature,
int width,
int height,
Class<?> resourceClass,
Class<R> transcodeClass,
Priority priority,
DiskCacheStrategy diskCacheStrategy,
Map<Class<?>, Transformation<?>> transformations,
boolean isTransformationRequired,
boolean isScaleOnlyOrNoTransform,
Options options,
boolean isMemoryCacheable,
boolean useUnlimitedSourceExecutorPool,
boolean useAnimationPool,
boolean onlyRetrieveFromCache,
ResourceCallback cb,
Executor callbackExecutor,
EngineKey key,
long startTime) {

EngineJob<?> current = jobs.get(key, onlyRetrieveFromCache);
if (current != null) {
current.addCallback(cb, callbackExecutor);
Expand Down Expand Up @@ -250,15 +291,38 @@ public synchronized <R> LoadStatus load(
return new LoadStatus(cb, engineJob);
}

@Nullable
private EngineResource<?> loadFromMemory(
EngineKey key, boolean isMemoryCacheable, long startTime) {
if (!isMemoryCacheable) {
return null;
}

EngineResource<?> active = loadFromActiveResources(key);
if (active != null) {
if (VERBOSE_IS_LOGGABLE) {
logWithTimeAndKey("Loaded resource from active resources", startTime, key);
}
return active;
}

EngineResource<?> cached = loadFromCache(key);
if (cached != null) {
if (VERBOSE_IS_LOGGABLE) {
logWithTimeAndKey("Loaded resource from cache", startTime, key);
}
return cached;
}

return null;
}

private static void logWithTimeAndKey(String log, long startTime, Key key) {
Log.v(TAG, log + " in " + LogTime.getElapsedMillis(startTime) + "ms, key: " + key);
}

@Nullable
private EngineResource<?> loadFromActiveResources(Key key, boolean isMemoryCacheable) {
if (!isMemoryCacheable) {
return null;
}
private EngineResource<?> loadFromActiveResources(Key key) {
EngineResource<?> active = activeResources.get(key);
if (active != null) {
active.acquire();
Expand All @@ -267,11 +331,7 @@ private EngineResource<?> loadFromActiveResources(Key key, boolean isMemoryCache
return active;
}

private EngineResource<?> loadFromCache(Key key, boolean isMemoryCacheable) {
if (!isMemoryCacheable) {
return null;
}

private EngineResource<?> loadFromCache(Key key) {
EngineResource<?> cached = getEngineResourceFromCache(key);
if (cached != null) {
cached.acquire();
Expand Down

0 comments on commit b96b000

Please sign in to comment.