A fix to an unfortunate, rare concurrency problem. During injector creation, it's possbile that an @Inject method will start a thread, and attempt to access injected members while the injector is still being created. The IdentityHashSet of members to inject is currently accessed, and everything blows up. Terrible!
The fix is somewhat involved so I moved it all to a new class, CreationTimeMemberInjector. It takes care of preventing other threads from accessing injected members until all such members have been injected.
git-svn-id: https://google-guice.googlecode.com/svn/trunk@555 d779f126-a31b-0410-b53b-1d3aecad763e
diff --git a/src/com/google/inject/BindCommandProcessor.java b/src/com/google/inject/BindCommandProcessor.java
index 765a225..3467edd 100644
--- a/src/com/google/inject/BindCommandProcessor.java
+++ b/src/com/google/inject/BindCommandProcessor.java
@@ -46,19 +46,19 @@
private final Map<Class<? extends Annotation>, Scope> scopes;
private final List<CreationListener> creationListeners = Lists.newArrayList();
private final Map<Key<?>, BindingImpl<?>> bindings;
- private final Set<Object> outstandingInjections;
+ private final CreationTimeMemberInjector memberInjector;
private final List<Runnable> untargettedBindings = Lists.newArrayList();
BindCommandProcessor(Errors errors,
InjectorImpl injector,
Map<Class<? extends Annotation>, Scope> scopes,
Map<Key<?>, BindingImpl<?>> bindings,
- Set<Object> outstandingInjections) {
+ CreationTimeMemberInjector memberInjector) {
super(errors);
this.injector = injector;
this.scopes = scopes;
this.bindings = bindings;
- this.outstandingInjections = outstandingInjections;
+ this.memberInjector = memberInjector;
}
@Override public <T> Boolean visitBind(BindCommand<T> command) {
@@ -110,7 +110,7 @@
}
ConstantFactory<? extends T> factory = new ConstantFactory<T>(instance);
- outstandingInjections.add(instance);
+ memberInjector.add(instance);
InternalFactory<? extends T> scopedFactory
= Scopes.scope(key, injector, factory, scope);
putBinding(new InstanceBindingImpl<T>(injector, key, source, scopedFactory, instance));
@@ -120,7 +120,7 @@
public Void visitToProvider(Provider<? extends T> provider) {
InternalFactoryToProviderAdapter<? extends T> factory
= new InternalFactoryToProviderAdapter<T>(provider, source);
- outstandingInjections.add(provider);
+ memberInjector.add(provider);
InternalFactory<? extends T> scopedFactory
= Scopes.scope(key, injector, factory, scope);
putBinding(new ProviderInstanceBindingImpl<T>(
diff --git a/src/com/google/inject/CreationTimeMemberInjector.java b/src/com/google/inject/CreationTimeMemberInjector.java
new file mode 100644
index 0000000..1c9519c
--- /dev/null
+++ b/src/com/google/inject/CreationTimeMemberInjector.java
@@ -0,0 +1,116 @@
+/**
+ * Copyright (C) 2008 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.inject;
+
+import com.google.common.base.ReferenceType;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.google.inject.internal.Errors;
+import com.google.inject.internal.ErrorsException;
+import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+
+/**
+ * Injects all instances that were registered for injection at injector-creation time.
+ *
+ * @author jessewilson@google.com (Jesse Wilson)
+ */
+class CreationTimeMemberInjector {
+ /** the only thread that we'll use to inject members. */
+ private final Thread creatingThread = Thread.currentThread();
+
+ /** zero means everything is injected. */
+ private final CountDownLatch ready = new CountDownLatch(1);
+
+ private final Set<Object> outstandingInjections = Sets.newIdentityHashSet(ReferenceType.STRONG);
+ private final InjectorImpl injector;
+
+ CreationTimeMemberInjector(InjectorImpl injector) {
+ this.injector = injector;
+ }
+
+ public void add(Object instance) {
+ outstandingInjections.add(instance);
+ }
+
+ /**
+ * Prepares member injectors for all injected instances. This prompts Guice to do static analysis
+ * on the injected instances.
+ */
+ void validateOustandingInjections(Errors errors) {
+ for (Object toInject : outstandingInjections) {
+ try {
+ injector.getMemberInjectors(toInject.getClass());
+ } catch (ErrorsException e) {
+ errors.merge(e.getErrors());
+ }
+ }
+ }
+
+ /**
+ * Performs creation-time injections on all objects that require it. Whenever fulfilling an
+ * injection depends on another object that requires injection, we use {@link
+ * #ensureInjected(Errors, Object)} to inject that member first.
+ *
+ * <p>If the two objects are codependent (directly or transitively), ordering of injection is
+ * arbitrary.
+ */
+ void injectAll(final Errors errors) {
+ // loop over a defensive copy since ensureInjected() mutates the set
+ for (Object toInject : Lists.newArrayList(outstandingInjections)) {
+ try {
+ ensureInjected(errors, toInject);
+ } catch (ErrorsException e) {
+ errors.merge(e.getErrors());
+ }
+ }
+
+ if (!outstandingInjections.isEmpty()) {
+ throw new AssertionError("Failed to satisfy " + outstandingInjections);
+ }
+
+ ready.countDown();
+ }
+
+ /**
+ * Reentrant. If {@code toInject} was registered for injection at injector-creation time, this
+ * method will ensure that all its members have been injected before returning. This method is
+ * used both internally, and by {@code InternalContext} to satisfy injections while satisfying
+ * other injections.
+ */
+ void ensureInjected(Errors errors, Object toInject) throws ErrorsException {
+ if (ready.getCount() == 0) {
+ return;
+ }
+
+ // just wait for everything to be injected by another thread
+ if (Thread.currentThread() != creatingThread) {
+ try {
+ ready.await();
+ return;
+ } catch (InterruptedException e) {
+ // Give up, since we don't know if our injection is ready
+ throw new RuntimeException(e);
+ }
+ }
+
+ // toInject needs injection, do it right away
+ if (outstandingInjections.remove(toInject)) {
+ injector.injectMembersOrThrow(errors, toInject);
+ }
+ }
+}
\ No newline at end of file
diff --git a/src/com/google/inject/InjectorBuilder.java b/src/com/google/inject/InjectorBuilder.java
index 17033f5..40df477 100644
--- a/src/com/google/inject/InjectorBuilder.java
+++ b/src/com/google/inject/InjectorBuilder.java
@@ -137,7 +137,7 @@
bindLogger();
bindCommandProcesor = new BindCommandProcessor(errors,
injector, injector.scopes, injector.explicitBindings,
- injector.outstandingInjections);
+ injector.memberInjector);
bindCommandProcesor.processCommands(commands);
bindCommandProcesor.createUntargettedBindings();
stopwatch.resetAndLog("Binding creation");
@@ -146,7 +146,7 @@
stopwatch.resetAndLog("Binding indexing");
requestInjectionCommandProcessor
- = new RequestInjectionCommandProcessor(errors, injector.outstandingInjections);
+ = new RequestInjectionCommandProcessor(errors, injector.memberInjector);
requestInjectionCommandProcessor.processCommands(commands);
stopwatch.resetAndLog("Static injection");
}
@@ -159,7 +159,7 @@
requestInjectionCommandProcessor.validate(injector);
stopwatch.resetAndLog("Static validation");
- injector.validateOustandingInjections(errors);
+ injector.memberInjector.validateOustandingInjections(errors);
stopwatch.resetAndLog("Instance member validation");
new GetProviderProcessor(errors, injector).processCommands(commands);
@@ -173,7 +173,7 @@
requestInjectionCommandProcessor.injectMembers(injector);
stopwatch.resetAndLog("Static member injection");
- injector.fulfillOutstandingInjections(errors);
+ injector.memberInjector.injectAll(errors);
stopwatch.resetAndLog("Instance injection");
errors.throwCreationExceptionIfErrorsExist();
diff --git a/src/com/google/inject/InjectorImpl.java b/src/com/google/inject/InjectorImpl.java
index 4b3c282..e8629dd 100644
--- a/src/com/google/inject/InjectorImpl.java
+++ b/src/com/google/inject/InjectorImpl.java
@@ -17,12 +17,10 @@
package com.google.inject;
import static com.google.common.base.Preconditions.checkState;
-import com.google.common.base.ReferenceType;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Multimaps;
-import com.google.common.collect.Sets;
import com.google.inject.internal.BytecodeGen.Visibility;
import static com.google.inject.internal.BytecodeGen.newFastClass;
import com.google.inject.internal.Classes;
@@ -55,7 +53,6 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import net.sf.cglib.reflect.FastClass;
import net.sf.cglib.reflect.FastMethod;
@@ -72,56 +69,13 @@
final Map<Class<? extends Annotation>, Scope> scopes = Maps.newHashMap();
final List<MatcherAndConverter> converters = Lists.newArrayList();
final Map<Key<?>, BindingImpl<?>> parentBindings = Maps.newHashMap();
- final Set<Object> outstandingInjections = Sets.newIdentityHashSet(ReferenceType.STRONG);
+ final CreationTimeMemberInjector memberInjector = new CreationTimeMemberInjector(this);
Reflection reflection;
InjectorImpl(Injector parentInjector) {
this.parentInjector = parentInjector;
}
- void validateOustandingInjections(Errors errors) {
- for (Object toInject : outstandingInjections) {
- try {
- getMemberInjectors(toInject.getClass());
- } catch (ErrorsException e) {
- errors.merge(e.getErrors());
- }
- }
- }
-
- /**
- * Performs creation-time injections on all objects that require it. Whenever fulfilling an
- * injection depends on another object that requires injection, we use {@link
- * InternalContext#ensureMemberInjected} to inject that member first.
- *
- * <p>If the two objects are codependent (directly or transitively), ordering of injection is
- * arbitrary.
- */
- void fulfillOutstandingInjections(final Errors errors) {
- try {
- callInContext(new ContextualCallable<Void>() {
- public Void call(InternalContext context) {
- // loop over a defensive copy, since ensureMemberInjected() mutates the set
- for (Object toInject : Lists.newArrayList(outstandingInjections)) {
- try {
- context.ensureMemberInjected(errors, toInject);
- } catch (ErrorsException e) {
- errors.merge(e.getErrors());
- }
- }
- return null;
- }
- });
- }
- catch (ErrorsException e) {
- throw new AssertionError(e);
- }
-
- if (!outstandingInjections.isEmpty()) {
- throw new AssertionError("failed to satisfy " + outstandingInjections);
- }
- }
-
/** Indexes bindings by type. */
void index() {
for (BindingImpl<?> binding : explicitBindings.values()) {
diff --git a/src/com/google/inject/InternalContext.java b/src/com/google/inject/InternalContext.java
index 143c4f3..b613344 100644
--- a/src/com/google/inject/InternalContext.java
+++ b/src/com/google/inject/InternalContext.java
@@ -74,10 +74,6 @@
* been injected before its use.
*/
public void ensureMemberInjected(Errors errors, Object toInject) throws ErrorsException {
- if (!injector.outstandingInjections.remove(toInject)) {
- return;
- }
-
- injector.injectMembersOrThrow(errors, toInject);
+ injector.memberInjector.ensureInjected(errors, toInject);
}
}
diff --git a/src/com/google/inject/RequestInjectionCommandProcessor.java b/src/com/google/inject/RequestInjectionCommandProcessor.java
index 30a1551..10c91dd 100644
--- a/src/com/google/inject/RequestInjectionCommandProcessor.java
+++ b/src/com/google/inject/RequestInjectionCommandProcessor.java
@@ -23,7 +23,6 @@
import com.google.inject.internal.Errors;
import com.google.inject.internal.ErrorsException;
import java.util.List;
-import java.util.Set;
/**
* Handles {@link Binder#requestInjection} and {@link Binder#requestStaticInjection} commands.
@@ -35,12 +34,12 @@
class RequestInjectionCommandProcessor extends CommandProcessor {
private final List<StaticInjection> staticInjections = Lists.newArrayList();
- private final Set<Object> outstandingInjections;
+ private final CreationTimeMemberInjector memberInjector;
RequestInjectionCommandProcessor(Errors errors,
- Set<Object> outstandingInjections) {
+ CreationTimeMemberInjector memberInjector) {
super(errors);
- this.outstandingInjections = outstandingInjections;
+ this.memberInjector = memberInjector;
}
@Override public Boolean visitRequestStaticInjection(RequestStaticInjectionCommand command) {
@@ -52,7 +51,7 @@
@Override public Boolean visitRequestInjection(RequestInjectionCommand command) {
for (Object instance : command.getInstances()) {
- outstandingInjections.add(instance);
+ memberInjector.add(instance);
}
return true;
}
diff --git a/test/com/google/inject/BindingOrderTest.java b/test/com/google/inject/BindingOrderTest.java
index 91e4772..2e48d44 100644
--- a/test/com/google/inject/BindingOrderTest.java
+++ b/test/com/google/inject/BindingOrderTest.java
@@ -16,6 +16,8 @@
package com.google.inject;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
import junit.framework.TestCase;
/**
@@ -47,12 +49,43 @@
}
});
- // For untargetted bindings with scopes, sometimes we lose the scope at
- // injector time. This is because we use the injector's just-in-time
- // bindings to build these, rather than the bind command. This is a known
- // bug.
- assertSame("known bug: untargetted binding out-of-order",
- injector.getInstance(A.class).b, injector.getInstance(A.class).b);
+ assertSame(injector.getInstance(A.class).b, injector.getInstance(A.class).b);
+ }
+
+ public void testBindingWithExtraThreads() throws InterruptedException {
+ final CountDownLatch ready = new CountDownLatch(1);
+ final CountDownLatch done = new CountDownLatch(1);
+ final AtomicReference<B> ref = new AtomicReference<B>();
+
+ final Object createsAThread = new Object() {
+ @Inject void createAnotherThread(final Injector injector) {
+ new Thread() {
+ public void run() {
+ ready.countDown();
+ A a = injector.getInstance(A.class);
+ ref.set(a.b);
+ done.countDown();
+ }
+ }.start();
+
+ // to encourage collisions, we make sure the other thread is running before returning
+ try {
+ ready.await();
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ };
+
+ Guice.createInjector(new AbstractModule() {
+ protected void configure() {
+ requestInjection(createsAThread);
+ bind(A.class).toInstance(new A());
+ }
+ });
+
+ done.await();
+ assertNotNull(ref.get());
}
static class A {
