Revision: 1584
Author: [email protected]
Date: Tue Sep 13 10:41:32 2011
Log:
Fix a bug in the way failed JIT bindings are cleaned up. Because we
removed bindings from the jitBindings Map, it was possible for an Injector
to try and recreate the failed JIT binding. Normally we want this
behavior.. but in the case of a circular failed JIT binding, it can lead to
a ComputationException from ComputingConcurrentHashMap, because we attempt
to create two ConstructorInjectors for the same InjectionPoint recursively.
Revision created by MOE tool push_codebase.
MOE_MIGRATION=3242
http://code.google.com/p/google-guice/source/detail?r=1584
Modified:
/trunk/core/src/com/google/inject/internal/ConstructorInjectorStore.java
/trunk/core/src/com/google/inject/internal/InjectorImpl.java
/trunk/core/test/com/google/inject/BinderTest.java
/trunk/core/test/com/google/inject/ImplicitBindingTest.java
=======================================
---
/trunk/core/src/com/google/inject/internal/ConstructorInjectorStore.java
Thu Jul 7 17:34:16 2011
+++
/trunk/core/src/com/google/inject/internal/ConstructorInjectorStore.java
Tue Sep 13 10:41:32 2011
@@ -31,6 +31,7 @@
private final FailableCache<InjectionPoint, ConstructorInjector<?>>
cache
= new FailableCache<InjectionPoint, ConstructorInjector<?>> () {
+ @Override
protected ConstructorInjector<?> create(InjectionPoint
constructorInjector, Errors errors)
throws ErrorsException {
return createConstructor(constructorInjector, errors);
=======================================
--- /trunk/core/src/com/google/inject/internal/InjectorImpl.java Fri Jul 22
14:13:53 2011
+++ /trunk/core/src/com/google/inject/internal/InjectorImpl.java Tue Sep 13
10:41:32 2011
@@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
import com.google.inject.Binder;
import com.google.inject.Binding;
import com.google.inject.ConfigurationException;
@@ -104,6 +105,11 @@
/** Just-in-time binding cache. Guarded by state.lock() */
final Map<Key<?>, BindingImpl<?>> jitBindings = Maps.newHashMap();
+ /**
+ * Cache of Keys that we were unable to create JIT bindings for, so we
don't
+ * keep trying. Also guarded by state.lock().
+ */
+ final Set<Key<?>> failedJitBindings = Sets.newHashSet();
Lookups lookups = new DeferredLookups(this);
@@ -116,6 +122,7 @@
localContext = parent.localContext;
} else {
localContext = new ThreadLocal<Object[]>() {
+ @Override
protected Object[] initialValue() {
return new Object[1];
}
@@ -231,7 +238,7 @@
private <T> BindingImpl<T> getJustInTimeBinding(Key<T> key, Errors
errors, JitLimitation jitType)
throws ErrorsException {
- boolean jitOverride = isProvider(key) || isTypeLiteral(key) ||
isMembersInjector(key);
+ boolean jitOverride = isProvider(key) || isTypeLiteral(key) ||
isMembersInjector(key);
synchronized (state.lock()) {
// first try to find a JIT binding that we've already created
for (InjectorImpl injector = this; injector != null; injector =
injector.parent) {
@@ -252,8 +259,25 @@
}
}
+ // If we previously failed creating this JIT binding and our Errors
has
+ // already recorded an error, then just directly throw that error.
+ // We need to do this because it's possible we already cleaned up the
+ // entry in jitBindings (during cleanup), and we may be trying
+ // to create it again (in the case of a recursive JIT binding).
+ // We need both of these guards for different reasons
+ // failedJitBindings.contains: We want to continue processing if
we've never
+ // failed before, so that our initial error message contains
+ // as much useful information as possible about what errors exist.
+ // errors.hasErrors: If we haven't already failed, then it's OK to
+ // continue processing, to make sure the ultimate error message
+ // is the correct one.
+ // See: ImplicitBindingsTest#testRecursiveJitBindingsCleanupCorrectly
+ // for where this guard compes into play.
+ if (failedJitBindings.contains(key) && errors.hasErrors()) {
+ throw errors.toException();
+ }
return createJustInTimeBindingRecursive(key, errors,
options.jitDisabled, jitType);
- }
+ } // end synchronized(state.lock())
}
/** Returns true if the key type is Provider (but not a subclass of
Provider). */
@@ -570,6 +594,7 @@
/** Cleans up any state that may have been cached when constructing the
JIT binding. */
private void removeFailedJitBinding(Key<?> key, InjectionPoint ip) {
+ failedJitBindings.add(key);
jitBindings.remove(key);
membersInjectorStore.remove(key.getTypeLiteral());
provisionListenerStore.remove(key);
@@ -1019,6 +1044,7 @@
}
}
+ @Override
public String toString() {
return Objects.toStringHelper(Injector.class)
.add("bindings", state.getExplicitBindingsThisLevel().values())
=======================================
--- /trunk/core/test/com/google/inject/BinderTest.java Thu Jul 7 17:34:16
2011
+++ /trunk/core/test/com/google/inject/BinderTest.java Tue Sep 13 10:41:32
2011
@@ -47,11 +47,13 @@
private final List<LogRecord> logRecords = Lists.newArrayList();
private final Handler fakeHandler = new Handler() {
+ @Override
public void publish(LogRecord logRecord) {
logRecords.add(logRecord);
}
-
+ @Override
public void flush() {}
+ @Override
public void close() throws SecurityException {}
};
@@ -86,6 +88,7 @@
public void testMissingBindings() {
try {
Guice.createInjector(new AbstractModule() {
+ @Override
public void configure() {
getProvider(Runnable.class);
bind(Comparator.class);
@@ -111,6 +114,7 @@
public void testMissingDependency() {
try {
Guice.createInjector(new AbstractModule() {
+ @Override
public void configure() {
bind(NeedsRunnable.class);
}
@@ -232,6 +236,7 @@
final Integer[] integers = new Integer[] { 1 };
Injector injector = Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
bind(String[].class).toInstance(strings);
bind(new TypeLiteral<Integer[]>() {}).toInstance(integers);
@@ -247,6 +252,7 @@
try {
Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
bind(String[].class).toInstance(new String[] { "A" });
bind(new TypeLiteral<String[]>() {}).toInstance(new String[]
{ "B" });
@@ -262,6 +268,7 @@
// passes because duplicates are ignored
injector = Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
bind(String[].class).toInstance(strings);
bind(new TypeLiteral<String[]>() {}).toInstance(strings);
@@ -354,6 +361,7 @@
*/
public void testUntargettedBinding() {
Injector injector = Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
bind(HasProvidedBy1.class);
bind(HasImplementedBy1.class);
@@ -388,6 +396,7 @@
final Message message = new Message(getClass(), "Whoops!");
try {
Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
addError(message);
}
@@ -401,6 +410,7 @@
public void testUserReportedErrorsAreAlsoLogged() {
try {
Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
addError(new Message(ImmutableList.of(), "Whoops!", new
IllegalArgumentException()));
}
@@ -417,6 +427,7 @@
public void testBindingToProvider() {
try {
Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
bind(new TypeLiteral<Provider<String>>()
{}).toInstance(Providers.of("A"));
}
@@ -434,6 +445,7 @@
try {
Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
bind(AbstractModule.class).annotatedWith(red)
.toProvider(Providers.<AbstractModule>of(null));
=======================================
--- /trunk/core/test/com/google/inject/ImplicitBindingTest.java Thu Jul 7
17:34:16 2011
+++ /trunk/core/test/com/google/inject/ImplicitBindingTest.java Tue Sep 13
10:41:32 2011
@@ -19,6 +19,7 @@
import com.google.common.collect.Iterables;
import com.google.inject.name.Named;
import com.google.inject.name.Names;
+import com.google.inject.spi.Message;
import junit.framework.TestCase;
@@ -74,6 +75,7 @@
public void testBindingOverridesImplementedBy() {
Injector injector = Guice.createInjector(new AbstractModule() {
+ @Override
protected void configure() {
bind(I.class).to(AlternateImpl.class);
}
@@ -158,7 +160,10 @@
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();
+ Message msg = Iterables.getOnlyElement(expected.getErrorMessages());
+ assertEquals("No implementation for " +
InvalidInterface.class.getName() + " was bound.",
+ msg.getMessage());
+ List<Object> sources = msg.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());
@@ -281,6 +286,62 @@
return providedStringValue;
}
- }
-
-}
+ }
+
+ /**
+ * Ensure that when we cleanup failed JIT bindings, we don't break.
+ * The test here requires a sequence of JIT bindings:
+ * A-> B
+ * B -> C, A
+ * C -> A, D
+ * D not JITable
+ * The problem was that C cleaned up A's binding and then handed control
back to B,
+ * which tried to continue processing A.. but A was removed from the
jitBindings Map,
+ * so it attempts to create a new JIT binding for A, but we haven't yet
finished
+ * constructing the first JIT binding for A, so we get a recursive
+ * computation exception from ComputingConcurrentHashMap.
+ *
+ * We also throw in a valid JIT binding, E, to guarantee that if
+ * something fails in this flow, it can be recreated later if it's
+ * not from a failed sequence.
+ */
+ public void testRecursiveJitBindingsCleanupCorrectly() throws Exception {
+ Injector injector = Guice.createInjector();
+ try {
+ injector.getInstance(A.class);
+ fail("Expected failure");
+ } catch(ConfigurationException expected) {
+ Message msg = Iterables.getOnlyElement(expected.getErrorMessages());
+ Asserts.assertContains(msg.getMessage(),
+ "Could not find a suitable constructor in " + D.class.getName());
+ }
+ // Assert that we've removed all the bindings.
+ assertNull(injector.getExistingBinding(Key.get(A.class)));
+ assertNull(injector.getExistingBinding(Key.get(B.class)));
+ assertNull(injector.getExistingBinding(Key.get(C.class)));
+ assertNull(injector.getExistingBinding(Key.get(D.class)));
+
+ // Confirm that we didn't prevent 'E' from working.
+ assertNotNull(injector.getBinding(Key.get(E.class)));
+ }
+
+ static class A {
+ @Inject public A(B b) {}
+ }
+
+ static class B {
+ @Inject public B(C c, A a) {}
+ }
+
+ static class C {
+ @Inject public C(A a, D d, E e) {}
+ }
+
+ static class D {
+ public D(int i) {}
+ }
+
+ // Valid JITable binding
+ static class E { }
+
+}
--
You received this message because you are subscribed to the Google Groups
"google-guice-dev" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/google-guice-dev?hl=en.
