fixes issue 319 -- cleanup invalid leftover bindings that were added optimistically to support circular references.
git-svn-id: https://google-guice.googlecode.com/svn/trunk@1167 d779f126-a31b-0410-b53b-1d3aecad763e
diff --git a/src/com/google/inject/internal/ConstructorBindingImpl.java b/src/com/google/inject/internal/ConstructorBindingImpl.java
index b363b50..85029df 100644
--- a/src/com/google/inject/internal/ConstructorBindingImpl.java
+++ b/src/com/google/inject/internal/ConstructorBindingImpl.java
@@ -121,7 +121,39 @@
factory.constructorInjector
= (ConstructorInjector<T>) injector.constructors.get(constructorInjectionPoint, errors);
}
+
+ /** True if this binding has been initialized and is ready for use. */
+ boolean isInitialized() {
+ return factory.constructorInjector != null;
+ }
+ /** Returns an injection point that can be used to clean up the constructor store. */
+ InjectionPoint getInternalConstructor() {
+ if(factory.constructorInjector != null) {
+ return factory.constructorInjector.getConstructionProxy().getInjectionPoint();
+ } else {
+ return constructorInjectionPoint;
+ }
+ }
+
+ /** Returns a set of dependencies that can be iterated over to clean up stray JIT bindings. */
+ Set<Dependency<?>> getInternalDependencies() {
+ ImmutableSet.Builder<InjectionPoint> builder = ImmutableSet.builder();
+ if(factory.constructorInjector == null) {
+ builder.add(constructorInjectionPoint);
+ // If the below throws, it's OK -- we just ignore those dependencies, because no one
+ // could have used them anyway.
+ try {
+ builder.addAll(InjectionPoint.forInstanceMethodsAndFields(constructorInjectionPoint.getDeclaringType()));
+ } catch(ConfigurationException ignored) {}
+ } else {
+ builder.add(getConstructor())
+ .addAll(getInjectableMembers());
+ }
+
+ return Dependency.forInjectionPoints(builder.build());
+ }
+
public <V> V acceptTargetVisitor(BindingTargetVisitor<? super T, V> visitor) {
checkState(factory.constructorInjector != null, "not initialized");
return visitor.visit(this);
diff --git a/src/com/google/inject/internal/ConstructorInjectorStore.java b/src/com/google/inject/internal/ConstructorInjectorStore.java
index e1122a5..7ea52fa 100644
--- a/src/com/google/inject/internal/ConstructorInjectorStore.java
+++ b/src/com/google/inject/internal/ConstructorInjectorStore.java
@@ -47,6 +47,19 @@
throws ErrorsException {
return cache.get(constructorInjector, errors);
}
+
+ /**
+ * Purges an injection point from the cache. Use this only if the cache is not actually valid and
+ * needs to be purged. (See issue 319 and
+ * ImplicitBindingTest#testCircularJitBindingsLeaveNoResidue and
+ * #testInstancesRequestingProvidersForThemselvesWithChildInjectors for examples of when this is
+ * necessary.)
+ *
+ * Returns true if the injector for that point was stored in the cache, false otherwise.
+ */
+ boolean remove(InjectionPoint ip) {
+ return cache.remove(ip);
+ }
private <T> ConstructorInjector<T> createConstructor(InjectionPoint injectionPoint, Errors errors)
throws ErrorsException {
diff --git a/src/com/google/inject/internal/FailableCache.java b/src/com/google/inject/internal/FailableCache.java
index 3522b9d..fea8cda 100644
--- a/src/com/google/inject/internal/FailableCache.java
+++ b/src/com/google/inject/internal/FailableCache.java
@@ -53,4 +53,8 @@
return result;
}
}
+
+ boolean remove(K key) {
+ return delegate.remove(key) != null;
+ }
}
diff --git a/src/com/google/inject/internal/InjectorImpl.java b/src/com/google/inject/internal/InjectorImpl.java
index d471551..520789a 100644
--- a/src/com/google/inject/internal/InjectorImpl.java
+++ b/src/com/google/inject/internal/InjectorImpl.java
@@ -44,6 +44,7 @@
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.util.Collections;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -434,21 +435,81 @@
// Put the partially constructed binding in the map a little early. This enables us to handle
// circular dependencies. Example: FooImpl -> BarImpl -> FooImpl.
// Note: We don't need to synchronize on state.lock() during injector creation.
- // TODO: for the above example, remove the binding for BarImpl if the binding for FooImpl fails
if (binding instanceof ConstructorBindingImpl<?>) {
Key<T> key = binding.getKey();
jitBindings.put(key, binding);
boolean successful = false;
+ ConstructorBindingImpl cb = (ConstructorBindingImpl)binding;
try {
- ((ConstructorBindingImpl) binding).initialize(this, errors);
+ cb.initialize(this, errors);
successful = true;
} finally {
if (!successful) {
- jitBindings.remove(key);
+ // We do not pass cb.getInternalConstructor as the second parameter
+ // so that cached exceptions while constructing it get stored.
+ // See TypeListenerTest#testTypeListenerThrows
+ removeFailedJitBinding(key, null);
+ cleanup(binding, new HashSet<Key>());
}
}
}
}
+
+ /**
+ * Iterates through the binding's dependencies to clean up any stray bindings that were leftover
+ * from a failed JIT binding. This is required because the bindings are eagerly &
+ * optimistically added to allow circular dependency support, so dependencies may pass where they
+ * should have failed.
+ */
+ private boolean cleanup(BindingImpl<?> binding, Set<Key> encountered) {
+ boolean bindingFailed = false;
+ Set<Dependency<?>> deps = getInternalDependencies(binding);
+ for(Dependency dep : deps) {
+ Key<?> depKey = dep.getKey();
+ InjectionPoint ip = dep.getInjectionPoint();
+ if(encountered.add(depKey)) { // only check if we haven't looked at this key yet
+ BindingImpl depBinding = jitBindings.get(depKey);
+ if(depBinding != null) { // if the binding still exists, validate
+ boolean failed = cleanup(depBinding, encountered); // if children fail, we fail
+ if(depBinding instanceof ConstructorBindingImpl) {
+ ConstructorBindingImpl ctorBinding = (ConstructorBindingImpl)depBinding;
+ ip = ctorBinding.getInternalConstructor();
+ if(!ctorBinding.isInitialized()) {
+ failed = true;
+ }
+ }
+ if(failed) {
+ removeFailedJitBinding(depKey, ip);
+ bindingFailed = true;
+ }
+ } else if(state.getExplicitBinding(depKey) == null) {
+ // ignore keys if they were explicitly bound, but if neither JIT
+ // nor explicit, it's also invalid & should let parent know.
+ bindingFailed = true;
+ }
+ }
+ }
+ return bindingFailed;
+ }
+
+ /** Cleans up any state that may have been cached when constructing the JIT binding. */
+ private void removeFailedJitBinding(Key<?> key, InjectionPoint ip) {
+ jitBindings.remove(key);
+ membersInjectorStore.remove(key.getTypeLiteral());
+ if(ip != null) {
+ constructors.remove(ip);
+ }
+ }
+
+ /** Safely gets the dependencies of possibly not initialized bindings. */
+ @SuppressWarnings("unchecked")
+ private Set<Dependency<?>> getInternalDependencies(BindingImpl<?> binding) {
+ if(binding instanceof ConstructorBindingImpl) {
+ return ((ConstructorBindingImpl)binding).getInternalDependencies();
+ } else {
+ return ((HasDependencies)binding).getDependencies();
+ }
+ }
/**
* Creates a binding for an injectable type with the given scope. Looks for a scope on the type if
diff --git a/src/com/google/inject/internal/MembersInjectorStore.java b/src/com/google/inject/internal/MembersInjectorStore.java
index 417c437..7b7f266 100644
--- a/src/com/google/inject/internal/MembersInjectorStore.java
+++ b/src/com/google/inject/internal/MembersInjectorStore.java
@@ -64,6 +64,19 @@
}
/**
+ * Purges a type literal from the cache. Use this only if the type is not actually valid for
+ * binding and needs to be purged. (See issue 319 and
+ * ImplicitBindingTest#testCircularJitBindingsLeaveNoResidue and
+ * #testInstancesRequestingProvidersForThemselvesWithChildInjectors for examples of when this is
+ * necessary.)
+ *
+ * Returns true if the type was stored in the cache, false otherwise.
+ */
+ boolean remove(TypeLiteral<?> type) {
+ return cache.remove(type);
+ }
+
+ /**
* Creates a new members injector and attaches both injection listeners and method aspects.
*/
private <T> MembersInjectorImpl<T> createWithListeners(TypeLiteral<T> type, Errors errors)
diff --git a/test/com/google/inject/AllTests.java b/test/com/google/inject/AllTests.java
index 6c2929c..4142863 100644
--- a/test/com/google/inject/AllTests.java
+++ b/test/com/google/inject/AllTests.java
@@ -54,8 +54,7 @@
private static final Set<String> SUPPRESSED_TEST_NAMES = ImmutableSet.of(
"testUnscopedProviderWorksOutsideOfRequestedScope(" + ScopesTest.class.getName() + ")",
- "testCannotConvertUnannotatedBindings(" + TypeConversionTest.class.getName() + ")",
- "testCircularJitBindingsLeaveNoResidue(" + ImplicitBindingTest.class.getName() + ")"
+ "testCannotConvertUnannotatedBindings(" + TypeConversionTest.class.getName() + ")"
);
public static Test suite() {
diff --git a/test/com/google/inject/ImplicitBindingTest.java b/test/com/google/inject/ImplicitBindingTest.java
index 7db69a7..40ff6bb 100644
--- a/test/com/google/inject/ImplicitBindingTest.java
+++ b/test/com/google/inject/ImplicitBindingTest.java
@@ -16,6 +16,9 @@
package com.google.inject;
+import java.util.List;
+
+import com.google.inject.internal.Iterables;
import com.google.inject.name.Named;
import com.google.inject.name.Names;
import junit.framework.TestCase;
@@ -107,31 +110,153 @@
* When we're building the binding for A, we temporarily insert that binding to support circular
* dependencies. And so we can successfully create a binding for B. But later, when the binding
* for A ultimately fails, we need to clean up the dependent binding for B.
+ *
+ * The test loops through linked bindings & bindings with constructor & member injections,
+ * to make sure that all are cleaned up and traversed. It also makes sure we don't touch
+ * explicit bindings.
*/
public void testCircularJitBindingsLeaveNoResidue() {
- Injector injector = Guice.createInjector();
+ Injector injector = Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(Valid.class);
+ bind(Valid2.class);
+ }
+ });
+
+ // Capture good bindings.
+ Binding v1 = injector.getBinding(Valid.class);
+ Binding v2 = injector.getBinding(Valid2.class);
+ Binding jv1 = injector.getBinding(JitValid.class);
+ Binding jv2 = injector.getBinding(JitValid2.class);
+ // Then validate that a whole series of invalid bindings are erased.
+ assertFailure(injector, Invalid.class);
+ assertFailure(injector, InvalidLinked.class);
+ assertFailure(injector, InvalidLinkedImpl.class);
+ assertFailure(injector, InvalidLinked2.class);
+ assertFailure(injector, InvalidLinked2Impl.class);
+ assertFailure(injector, Invalid2.class);
+
+ // Validate we didn't do anything to the valid explicit bindings.
+ assertSame(v1, injector.getBinding(Valid.class));
+ assertSame(v2, injector.getBinding(Valid2.class));
+
+ // Validate that we didn't erase the valid JIT bindings
+ assertSame(jv1, injector.getBinding(JitValid.class));
+ assertSame(jv2, injector.getBinding(JitValid2.class));
+ }
+
+ @SuppressWarnings("unchecked")
+ private void assertFailure(Injector injector, Class clazz) {
try {
- injector.getBinding(A.class);
- fail();
- } catch (ConfigurationException expected) {
- }
-
- try {
- injector.getBinding(B.class);
- fail();
- } catch (ConfigurationException expected) {
+ injector.getBinding(clazz);
+ fail("Shouldn't have been able to get binding of: " + clazz);
+ } catch(ConfigurationException expected) {
+ List<Object> sources = Iterables.getOnlyElement(expected.getErrorMessages()).getSources();
+ // Assert that the first item in the sources if the key for the class we're looking up,
+ // ensuring that each lookup is "new".
+ assertEquals(Key.get(clazz).toString(), sources.get(0).toString());
+ // Assert that the last item in each lookup contains the InvalidInterface class
+ Asserts.assertContains(sources.get(sources.size()-1).toString(),
+ Key.get(InvalidInterface.class).toString());
}
}
- static class A {
- @Inject B b;
- @Inject D d;
+ static class Invalid {
+ @Inject Valid a;
+ @Inject JitValid b;
+ @Inject Invalid(InvalidLinked a) {}
+ @Inject void foo(InvalidInterface a) {}
+
}
- static class B {
- @Inject A a;
+ @ImplementedBy(InvalidLinkedImpl.class)
+ static interface InvalidLinked {}
+ static class InvalidLinkedImpl implements InvalidLinked {
+ @Inject InvalidLinked2 a;
+ }
+
+ @ImplementedBy(InvalidLinked2Impl.class)
+ static interface InvalidLinked2 {}
+ static class InvalidLinked2Impl implements InvalidLinked2 {
+ @Inject InvalidLinked2Impl(Invalid2 a) {}
+ }
+
+ static class Invalid2 {
+ @Inject Invalid a;
}
- interface D {}
+ interface InvalidInterface {}
+
+ static class Valid { @Inject Valid2 a; }
+ static class Valid2 {}
+
+ static class JitValid { @Inject JitValid2 a; }
+ static class JitValid2 {}
+
+ /**
+ * Regression test for http://code.google.com/p/google-guice/issues/detail?id=319
+ *
+ * The bug is that a class that asks for a provider for itself during injection time,
+ * where any one of the other types required to fulfill the object creation was bound
+ * in a child constructor, explodes when the injected Provider is called.
+ *
+ * It works just fine when the other types are bound in a main injector.
+ */
+ public void testInstancesRequestingProvidersForThemselvesWithChildInjectors() {
+ final Module testModule = new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(String.class)
+ .toProvider(TestStringProvider.class);
+ }
+ };
+
+ // Verify it works when the type is setup in the parent.
+ Injector parentSetupRootInjector = Guice.createInjector(testModule);
+ Injector parentSetupChildInjector = parentSetupRootInjector.createChildInjector();
+ assertEquals(TestStringProvider.TEST_VALUE,
+ parentSetupChildInjector.getInstance(
+ RequiresProviderForSelfWithOtherType.class).getValue());
+
+ // Verify it works when the type is setup in the child, not the parent.
+ // If it still occurs, the bug will explode here.
+ Injector childSetupRootInjector = Guice.createInjector();
+ Injector childSetupChildInjector = childSetupRootInjector.createChildInjector(testModule);
+ assertEquals(TestStringProvider.TEST_VALUE,
+ childSetupChildInjector.getInstance(
+ RequiresProviderForSelfWithOtherType.class).getValue());
+ }
+
+ static class TestStringProvider implements Provider<String> {
+ static final String TEST_VALUE = "This is to verify it all works";
+
+ public String get() {
+ return TEST_VALUE;
+ }
+ }
+
+ static class RequiresProviderForSelfWithOtherType {
+ private final Provider<RequiresProviderForSelfWithOtherType> selfProvider;
+ private final String providedStringValue;
+
+ @Inject
+ RequiresProviderForSelfWithOtherType(
+ String providedStringValue,
+ Provider<RequiresProviderForSelfWithOtherType> selfProvider
+ ) {
+ this.providedStringValue = providedStringValue;
+ this.selfProvider = selfProvider;
+ }
+
+ public String getValue() {
+ // Attempt to get another instance of ourself. This pattern
+ // is possible for recursive processing.
+ selfProvider.get();
+
+ return providedStringValue;
+ }
+ }
+
}
