Skip to content

Commit 100ac4a

Browse files
sjuddglide-copybara-robot
authored andcommittedDec 2, 2019
Fix deadlock releasing resources that recursively clear when evicted from cache.
PiperOrigin-RevId: 283380663
1 parent f4da653 commit 100ac4a

File tree

4 files changed

+22
-10
lines changed

4 files changed

+22
-10
lines changed
 

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

+4-2
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,9 @@ public synchronized void onEngineJobCancelled(EngineJob<?> engineJob, Key key) {
384384

385385
@Override
386386
public void onResourceRemoved(@NonNull final Resource<?> resource) {
387-
resourceRecycler.recycle(resource);
387+
// Avoid deadlock with RequestManagers when recycling triggers recursive clear() calls.
388+
// See b/145519760.
389+
resourceRecycler.recycle(resource, /*forceNextFrame=*/ true);
388390
}
389391

390392
@Override
@@ -393,7 +395,7 @@ public void onResourceReleased(Key cacheKey, EngineResource<?> resource) {
393395
if (resource.isMemoryCacheable()) {
394396
cache.put(cacheKey, resource);
395397
} else {
396-
resourceRecycler.recycle(resource);
398+
resourceRecycler.recycle(resource, /*forceNextFrame=*/ false);
397399
}
398400
}
399401

‎library/src/main/java/com/bumptech/glide/load/engine/ResourceRecycler.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ class ResourceRecycler {
1111
private final Handler handler =
1212
new Handler(Looper.getMainLooper(), new ResourceRecyclerCallback());
1313

14-
synchronized void recycle(Resource<?> resource) {
15-
if (isRecycling) {
14+
synchronized void recycle(Resource<?> resource, boolean forceNextFrame) {
15+
if (isRecycling || forceNextFrame) {
1616
// If a resource has sub-resources, releasing a sub resource can cause it's parent to be
1717
// synchronously evicted which leads to a recycle loop when the parent releases it's children.
1818
// Posting breaks this loop.

‎library/test/src/test/java/com/bumptech/glide/load/engine/EngineTest.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ public void testResourceIsNotAddedToCacheOnReleasedIfNotCacheable() {
351351
public void testResourceIsRecycledIfNotCacheableWhenReleased() {
352352
when(harness.resource.isMemoryCacheable()).thenReturn(false);
353353
harness.getEngine().onResourceReleased(harness.cacheKey, harness.resource);
354-
verify(harness.resourceRecycler).recycle(eq(harness.resource));
354+
verify(harness.resourceRecycler).recycle(eq(harness.resource), eq(false));
355355
}
356356

357357
@Test
@@ -372,7 +372,7 @@ public void testEngineAddedAsListenerToMemoryCache() {
372372
@Test
373373
public void testResourceIsRecycledWhenRemovedFromCache() {
374374
harness.getEngine().onResourceRemoved(harness.resource);
375-
verify(harness.resourceRecycler).recycle(eq(harness.resource));
375+
verify(harness.resourceRecycler).recycle(eq(harness.resource), eq(true));
376376
}
377377

378378
@Test

‎library/test/src/test/java/com/bumptech/glide/load/engine/ResourceRecyclerTest.java

+14-4
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,20 @@ public void setUp() {
2727
}
2828

2929
@Test
30-
public void testRecyclesResourceSynchronouslyIfNotAlreadyRecyclingResource() {
30+
public void recycle_withoutForceNextFrame_recyclesResourceSynchronously() {
3131
Resource<?> resource = mockResource();
3232
Shadows.shadowOf(Looper.getMainLooper()).pause();
33-
recycler.recycle(resource);
33+
recycler.recycle(resource, /*forceNextFrame=*/ false);
34+
verify(resource).recycle();
35+
}
36+
37+
@Test
38+
public void recycle_withForceNextFrame_postsRecycle() {
39+
Resource<?> resource = mockResource();
40+
Shadows.shadowOf(Looper.getMainLooper()).pause();
41+
recycler.recycle(resource, /*forceNextFrame=*/ true);
42+
verify(resource, never()).recycle();
43+
Shadows.shadowOf(Looper.getMainLooper()).runToEndOfTasks();
3444
verify(resource).recycle();
3545
}
3646

@@ -42,7 +52,7 @@ public void testDoesNotRecycleChildResourceSynchronously() {
4252
new Answer<Void>() {
4353
@Override
4454
public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
45-
recycler.recycle(child);
55+
recycler.recycle(child, /*forceNextFrame=*/ false);
4656
return null;
4757
}
4858
})
@@ -51,7 +61,7 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable {
5161

5262
Shadows.shadowOf(Looper.getMainLooper()).pause();
5363

54-
recycler.recycle(parent);
64+
recycler.recycle(parent, /*forceNextFrame=*/ false);
5565

5666
verify(parent).recycle();
5767
verify(child, never()).recycle();

0 commit comments

Comments
 (0)
Please sign in to comment.