From ca9836d2ff53df3aebf3c602ef9f635c985135ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B3bert=20Papp=20=28TWiStErRob=29?= Date: Thu, 12 Oct 2023 16:32:51 +0100 Subject: [PATCH] Proof of concept for investigation of "Cannot recycle a resource while it is still acquired" --- .../glide/load/engine/ResourceRecycler.java | 32 +++++++++-- .../load/engine/ResourceRecyclerTest.java | 53 +++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) 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 ed26e67ca4..c9105c9f00 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 @@ -16,7 +16,7 @@ synchronized void recycle(Resource resource, boolean 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. - handler.obtainMessage(ResourceRecyclerCallback.RECYCLE_RESOURCE, resource).sendToTarget(); + handler.obtainMessage(ResourceRecyclerCallback.RECYCLE_RESOURCE, new RecycleTask(resource, new Throwable("Async stack trace"))).sendToTarget(); } else { isRecycling = true; resource.recycle(); @@ -33,11 +33,37 @@ private static final class ResourceRecyclerCallback implements Handler.Callback @Override public boolean handleMessage(Message message) { if (message.what == RECYCLE_RESOURCE) { - Resource resource = (Resource) message.obj; - resource.recycle(); + RecycleTask task = (RecycleTask) message.obj; + try { + Resource resource = task.resource; + resource.recycle(); + } catch (RuntimeException | Error ex) { + task.rethrow(ex); + } return true; } return false; } } + + private static final class RecycleTask { + final Resource resource; + final Throwable stacktrace; + + RecycleTask(Resource resource, Throwable stacktrace) { + this.resource = resource; + this.stacktrace = stacktrace; + } + + void rethrow(Throwable original) { + Throwable rootCause = original; + while (rootCause.getCause() != null) { + rootCause = rootCause.getCause(); + } + rootCause.initCause(stacktrace); + if (original instanceof Error) throw (Error) original; + if (original instanceof RuntimeException) throw (RuntimeException) original; + throw new IllegalStateException("Unknown exception: " + original, original); + } + } } 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 94767b4d8e..0c92144aa8 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 @@ -2,11 +2,19 @@ import static com.bumptech.glide.RobolectricConstants.ROBOLECTRIC_SDK; import static com.bumptech.glide.tests.Util.mockResource; +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import android.os.Looper; +import com.google.common.truth.Correspondence; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -71,4 +79,49 @@ public Void answer(InvocationOnMock invocationOnMock) throws Throwable { verify(child).recycle(); } + + @Test + public void recycle_withChild_asyncTraceable() { + final Resource child = mockResource(); + Throwable someCause = new Throwable("Some simulated cause."); + IllegalStateException testEx = new IllegalStateException("Simulated error for test.", someCause); + doThrow(testEx).when(child).recycle(); + Resource parent = mockResource(); + class ChildRecycler implements Answer { + @Override + public Void answer(InvocationOnMock invocationOnMock) throws Throwable { + recycler.recycle(child, /* forceNextFrame= */ false); + return null; + } + } + doAnswer(new ChildRecycler()).when(parent).recycle(); + + Shadows.shadowOf(Looper.getMainLooper()).pause(); + recycler.recycle(parent, /* forceNextFrame= */ false); + IllegalStateException ex = assertThrows( + IllegalStateException.class, + () -> Shadows.shadowOf(Looper.getMainLooper()).runOneTask() + ); + assertSame("Original exception is thrown", testEx, ex); + assertSame("Cause is kept", someCause, ex.getCause()); + assertThat(fullStackOf(ex)) + .comparingElementsUsing(className()) + .contains(ChildRecycler.class.getName()); + } + + private static Correspondence className() { + return Correspondence.from( + (actual, expected) -> actual.getClassName().equals(expected), + "StackTraceElement.className" + ); + } + + private static List fullStackOf(Throwable t) { + List stack = new ArrayList<>(); + do { + stack.addAll(Arrays.asList(t.getStackTrace())); + t = t.getCause(); + } while (t != null); + return stack; + } }