Skip to content

Commit

Permalink
Wrap recursive initialization booleans in try/finally
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sjudd committed Dec 29, 2022
1 parent 2aed37c commit 00e8c23
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 9 deletions.
13 changes: 8 additions & 5 deletions library/src/main/java/com/bumptech/glide/Glide.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

/**
Expand Down
10 changes: 6 additions & 4 deletions library/src/main/java/com/bumptech/glide/RegistryFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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();
}
});
}
}

0 comments on commit 00e8c23

Please sign in to comment.