From a38161c41310c2213f520e4b3b3b61218f2cea80 Mon Sep 17 00:00:00 2001 From: Sam Berlin Date: Tue, 18 Apr 2023 06:58:45 -0700 Subject: [PATCH] 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: 525135213 --- .../internal/ConstructorInjectorStore.java | 6 +- .../google/inject/internal/FailableCache.java | 11 +++ .../google/inject/internal/InjectorImpl.java | 24 ++++- .../google/inject/ImplicitBindingTest.java | 92 +++++++++++++++++++ 4 files changed, 127 insertions(+), 6 deletions(-) diff --git a/core/src/com/google/inject/internal/ConstructorInjectorStore.java b/core/src/com/google/inject/internal/ConstructorInjectorStore.java index 11a0e1d0a5..8e650d9519 100644 --- a/core/src/com/google/inject/internal/ConstructorInjectorStore.java +++ b/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 { diff --git a/core/src/com/google/inject/internal/FailableCache.java b/core/src/com/google/inject/internal/FailableCache.java index def5d980dd..ea573145ea 100644 --- a/core/src/com/google/inject/internal/FailableCache.java +++ b/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 { + private final Set loadingSet = ConcurrentHashMap.newKeySet(); + private final LoadingCache delegate = CacheBuilder.newBuilder() .build( new CacheLoader() { @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 asMap() { return Maps.transformValues( Maps.filterValues( diff --git a/core/src/com/google/inject/internal/InjectorImpl.java b/core/src/com/google/inject/internal/InjectorImpl.java index 9d0d271790..6bac30eafb 100644 --- a/core/src/com/google/inject/internal/InjectorImpl.java +++ b/core/src/com/google/inject/internal/InjectorImpl.java @@ -264,7 +264,7 @@ private BindingImpl getJustInTimeBinding(Key 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 binding = (BindingImpl) injector.jitBindingData.getJitBindings().get(key); + BindingImpl binding = (BindingImpl) 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> 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. */ diff --git a/core/test/com/google/inject/ImplicitBindingTest.java b/core/test/com/google/inject/ImplicitBindingTest.java index 78dd868b22..302ea9b0b2 100644 --- a/core/test/com/google/inject/ImplicitBindingTest.java +++ b/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 {} }