Avoid holding locks when releasing resources.

We need to lock while updating the various states of the objects that
care about resources. We don't need a lock when all we're doing is
releasing the resource itself. Previously we fixed a deadlock by adding
locks so that locks were always acquired in order. It appears though
that we can go the other direction for this particular case and avoid
holding any locks, which should also solve the deadlock.

If it turns out this doesn't work we can revert this and add another
layer of locking when releasing from the memory cache. Ideally though
this should provide better performance with too much more complication.

Fixes #3795

PiperOrigin-RevId: 260748170

Author: sjudd

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

@@ -106,25 +106,18 @@ synchronized EngineResource<?> get(Key key) {
   @SuppressWarnings({"WeakerAccess", "SynchronizeOnNonFinalField"})
   @Synthetic
   void cleanupActiveReference(@NonNull ResourceWeakReference ref) {
-    // Fixes a deadlock where we normally acquire the Engine lock and then the ActiveResources lock
-    // but reverse that order in this one particular test. This is definitely a bit of a hack...
-    synchronized (listener) {
-      synchronized (this) {
-        activeEngineResources.remove(ref.key);
+    synchronized (this) {
+      activeEngineResources.remove(ref.key);
 
-        if (!ref.isCacheable || ref.resource == null) {
-          return;
-        }
-        EngineResource<?> newResource =
-            new EngineResource<>(
-                ref.resource,
-                /*isMemoryCacheable=*/ true,
-                /*isRecyclable=*/ false,
-                ref.key,
-                listener);
-        listener.onResourceReleased(ref.key, newResource);
+      if (!ref.isCacheable || ref.resource == null) {
+        return;
       }
     }
+
+    EngineResource<?> newResource =
+        new EngineResource<>(
+            ref.resource, /*isMemoryCacheable=*/ true, /*isRecyclable=*/ false, ref.key, listener);
+    listener.onResourceReleased(ref.key, newResource);
   }
 
   @SuppressWarnings("WeakerAccess")

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

@@ -388,7 +388,7 @@ public void onResourceRemoved(@NonNull final Resource<?> resource) {
   }
 
   @Override
-  public synchronized void onResourceReleased(Key cacheKey, EngineResource<?> resource) {
+  public void onResourceReleased(Key cacheKey, EngineResource<?> resource) {
     activeResources.deactivate(cacheKey);
     if (resource.isMemoryCacheable()) {
       cache.put(cacheKey, resource);

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

@@ -274,17 +274,22 @@ synchronized void incrementPendingCallbacks(int count) {
 
   @SuppressWarnings("WeakerAccess")
   @Synthetic
-  synchronized void decrementPendingCallbacks() {
-    stateVerifier.throwIfRecycled();
-    Preconditions.checkArgument(isDone(), "Not yet complete!");
-    int decremented = pendingCallbacks.decrementAndGet();
-    Preconditions.checkArgument(decremented >= 0, "Can't decrement below 0");
-    if (decremented == 0) {
-      if (engineResource != null) {
-        engineResource.release();
+  void decrementPendingCallbacks() {
+    EngineResource<?> toRelease = null;
+    synchronized (this) {
+      stateVerifier.throwIfRecycled();
+      Preconditions.checkArgument(isDone(), "Not yet complete!");
+      int decremented = pendingCallbacks.decrementAndGet();
+      Preconditions.checkArgument(decremented >= 0, "Can't decrement below 0");
+      if (decremented == 0) {
+        toRelease = engineResource;
+
+        release();
       }
+    }
 
-      release();
+    if (toRelease != null) {
+      toRelease.release();
     }
   }
 

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

@@ -103,18 +103,17 @@ synchronized void acquire() {
   // 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);
-        }
+    boolean release = false;
+    synchronized (this) {
+      if (acquired <= 0) {
+        throw new IllegalStateException("Cannot release a recycled or not yet acquired resource");
       }
+      if (--acquired == 0) {
+        release = true;
+      }
+    }
+    if (release) {
+      listener.onResourceReleased(key, this);
     }
   }
 

library/src/main/java/com/bumptech/glide/request/SingleRequest.java

@@ -307,22 +307,30 @@ private void assertNotCallingCallbacks() {
    * @see #cancel()
    */
   @Override
-  public synchronized void clear() {
-    assertNotCallingCallbacks();
-    stateVerifier.throwIfRecycled();
-    if (status == Status.CLEARED) {
-      return;
-    }
-    cancel();
-    // Resource must be released before canNotifyStatusChanged is called.
-    if (resource != null) {
-      releaseResource(resource);
-    }
-    if (canNotifyCleared()) {
-      target.onLoadCleared(getPlaceholderDrawable());
+  public void clear() {
+    Resource<R> toRelease = null;
+    synchronized (this) {
+      assertNotCallingCallbacks();
+      stateVerifier.throwIfRecycled();
+      if (status == Status.CLEARED) {
+        return;
+      }
+      cancel();
+      // Resource must be released before canNotifyStatusChanged is called.
+      if (resource != null) {
+        toRelease = resource;
+        resource = null;
+      }
+      if (canNotifyCleared()) {
+        target.onLoadCleared(getPlaceholderDrawable());
+      }
+
+      status = Status.CLEARED;
     }
 
-    status = Status.CLEARED;
+    if (toRelease != null) {
+      engine.release(toRelease);
+    }
   }
 
   @Override
@@ -332,11 +340,6 @@ public synchronized void pause() {
     }
   }
 
-  private void releaseResource(Resource<?> resource) {
-    engine.release(resource);
-    this.resource = null;
-  }
-
   @Override
   public synchronized boolean isRunning() {
     return status == Status.RUNNING || status == Status.WAITING_FOR_SIZE;
@@ -506,53 +509,64 @@ private void notifyLoadFailed() {
   @SuppressWarnings("unchecked")
   @Override
   public synchronized void onResourceReady(Resource<?> resource, DataSource dataSource) {
-    stateVerifier.throwIfRecycled();
-    loadStatus = null;
-    if (resource == null) {
-      GlideException exception =
-          new GlideException(
-              "Expected to receive a Resource<R> with an "
-                  + "object of "
-                  + transcodeClass
-                  + " inside, but instead got null.");
-      onLoadFailed(exception);
-      return;
-    }
+    Resource<?> toRelease = null;
+    try {
+      synchronized (this) {
+        stateVerifier.throwIfRecycled();
+        loadStatus = null;
+        if (resource == null) {
+          GlideException exception =
+              new GlideException(
+                  "Expected to receive a Resource<R> with an "
+                      + "object of "
+                      + transcodeClass
+                      + " inside, but instead got null.");
+          onLoadFailed(exception);
+          return;
+        }
 
-    Object received = resource.get();
-    if (received == null || !transcodeClass.isAssignableFrom(received.getClass())) {
-      releaseResource(resource);
-      GlideException exception =
-          new GlideException(
-              "Expected to receive an object of "
-                  + transcodeClass
-                  + " but instead"
-                  + " got "
-                  + (received != null ? received.getClass() : "")
-                  + "{"
-                  + received
-                  + "} inside"
-                  + " "
-                  + "Resource{"
-                  + resource
-                  + "}."
-                  + (received != null
-                      ? ""
-                      : " "
-                          + "To indicate failure return a null Resource "
-                          + "object, rather than a Resource object containing null data."));
-      onLoadFailed(exception);
-      return;
-    }
+        Object received = resource.get();
+        if (received == null || !transcodeClass.isAssignableFrom(received.getClass())) {
+          toRelease = resource;
+          this.resource = null;
+          GlideException exception =
+              new GlideException(
+                  "Expected to receive an object of "
+                      + transcodeClass
+                      + " but instead"
+                      + " got "
+                      + (received != null ? received.getClass() : "")
+                      + "{"
+                      + received
+                      + "} inside"
+                      + " "
+                      + "Resource{"
+                      + resource
+                      + "}."
+                      + (received != null
+                          ? ""
+                          : " "
+                              + "To indicate failure return a null Resource "
+                              + "object, rather than a Resource object containing null data."));
+          onLoadFailed(exception);
+          return;
+        }
 
-    if (!canSetResource()) {
-      releaseResource(resource);
-      // We can't put the status to complete before asking canSetResource().
-      status = Status.COMPLETE;
-      return;
-    }
+        if (!canSetResource()) {
+          toRelease = resource;
+          this.resource = null;
+          // We can't put the status to complete before asking canSetResource().
+          status = Status.COMPLETE;
+          return;
+        }
 
-    onResourceReady((Resource<R>) resource, (R) received, dataSource);
+        onResourceReady((Resource<R>) resource, (R) received, dataSource);
+      }
+    } finally {
+      if (toRelease != null) {
+        engine.release(toRelease);
+      }
+    }
   }
 
   /**