From 00e8c233a1ddf65092d840e96e94a11ff9d8b26f Mon Sep 17 00:00:00 2001 From: Sam Judd <judds@google.com> Date: Sat, 24 Dec 2022 00:05:12 -0800 Subject: [PATCH] Wrap recursive initialization booleans in try/finally Previously any exception in the initialization process could be hidden by one of these recursive checks. The first time the exception occured, it would be thrown normally, but subsequent times the recursive check would be thrown instead. This complicates debugging. Now we always unset the isInitializing boolean using try/finally. While this doesn't fix anything directly, it does make debugging easier when some kind of initialization error occurs. Helps with #4977 --- .../main/java/com/bumptech/glide/Glide.java | 13 ++-- .../com/bumptech/glide/RegistryFactory.java | 10 +-- .../bumptech/glide/InitializeGlideTest.java | 70 +++++++++++++++++++ .../bumptech/glide/RegistryFactoryTest.java | 63 +++++++++++++++++ 4 files changed, 147 insertions(+), 9 deletions(-) create mode 100644 library/test/src/test/java/com/bumptech/glide/InitializeGlideTest.java create mode 100644 library/test/src/test/java/com/bumptech/glide/RegistryFactoryTest.java diff --git a/library/src/main/java/com/bumptech/glide/Glide.java b/library/src/main/java/com/bumptech/glide/Glide.java index 4fc4b71ef4..bc7751556c 100644 --- a/library/src/main/java/com/bumptech/glide/Glide.java +++ b/library/src/main/java/com/bumptech/glide/Glide.java @@ -137,18 +137,21 @@ public static Glide get(@NonNull Context context) { } @GuardedBy("Glide.class") - private static void checkAndInitializeGlide( + @VisibleForTesting + static void checkAndInitializeGlide( @NonNull Context context, @Nullable GeneratedAppGlideModule generatedAppGlideModule) { // In the thread running initGlide(), one or more classes may call Glide.get(context). // Without this check, those calls could trigger infinite recursion. if (isInitializing) { throw new IllegalStateException( - "You cannot call Glide.get() in registerComponents()," - + " use the provided Glide instance instead"); + "Glide has been called recursively, this is probably an internal library error!"); } isInitializing = true; - initializeGlide(context, generatedAppGlideModule); - isInitializing = false; + try { + initializeGlide(context, generatedAppGlideModule); + } finally { + isInitializing = false; + } } /** diff --git a/library/src/main/java/com/bumptech/glide/RegistryFactory.java b/library/src/main/java/com/bumptech/glide/RegistryFactory.java index a8b7462db5..3ec3e961ed 100644 --- a/library/src/main/java/com/bumptech/glide/RegistryFactory.java +++ b/library/src/main/java/com/bumptech/glide/RegistryFactory.java @@ -90,22 +90,24 @@ static GlideSupplier<Registry> lazilyCreateAndInitializeRegistry( final List<GlideModule> manifestModules, @Nullable final AppGlideModule annotationGeneratedModule) { return new GlideSupplier<Registry>() { - private boolean isInitializingOrInitialized; + // Rely on callers using memoization if they want to avoid duplicate work, but + // rely on ourselves to verify that no recursive initialization occurs. + private boolean isInitializing; @Override public Registry get() { - if (isInitializingOrInitialized) { + if (isInitializing) { throw new IllegalStateException( "Recursive Registry initialization! In your" + " AppGlideModule and LibraryGlideModules, Make sure you're using the provided " + "Registry rather calling glide.getRegistry()!"); } - isInitializingOrInitialized = true; - Trace.beginSection("Glide registry"); + isInitializing = true; try { return createAndInitRegistry(glide, manifestModules, annotationGeneratedModule); } finally { + isInitializing = false; Trace.endSection(); } } diff --git a/library/test/src/test/java/com/bumptech/glide/InitializeGlideTest.java b/library/test/src/test/java/com/bumptech/glide/InitializeGlideTest.java new file mode 100644 index 0000000000..e4e4b5907f --- /dev/null +++ b/library/test/src/test/java/com/bumptech/glide/InitializeGlideTest.java @@ -0,0 +1,70 @@ +package com.bumptech.glide; + +import static org.junit.Assert.assertThrows; + +import android.content.Context; +import androidx.annotation.NonNull; +import androidx.test.core.app.ApplicationProvider; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.bumptech.glide.tests.TearDownGlide; +import java.util.Set; +import org.junit.Rule; +import org.junit.Test; +import org.junit.function.ThrowingRunnable; +import org.junit.runner.RunWith; + +// This test is about edge cases that might otherwise make debugging more challenging. +@RunWith(AndroidJUnit4.class) +public class InitializeGlideTest { + @Rule public final TearDownGlide tearDownGlide = new TearDownGlide(); + private final Context context = ApplicationProvider.getApplicationContext(); + private static final class TestException extends RuntimeException { + private static final long serialVersionUID = 2515021766931124927L; + } + + @Test + public void initialize_whenInternalMethodThrows_throwsException() { + assertThrows( + TestException.class, + new ThrowingRunnable() { + @Override + public void run() { + synchronized (Glide.class) { + Glide.checkAndInitializeGlide( + context, + new GeneratedAppGlideModule() { + @NonNull + @Override + Set<Class<?>> getExcludedModuleClasses() { + throw new TestException(); + } + }); + } + } + }); + } + + @Test + public void initialize_whenInternalMethodThrows_andCalledTwice_throwsException() { + GeneratedAppGlideModule throwingGeneratedAppGlideModule = + new GeneratedAppGlideModule() { + @NonNull + @Override + Set<Class<?>> getExcludedModuleClasses() { + throw new TestException(); + } + }; + ThrowingRunnable initializeGlide = new ThrowingRunnable() { + @Override + public void run() throws Throwable { + synchronized (Glide.class) { + Glide.checkAndInitializeGlide(context, throwingGeneratedAppGlideModule); + } + } + }; + + assertThrows(TestException.class, initializeGlide); + // Make sure the second exception isn't hidden by some Glide initialization related exception. + assertThrows(TestException.class, initializeGlide); + } +} diff --git a/library/test/src/test/java/com/bumptech/glide/RegistryFactoryTest.java b/library/test/src/test/java/com/bumptech/glide/RegistryFactoryTest.java new file mode 100644 index 0000000000..6591418e34 --- /dev/null +++ b/library/test/src/test/java/com/bumptech/glide/RegistryFactoryTest.java @@ -0,0 +1,63 @@ +package com.bumptech.glide; + +import static org.junit.Assert.assertThrows; + +import android.content.Context; +import androidx.annotation.NonNull; +import androidx.test.core.app.ApplicationProvider; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.bumptech.glide.module.AppGlideModule; +import com.bumptech.glide.tests.TearDownGlide; +import com.bumptech.glide.util.GlideSuppliers.GlideSupplier; +import com.google.common.collect.ImmutableList; +import org.junit.Rule; +import org.junit.Test; +import org.junit.function.ThrowingRunnable; +import org.junit.runner.RunWith; + +@RunWith(AndroidJUnit4.class) +public class RegistryFactoryTest { + @Rule public final TearDownGlide tearDownGlide = new TearDownGlide(); + private final Context context = ApplicationProvider.getApplicationContext(); + + private static final class TestException extends RuntimeException { + private static final long serialVersionUID = 2334956185897161236L; + } + + @Test + public void create_whenCalledTwiceWithThrowingModule_throwsOriginalException() { + AppGlideModule throwingAppGlideModule = + new AppGlideModule() { + @Override + public void registerComponents(@NonNull Context context, @NonNull Glide glide, + @NonNull Registry registry) { + throw new TestException(); + } + }; + + Glide glide = Glide.get(context); + GlideSupplier<Registry> registrySupplier = + RegistryFactory.lazilyCreateAndInitializeRegistry( + glide, + /* manifestModules= */ ImmutableList.of(), + throwingAppGlideModule); + + assertThrows( + TestException.class, + new ThrowingRunnable() { + @Override + public void run() { + registrySupplier.get(); + } + }); + + assertThrows( + TestException.class, + new ThrowingRunnable() { + @Override + public void run() { + registrySupplier.get(); + } + }); + } +}