From 100ac4a29eb525c0004d638edcd6ed8020864d75 Mon Sep 17 00:00:00 2001
From: Sam Judd <judds@google.com>
Date: Mon, 2 Dec 2019 11:29:03 -0800
Subject: [PATCH] Fix deadlock releasing resources that recursively clear when
 evicted from cache.

PiperOrigin-RevId: 283380663
---
 .../com/bumptech/glide/load/engine/Engine.java |  6 ++++--
 .../glide/load/engine/ResourceRecycler.java    |  4 ++--
 .../bumptech/glide/load/engine/EngineTest.java |  4 ++--
 .../load/engine/ResourceRecyclerTest.java      | 18 ++++++++++++++----
 4 files changed, 22 insertions(+), 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
index 21bb80f34b..25795af6e5 100644
--- 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
index ac8ade21fb..ed26e67ca4 100644
--- 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
index 7c9313b3a6..ae907187fa 100644
--- 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
index 322e9349c7..da172dc103 100644
--- 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();