Fix issues with recursive JIT loading that would lead to a deadlock and/or crash inside Guava's LoadingCache. The problem had to do with the fact that Guice eagerly stores uninitialized JIT bindings in an attempt to allow circular dependencies, and Guice also attempts to cleanup failed JIT bindings (to remove these uninitialized JIT bindings). The JIT binding cache was a guard against a recursive call back into the constructor's cache, but we were removing that guard.

We now only remove the guard if we aren't in the process of loading that JIT binding. The failed JIT binding is naturally cleaned up later on, as the existing & new tests attest to.

Fixes many issues:
 - Fixes #1633
 - Fixes #785
 - Fixes #1389
 - Fixes #1394

Many thanks to @swankjesse for the test-case added in #1389 that was helpful in diagnosing the problem, and to @PaulFridrick for the diagnoses in #1633.

PiperOrigin-RevId: 525219839

Author: sameb

core/src/com/google/inject/internal/ConstructorInjectorStore.java

@@ -16,7 +16,6 @@
 
 package com.google.inject.internal;
 
-
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.inject.spi.InjectionPoint;
@@ -43,6 +42,11 @@ protected ConstructorInjector<?> create(InjectionPoint constructorInjector, Erro
     this.injector = injector;
   }
 
+  /** Returns true if the store is in the process of loading this injection point. */
+  boolean isLoading(InjectionPoint ip) {
+    return cache.isLoading(ip);
+  }
+
   /** Returns a new complete constructor injector with injection listeners registered. */
   public ConstructorInjector<?> get(InjectionPoint constructorInjector, Errors errors)
       throws ErrorsException {

core/src/com/google/inject/internal/FailableCache.java

@@ -22,6 +22,8 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Lazily creates (and caches) values for keys. If creating the value fails (with errors), an
@@ -31,18 +33,23 @@
  */
 public abstract class FailableCache<K, V> {
 
+  private final Set<K> loadingSet = ConcurrentHashMap.newKeySet();
+
   private final LoadingCache<K, Object> delegate =
       CacheBuilder.newBuilder()
           .build(
               new CacheLoader<K, Object>() {
                 @Override
                 public Object load(K key) {
+                  loadingSet.add(key);
                   Errors errors = new Errors();
                   V result = null;
                   try {
                     result = FailableCache.this.create(key, errors);
                   } catch (ErrorsException e) {
                     errors.merge(e.getErrors());
+                  } finally {
+                    loadingSet.remove(key);
                   }
                   return errors.hasErrors() ? errors : result;
                 }
@@ -66,6 +73,10 @@ boolean remove(K key) {
     return delegate.asMap().remove(key) != null;
   }
 
+  boolean isLoading(K key) {
+    return loadingSet.contains(key);
+  }
+
   Map<K, V> asMap() {
     return Maps.transformValues(
         Maps.filterValues(

core/src/com/google/inject/internal/InjectorImpl.java

@@ -264,7 +264,7 @@ private <T> BindingImpl<T> getJustInTimeBinding(Key<T> key, Errors errors, JitLi
       // first try to find a JIT binding that we've already created
       for (InjectorImpl injector = this; injector != null; injector = injector.parent) {
         @SuppressWarnings("unchecked") // we only store bindings that match their key
-        BindingImpl<T> binding = (BindingImpl<T>) injector.jitBindingData.getJitBindings().get(key);
+        BindingImpl<T> binding = (BindingImpl<T>) injector.jitBindingData.getJitBinding(key);
 
         if (binding != null) {
           // If we found a JIT binding and we don't allow them,
@@ -656,12 +656,26 @@ private boolean cleanup(BindingImpl<?> binding, Set<Key<?>> encountered) {
   /** Cleans up any state that may have been cached when constructing the JIT binding. */
   private void removeFailedJitBinding(Binding<?> binding, InjectionPoint ip) {
     jitBindingData.addFailedJitBinding(binding.getKey());
-    jitBindingData.removeJitBinding(binding.getKey());
-    membersInjectorStore.remove(binding.getKey().getTypeLiteral());
-    provisionListenerStore.remove(binding);
-    if (ip != null) {
+    // Be careful cleaning up constructors & jitBindings -- we can't remove
+    // from `jitBindings` if we're still in the process of loading this constructor,
+    // otherwise we can re-enter the constructor's cache and attempt to load it
+    // while already loading it.  See issues:
+    // - https://github.com/google/guice/pull/1633
+    // - https://github.com/google/guice/issues/785
+    // - https://github.com/google/guice/pull/1389
+    // - https://github.com/google/guice/pull/1394
+    // (Note: there may be a better way to do this that avoids the need for the `isLoading`
+    //  conditional, but due to the recursive nature of JIT loading and the way we allow partially
+    //  initialized JIT bindings [to support circular dependencies], there's no other great way
+    //  that I could figure out.)
+    if (ip == null || !constructors.isLoading(ip)) {
+      jitBindingData.removeJitBinding(binding.getKey());
+    }
+    if (ip != null && !constructors.isLoading(ip)) {
       constructors.remove(ip);
     }
+    membersInjectorStore.remove(binding.getKey().getTypeLiteral());
+    provisionListenerStore.remove(binding);
   }
 
   /** Safely gets the dependencies of possibly not initialized bindings. */

core/test/com/google/inject/ImplicitBindingTest.java

@@ -16,7 +16,9 @@
 
 package com.google.inject;
 
+import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
 
 import com.google.common.collect.Iterables;
 import com.google.inject.internal.Annotations;
@@ -439,4 +441,94 @@ public void testImplicitJdkBindings_publicCxtor() {
     // String has a public nullary constructor, so Guice will call it.
     assertEquals("", injector.getInstance(String.class));
   }
+
+  public void testRecursiveLoadWithOptionals() {
+    Injector injector =
+        Guice.createInjector(
+            new AbstractModule() {
+              @Override
+              protected void configure() {
+                bind(A1.class);
+              }
+            });
+    assertThat(injector.getExistingBinding(Key.get(D1.class))).isNull();
+    assertThat(injector.getExistingBinding(Key.get(Unresolved.class))).isNull();
+  }
+
+  static class A1 {
+    @Inject B1 b;
+  }
+
+  static class B1 {
+    @Inject C1 c;
+  }
+
+  static class C1 {
+    @Inject(optional = true)
+    D1 d;
+
+    @Inject E1 e;
+  }
+
+  static class D1 {
+    @Inject B1 b;
+    @Inject Unresolved unresolved;
+  }
+
+  static class E1 {
+    @Inject B1 b;
+  }
+
+  public void testRecursiveLoadWithoutOptionals_atInjectorCreation() {
+    CreationException ce =
+        assertThrows(
+            CreationException.class,
+            () ->
+                Guice.createInjector(
+                    new AbstractModule() {
+                      @Provides
+                      public V provideV(Z z) {
+                        return null;
+                      }
+                    }));
+    assertThat(ce.getErrorMessages()).hasSize(1);
+    assertThat(getOnlyElement(ce.getErrorMessages()).getMessage())
+        .contains("No implementation for " + Unresolved.class.getName() + " was bound.");
+  }
+
+  public void testRecursiveLoadWithoutOptionals_afterCreation() {
+    Injector injector = Guice.createInjector();
+    ConfigurationException ce =
+        assertThrows(ConfigurationException.class, () -> injector.getBinding(Z.class));
+    assertThat(ce.getErrorMessages()).hasSize(1);
+    assertThat(getOnlyElement(ce.getErrorMessages()).getMessage())
+        .contains("No implementation for " + Unresolved.class.getName() + " was bound.");
+    assertThat(injector.getExistingBinding(Key.get(Z.class))).isNull();
+    assertThat(injector.getExistingBinding(Key.get(Y.class))).isNull();
+    assertThat(injector.getExistingBinding(Key.get(X.class))).isNull();
+    assertThat(injector.getExistingBinding(Key.get(W.class))).isNull();
+    assertThat(injector.getExistingBinding(Key.get(Unresolved.class))).isNull();
+  }
+
+  static class V {}
+
+  static class X {
+    @Inject Z z;
+  }
+
+  static class Z {
+    @Inject W w;
+    @Inject X x;
+  }
+
+  static class W {
+    @Inject Y y;
+    @Inject Z z;
+  }
+
+  static class Y {
+    @Inject Unresolved unresolved;
+  }
+
+  interface Unresolved {}
 }