Fix deadlock releasing resources that recursively clear when evicted …

…from cache. PiperOrigin-RevId: 283380663
bumptech · Dec 2, 2019 · 100ac4a · 100ac4a
1 parent f4da653
commit 100ac4a
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 10 deletions.
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
@@ -384,7 +384,9 @@ public synchronized void onEngineJobCancelled(EngineJob<?> engineJob, Key key) {
 
   @Override
   public void onResourceRemoved(@NonNull final Resource<?> resource) {
-    resourceRecycler.recycle(resource);
+    // Avoid deadlock with RequestManagers when recycling triggers recursive clear() calls.
+    // See b/145519760.
+    resourceRecycler.recycle(resource, /*forceNextFrame=*/ true);
   }
 
   @Override
@@ -393,7 +395,7 @@ public void onResourceReleased(Key cacheKey, EngineResource<?> resource) {
     if (resource.isMemoryCacheable()) {
       cache.put(cacheKey, resource);
     } else {
-      resourceRecycler.recycle(resource);
+      resourceRecycler.recycle(resource, /*forceNextFrame=*/ false);
     }
   }
 

diff --git a/library/src/main/java/com/bumptech/glide/load/engine/ResourceRecycler.java b/library/src/main/java/com/bumptech/glide/load/engine/ResourceRecycler.java
@@ -11,8 +11,8 @@ class ResourceRecycler {
   private final Handler handler =
       new Handler(Looper.getMainLooper(), new ResourceRecyclerCallback());
 
-  synchronized void recycle(Resource<?> resource) {
-    if (isRecycling) {
+  synchronized void recycle(Resource<?> resource, boolean forceNextFrame) {
+    if (isRecycling || forceNextFrame) {
       // If a resource has sub-resources, releasing a sub resource can cause it's parent to be
       // synchronously evicted which leads to a recycle loop when the parent releases it's children.
       // Posting breaks this loop.

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
@@ -351,7 +351,7 @@ public void testResourceIsNotAddedToCacheOnReleasedIfNotCacheable() {
   public void testResourceIsRecycledIfNotCacheableWhenReleased() {
     when(harness.resource.isMemoryCacheable()).thenReturn(false);
     harness.getEngine().onResourceReleased(harness.cacheKey, harness.resource);
-    verify(harness.resourceRecycler).recycle(eq(harness.resource));
+    verify(harness.resourceRecycler).recycle(eq(harness.resource), eq(false));
   }
 
   @Test
@@ -372,7 +372,7 @@ public void testEngineAddedAsListenerToMemoryCache() {
   @Test
   public void testResourceIsRecycledWhenRemovedFromCache() {
     harness.getEngine().onResourceRemoved(harness.resource);
-    verify(harness.resourceRecycler).recycle(eq(harness.resource));
+    verify(harness.resourceRecycler).recycle(eq(harness.resource), eq(true));
   }
 
   @Test

diff --git a/library/test/src/test/java/com/bumptech/glide/load/engine/ResourceRecyclerTest.java b/library/test/src/test/java/com/bumptech/glide/load/engine/ResourceRecyclerTest.java
@@ -27,10 +27,20 @@ public void setUp() {
   }
 
   @Test
-  public void testRecyclesResourceSynchronouslyIfNotAlreadyRecyclingResource() {
+  public void recycle_withoutForceNextFrame_recyclesResourceSynchronously() {
     Resource<?> resource = mockResource();
     Shadows.shadowOf(Looper.getMainLooper()).pause();
-    recycler.recycle(resource);
+    recycler.recycle(resource, /*forceNextFrame=*/ false);
+    verify(resource).recycle();
+  }
+
+  @Test
+  public void recycle_withForceNextFrame_postsRecycle() {
+    Resource<?> resource = mockResource();
+    Shadows.shadowOf(Looper.getMainLooper()).pause();
+    recycler.recycle(resource, /*forceNextFrame=*/ true);
+    verify(resource, never()).recycle();
+    Shadows.shadowOf(Looper.getMainLooper()).runToEndOfTasks();
     verify(resource).recycle();
   }
 
@@ -42,7 +52,7 @@ public void testDoesNotRecycleChildResourceSynchronously() {
             new Answer<Void>() {
               @Override
               public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
-                recycler.recycle(child);
+                recycler.recycle(child, /*forceNextFrame=*/ false);
                 return null;
               }
             })
@@ -51,7 +61,7 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
 
     Shadows.shadowOf(Looper.getMainLooper()).pause();
 
-    recycler.recycle(parent);
+    recycler.recycle(parent, /*forceNextFrame=*/ false);
 
     verify(parent).recycle();
     verify(child, never()).recycle();