Hierarchical Parent Child Chained injectors are now threadsafe
git-svn-id: https://google-guice.googlecode.com/svn/trunk@662 d779f126-a31b-0410-b53b-1d3aecad763e
diff --git a/src/com/google/inject/BindingProcessor.java b/src/com/google/inject/BindingProcessor.java
index 8c6df23..42f9634 100644
--- a/src/com/google/inject/BindingProcessor.java
+++ b/src/com/google/inject/BindingProcessor.java
@@ -27,7 +27,6 @@
import com.google.inject.spi.InjectionPoint;
import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
-import java.lang.reflect.Type;
import java.util.List;
import java.util.Set;
@@ -169,8 +168,6 @@
}
public Void visitUntargetted() {
- final Type type = key.getTypeLiteral().getType();
-
// Error: Missing implementation.
// Example: bind(Date.class).annotatedWith(Red.class);
// We can't assume abstract types aren't injectable. They may have an
@@ -259,8 +256,6 @@
return;
}
- // TODO: make getExplicitBinding() and blacklist() atomic
-
// prevent the parent from creating a JIT binding for this key
state.parent().blacklist(key);
state.putBinding(key, binding);
diff --git a/src/com/google/inject/InheritingState.java b/src/com/google/inject/InheritingState.java
index 0d08429..2faa186 100644
--- a/src/com/google/inject/InheritingState.java
+++ b/src/com/google/inject/InheritingState.java
@@ -31,8 +31,6 @@
*/
class InheritingState implements State {
- // TODO(jessewilson): think about what we need to do w.r.t. concurrency
-
private final State parent;
private final Map<Key<?>, Binding<?>> explicitBindingsMutable = Maps.newHashMap();
private final Map<Key<?>, Binding<?>> explicitBindings
@@ -41,9 +39,11 @@
private final List<MatcherAndConverter> converters = Lists.newArrayList();
private final List<MethodAspect> methodAspects = Lists.newArrayList();
private final WeakKeySet blacklistedKeys = new WeakKeySet();
+ private final Object lock;
InheritingState(State parent) {
this.parent = checkNotNull(parent, "parent");
+ this.lock = (parent == State.NONE) ? this : parent.lock();
}
public State parent() {
@@ -116,4 +116,8 @@
public boolean isBlacklisted(Key<?> key) {
return blacklistedKeys.contains(key);
}
+
+ public Object lock() {
+ return lock;
+ }
}
diff --git a/src/com/google/inject/InjectorBuilder.java b/src/com/google/inject/InjectorBuilder.java
index cf47351..8932423 100644
--- a/src/com/google/inject/InjectorBuilder.java
+++ b/src/com/google/inject/InjectorBuilder.java
@@ -36,7 +36,16 @@
import java.util.logging.Logger;
/**
- * Builds a dependency injection {@link Injector}.
+ * Builds a dependency injection {@link Injector}. Injector construction happens in two phases.
+ * <ol>
+ * <li>Static building. In this phase, we interpret commands, create bindings, and inspect
+ * dependencies. During this phase, we hold a lock to ensure consistency with parent injectors.
+ * No user code is executed in this phase.</li>
+ * <li>Dynamic injection. In this phase, we call user code. We inject members that requested
+ * injection. This may require user's objects be created and their providers be called. And we
+ * create eager singletons. In this phase, user code may have started other threads. This phase
+ * is not executed for injectors created using {@link Stage#TOOL the tool stage}</li>
+ * </ol>
*
* @author crazybob@google.com (Bob Lee)
* @author jessewilson@google.com (Jesse Wilson)
@@ -85,69 +94,71 @@
injector = new InjectorImpl(parent);
- // bind Stage and Singleton if this is a top-level injector
- if (parent == null) {
- modules.add(0, new RootModule(stage));
- }
+ buildStatically();
- elements.addAll(Elements.getElements(stage, modules));
-
- buildCoreInjector();
-
- validate();
-
- errors.throwCreationExceptionIfErrorsExist();
-
- // If we're in the tool stage, stop here. Don't eagerly inject or load
- // anything.
+ // If we're in the tool stage, stop here. Don't eagerly inject or load anything.
if (stage == Stage.TOOL) {
return new ToolStageInjector(injector);
}
- fulfillInjectionRequests();
-
- if (!elements.isEmpty()) {
- throw new AssertionError("Failed to execute " + elements);
- }
+ injectDynamically();
return injector;
}
- /** Builds the injector. */
- private void buildCoreInjector() {
- new MessageProcessor(errors)
- .processCommands(elements);
+ /**
+ * Synchronize while we're building up the bindings and other injector state. This ensures that
+ * the JIT bindings in the parent injector don't change while we're being built
+ */
+ void buildStatically() {
+ synchronized (injector.state.lock()) {
+ // bind Stage and Singleton if this is a top-level injector
+ if (parent == null) {
+ modules.add(0, new RootModule(stage));
+ }
- InterceptorBindingProcessor interceptorCommandProcessor
- = new InterceptorBindingProcessor(errors, injector.state);
- interceptorCommandProcessor.processCommands(elements);
- injector.constructionProxyFactory = interceptorCommandProcessor.createProxyFactory();
- stopwatch.resetAndLog("Interceptors creation");
+ elements.addAll(Elements.getElements(stage, modules));
+ stopwatch.resetAndLog("Module execution");
- new ScopeBindingProcessor(errors, injector.state).processCommands(elements);
- stopwatch.resetAndLog("Scopes creation");
+ new MessageProcessor(errors).processCommands(elements);
- new TypeConverterBindingProcessor(errors, injector.state).processCommands(elements);
- stopwatch.resetAndLog("Converters creation");
+ InterceptorBindingProcessor interceptorCommandProcessor
+ = new InterceptorBindingProcessor(errors, injector.state);
+ interceptorCommandProcessor.processCommands(elements);
+ injector.constructionProxyFactory = interceptorCommandProcessor.createProxyFactory();
+ stopwatch.resetAndLog("Interceptors creation");
- bindInjector();
- bindLogger();
- bindCommandProcesor = new BindingProcessor(errors,
- injector, injector.state, injector.initializer);
- bindCommandProcesor.processCommands(elements);
- bindCommandProcesor.createUntargettedBindings();
- stopwatch.resetAndLog("Binding creation");
+ new ScopeBindingProcessor(errors, injector.state).processCommands(elements);
+ stopwatch.resetAndLog("Scopes creation");
- injector.index();
- stopwatch.resetAndLog("Binding indexing");
+ new TypeConverterBindingProcessor(errors, injector.state).processCommands(elements);
+ stopwatch.resetAndLog("Converters creation");
- injectionCommandProcessor = new InjectionRequestProcessor(errors, injector.initializer);
- injectionCommandProcessor.processCommands(elements);
- stopwatch.resetAndLog("Static injection");
+ bindInjector();
+ bindLogger();
+ bindCommandProcesor = new BindingProcessor(errors,
+ injector, injector.state, injector.initializer);
+ bindCommandProcesor.processCommands(elements);
+ bindCommandProcesor.createUntargettedBindings();
+ stopwatch.resetAndLog("Binding creation");
+
+ injector.index();
+ stopwatch.resetAndLog("Binding indexing");
+
+ injectionCommandProcessor = new InjectionRequestProcessor(errors, injector.initializer);
+ injectionCommandProcessor.processCommands(elements);
+ stopwatch.resetAndLog("Static injection");
+
+ validateStatically();
+
+ if (!elements.isEmpty()) {
+ throw new AssertionError("Failed to execute " + elements);
+ }
+ }
}
/** Validate everything that we can validate now that the injector is ready for use. */
- private void validate() {
+ private void validateStatically() {
bindCommandProcesor.runCreationListeners(injector);
stopwatch.resetAndLog("Validation");
@@ -163,8 +174,12 @@
errors.throwCreationExceptionIfErrorsExist();
}
- /** Inject everything that can be injected. */
- private void fulfillInjectionRequests() {
+ /**
+ * Inject everything that can be injected. This method is intentionally not synchronized. If we
+ * locked while injecting members (ie. running user code), things would deadlock should the user
+ * code build a just-in-time binding from another thread.
+ */
+ private void injectDynamically() {
injectionCommandProcessor.injectMembers(injector);
stopwatch.resetAndLog("Static member injection");
diff --git a/src/com/google/inject/InjectorImpl.java b/src/com/google/inject/InjectorImpl.java
index 54908af..a827c46 100644
--- a/src/com/google/inject/InjectorImpl.java
+++ b/src/com/google/inject/InjectorImpl.java
@@ -152,7 +152,7 @@
// TODO: synch should span parent and child
- synchronized (jitBindings) {
+ synchronized (state.lock()) {
// try to get the JIT binding from the parent injector
if (parent != null) {
try {
@@ -321,7 +321,7 @@
<T> void initializeBinding(BindingImpl<T> binding, Errors errors) throws ErrorsException {
// 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 jitBindings during injector creation.
+ // Note: We don't need to synchronize on state.lock() during injector creation.
if (binding instanceof ClassBindingImpl<?>) {
Key<T> key = binding.getKey();
jitBindings.put(key, binding);
diff --git a/src/com/google/inject/State.java b/src/com/google/inject/State.java
index 580eeae..23ea08e 100644
--- a/src/com/google/inject/State.java
+++ b/src/com/google/inject/State.java
@@ -84,6 +84,10 @@
public boolean isBlacklisted(Key<?> key) {
return true;
}
+
+ public Object lock() {
+ throw new UnsupportedOperationException();
+ }
};
State parent();
@@ -126,4 +130,10 @@
* one of this injector's descendent's has bound the key.
*/
boolean isBlacklisted(Key<?> key);
+
+ /**
+ * Returns the shared lock for all injector data. This is a low-granularity, high-contention lock
+ * to be used when reading mutable data (ie. just-in-time bindings, and binding blacklists).
+ */
+ Object lock();
}
diff --git a/test/com/google/inject/InjectorTest.java b/test/com/google/inject/InjectorTest.java
index 4f339de..733e399 100644
--- a/test/com/google/inject/InjectorTest.java
+++ b/test/com/google/inject/InjectorTest.java
@@ -21,6 +21,12 @@
import java.io.IOException;
import java.lang.annotation.Retention;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicReference;
import junit.framework.TestCase;
/**
@@ -387,4 +393,29 @@
return this;
}
}
+
+ public void testJitBindingFromAnotherThreadDuringInjection() {
+ final ExecutorService executorService = Executors.newSingleThreadExecutor();
+ final AtomicReference<JustInTime> got = new AtomicReference<JustInTime>();
+
+ Guice.createInjector(new AbstractModule() {
+ protected void configure() {
+ requestInjection(new Object() {
+ @Inject void initialize(final Injector injector)
+ throws ExecutionException, InterruptedException {
+ Future<JustInTime> future = executorService.submit(new Callable<JustInTime>() {
+ public JustInTime call() throws Exception {
+ return injector.getInstance(JustInTime.class);
+ }
+ });
+ got.set(future.get());
+ }
+ });
+ }
+ });
+
+ assertNotNull(got.get());
+ }
+
+ static class JustInTime {}
}
