Skip to content

Commit

Permalink
Fix a deadlock in EngineResource/Engine.
Browse files Browse the repository at this point in the history
Engine acquires it's lock, then the EngineResource lock when it loads
resources from cache or active resources. EngineResource acquires its
lock, then the EngineLock, when a resource is cleared. This conflicting
lock order leads to deadlock.

The fix is similar to the fix applied in ActiveResources. We always make
sure that the Engine (listener) lock is acquired before the
EngineResource lock.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=233861929
  • Loading branch information
sjudd committed Feb 14, 2019
1 parent a16a1ba commit 92d8cc4
Showing 1 changed file with 17 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
class EngineResource<Z> implements Resource<Z> {
private final boolean isCacheable;
private final boolean isRecyclable;
private final Resource<Z> resource;

private ResourceListener listener;
private Key key;
private int acquired;
private boolean isRecycled;
private final Resource<Z> resource;

interface ResourceListener {
void onResourceReleased(Key key, EngineResource<?> resource);
Expand Down Expand Up @@ -97,12 +98,21 @@ synchronized void acquire() {
* done with the resource. Generally external users should never call this method, the framework
* will take care of this for you.
*/
synchronized void release() {
if (acquired <= 0) {
throw new IllegalStateException("Cannot release a recycled or not yet acquired resource");
}
if (--acquired == 0) {
listener.onResourceReleased(key, this);
// listener is effectively final.
@SuppressWarnings("SynchronizeOnNonFinalField")
void release() {
// To avoid deadlock, always acquire the listener lock before our lock so that the locking
// scheme is consistent (Engine -> EngineResource). Violating this order leads to deadlock
// (b/123646037).
synchronized (listener) {
synchronized (this) {
if (acquired <= 0) {
throw new IllegalStateException("Cannot release a recycled or not yet acquired resource");
}
if (--acquired == 0) {
listener.onResourceReleased(key, this);
}
}
}
}

Expand Down

1 comment on commit 92d8cc4

@SZn007
Copy link

@SZn007 SZn007 commented on 92d8cc4 Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2 main
java.lang.NullPointerException
Null reference used for synchronization (monitor-enter)

1 com.bumptech.glide.load.engine.EngineResource.release(SourceFile:107)
2 com.bumptech.glide.load.engine.Engine.release(SourceFile:286)
3 com.bumptech.glide.request.SingleRequest.releaseResource(SourceFile:340)
4 com.bumptech.glide.request.SingleRequest.clear(SourceFile:330)
5 com.bumptech.glide.manager.RequestTracker.clearRemoveAndMaybeRecycle(SourceFile:79)
6 com.bumptech.glide.manager.RequestTracker.clearRemoveAndRecycle(SourceFile:66)
7 com.bumptech.glide.RequestManager.untrack(SourceFile:630)
8 com.bumptech.glide.RequestManager.untrackOrDelegate(SourceFile:598)
9 com.bumptech.glide.RequestManager.clear(SourceFile:594)
10 com.bumptech.glide.request.target.PreloadTarget.clear(SourceFile:57)
11 com.bumptech.glide.request.target.PreloadTarget$1.handleMessage(SourceFile:25)
12 android.os.Handler.dispatchMessage(Handler.java:102)
13 android.os.Looper.loop(Looper.java:164)
14 android.app.ActivityThread.main(ActivityThread.java:6906)
15 java.lang.reflect.Method.invoke(Native Method)
16 com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
17 com.android.internal.os.ZygoteInit.main(ZygoteInit.java:820)

Please sign in to comment.