Implement more granular locks for a Singleton scope in Guice.
Now when you can create two independent singletons using
the same injector in different threads.
This make it easy to create scopes creating singletons using
thread pools with all the concurrency being done by Guice.
As a nice side effect Singleton scope is no longer treated
specially in Guice codebase.
The obvious problem to solve is potential deadlocks:
A requires B, B requires C, C requires A where all are
singletons and all are created simultaneously.
It's impossible to detect this deadlock using information
within one thread, so we have to have a shared storage.
An idea is to have a map of creators' locks and a map
of which threads are waiting for other singletons to be created.
Using this information circular dependencies are trivially
discovered within O(N) where N is a number of concurrent threads.
Important to not that no other deadlock scenarios within
Guice code is introduced as Guice does not expose any
other scopes that can span several threads.
Now it would be possible for
client code to deadlock on itself with two lazy singletons
calling each other's providers during creation.
This is deemed as a non-issue as it is up to the client
to write a thread-safe code.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=91610630
diff --git a/core/src/com/google/inject/internal/BindingProcessor.java b/core/src/com/google/inject/internal/BindingProcessor.java
index 8e83d18..b3c869e 100644
--- a/core/src/com/google/inject/internal/BindingProcessor.java
+++ b/core/src/com/google/inject/internal/BindingProcessor.java
@@ -111,7 +111,7 @@
// always visited with Binding<T>
@SuppressWarnings("unchecked")
InternalFactory<T> factory = new InternalFactoryToInitializableAdapter<T>(
- initializable, source, !injector.options.disableCircularProxies,
+ initializable, source,
injector.provisionListenerStore.get((ProviderInstanceBinding<T>)binding));
InternalFactory<? extends T> scopedFactory
= Scoping.scope(key, injector, factory, source, scoping);
@@ -127,7 +127,7 @@
// always visited with Binding<T>
@SuppressWarnings("unchecked")
BoundProviderFactory<T> boundProviderFactory = new BoundProviderFactory<T>(
- injector, providerKey, source, !injector.options.disableCircularProxies,
+ injector, providerKey, source,
injector.provisionListenerStore.get((ProviderKeyBinding<T>) binding));
bindingData.addCreationListener(boundProviderFactory);
InternalFactory<? extends T> scopedFactory = Scoping.scope(
diff --git a/core/src/com/google/inject/internal/BoundProviderFactory.java b/core/src/com/google/inject/internal/BoundProviderFactory.java
index 27ae1aa..9f523df 100644
--- a/core/src/com/google/inject/internal/BoundProviderFactory.java
+++ b/core/src/com/google/inject/internal/BoundProviderFactory.java
@@ -38,9 +38,8 @@
InjectorImpl injector,
Key<? extends javax.inject.Provider<? extends T>> providerKey,
Object source,
- boolean allowProxy,
ProvisionListenerStackCallback<T> provisionCallback) {
- super(source, allowProxy);
+ super(source);
this.provisionCallback = checkNotNull(provisionCallback, "provisionCallback");
this.injector = injector;
this.providerKey = providerKey;
@@ -60,7 +59,7 @@
try {
errors = errors.withSource(providerKey);
javax.inject.Provider<? extends T> provider = providerFactory.get(errors, context, dependency, true);
- return circularGet(provider, errors, context, dependency, linked, provisionCallback);
+ return circularGet(provider, errors, context, dependency, provisionCallback);
} finally {
context.popState();
}
diff --git a/core/src/com/google/inject/internal/ConstructionContext.java b/core/src/com/google/inject/internal/ConstructionContext.java
index 8a5be01..ced388f 100644
--- a/core/src/com/google/inject/internal/ConstructionContext.java
+++ b/core/src/com/google/inject/internal/ConstructionContext.java
@@ -16,6 +16,8 @@
package com.google.inject.internal;
+import com.google.inject.internal.InjectorImpl.InjectorOptions;
+
import java.lang.reflect.Proxy;
import java.util.ArrayList;
import java.util.List;
@@ -57,11 +59,11 @@
invocationHandlers = null;
}
- public Object createProxy(Errors errors, Class<?> expectedType) throws ErrorsException {
- // TODO: if I create a proxy which implements all the interfaces of
- // the implementation type, I'll be able to get away with one proxy
- // instance (as opposed to one per caller).
-
+ public Object createProxy(Errors errors, InjectorOptions injectorOptions,
+ Class<?> expectedType) throws ErrorsException {
+ if (injectorOptions.disableCircularProxies) {
+ throw errors.circularProxiesDisabled(expectedType).toException();
+ }
if (!expectedType.isInterface()) {
throw errors.cannotSatisfyCircularDependency(expectedType).toException();
}
@@ -73,6 +75,9 @@
DelegatingInvocationHandler<T> invocationHandler = new DelegatingInvocationHandler<T>();
invocationHandlers.add(invocationHandler);
+ // TODO: if I create a proxy which implements all the interfaces of
+ // the implementation type, I'll be able to get away with one proxy
+ // instance (as opposed to one per caller).
ClassLoader classLoader = BytecodeGen.getClassLoader(expectedType);
return expectedType.cast(Proxy.newProxyInstance(classLoader,
new Class[] { expectedType, CircularDependencyProxy.class }, invocationHandler));
@@ -83,6 +88,8 @@
for (DelegatingInvocationHandler<T> handler : invocationHandlers) {
handler.setDelegate(delegate);
}
+ // initialization of each handler can happen no more than once
+ invocationHandlers = null;
}
}
}
diff --git a/core/src/com/google/inject/internal/ConstructorBindingImpl.java b/core/src/com/google/inject/internal/ConstructorBindingImpl.java
index 1e8bf78..8fb2103 100644
--- a/core/src/com/google/inject/internal/ConstructorBindingImpl.java
+++ b/core/src/com/google/inject/internal/ConstructorBindingImpl.java
@@ -133,7 +133,6 @@
@SuppressWarnings("unchecked") // the result type always agrees with the ConstructorInjector type
public void initialize(InjectorImpl injector, Errors errors) throws ErrorsException {
- factory.allowCircularProxy = !injector.options.disableCircularProxies;
factory.constructorInjector =
(ConstructorInjector<T>) injector.constructors.get(constructorInjectionPoint, errors);
factory.provisionCallback =
@@ -246,7 +245,6 @@
private static class Factory<T> implements InternalFactory<T> {
private final boolean failIfNotLinked;
private final Key<?> key;
- private boolean allowCircularProxy;
private ConstructorInjector<T> constructorInjector;
private ProvisionListenerStackCallback<T> provisionCallback;
@@ -267,7 +265,7 @@
// This may not actually be safe because it could return a super type of T (if that's all the
// client needs), but it should be OK in practice thanks to the wonders of erasure.
return (T) constructorInjector.construct(errors, context,
- dependency.getKey().getTypeLiteral().getRawType(), allowCircularProxy, provisionCallback);
+ dependency.getKey().getTypeLiteral().getRawType(), provisionCallback);
}
}
}
diff --git a/core/src/com/google/inject/internal/ConstructorInjector.java b/core/src/com/google/inject/internal/ConstructorInjector.java
index ca7bb27..1ff4be1 100644
--- a/core/src/com/google/inject/internal/ConstructorInjector.java
+++ b/core/src/com/google/inject/internal/ConstructorInjector.java
@@ -59,19 +59,16 @@
* it may return a proxy.
*/
Object construct(final Errors errors, final InternalContext context,
- Class<?> expectedType, boolean allowProxy,
+ Class<?> expectedType,
ProvisionListenerStackCallback<T> provisionCallback)
throws ErrorsException {
final ConstructionContext<T> constructionContext = context.getConstructionContext(this);
// We have a circular reference between constructors. Return a proxy.
if (constructionContext.isConstructing()) {
- if(!allowProxy) {
- throw errors.circularProxiesDisabled(expectedType).toException();
- } else {
- // TODO (crazybob): if we can't proxy this object, can we proxy the other object?
- return constructionContext.createProxy(errors, expectedType);
- }
+ // TODO (crazybob): if we can't proxy this object, can we proxy the other object?
+ return constructionContext.createProxy(
+ errors, context.getInjectorOptions(), expectedType);
}
// If we're re-entering this factory while injecting fields or methods,
@@ -85,7 +82,7 @@
try {
// Optimization: Don't go through the callback stack if we have no listeners.
if (!provisionCallback.hasListeners()) {
- return provision(errors, context, constructionContext);
+ return provision(errors, context, constructionContext);
} else {
return provisionCallback.provision(errors, context, new ProvisionCallback<T>() {
public T call() throws ErrorsException {
diff --git a/core/src/com/google/inject/internal/CycleDetectingLock.java b/core/src/com/google/inject/internal/CycleDetectingLock.java
new file mode 100644
index 0000000..9c46c44
--- /dev/null
+++ b/core/src/com/google/inject/internal/CycleDetectingLock.java
@@ -0,0 +1,292 @@
+package com.google.inject.internal;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Multimap;
+import com.google.common.collect.MultimapBuilder;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+/**
+ * Simplified version of {@link Lock} that is special due to how it handles deadlocks detection.
+ *
+ * <p>Is an inherent part of {@link SingletonScope}, moved into a upper level class due
+ * to its size and complexity.
+ *
+ * @param <ID> Lock identification provided by the client, is returned unmodified to the client
+ * when lock cycle is detected to identify it. Only toString() needs to be implemented.
+ * Lock references this object internally,
+ * for the purposes of Garbage Collection you should not use heavy IDs.
+ * Lock is referenced by a lock factory as long as it's owned by a thread.
+ *
+ * @see SingletonScope
+ * @see com.google.inject.internal.CycleDetectingLock.CycleDetectingLockFactory
+ *
+ * @author timofeyb (Timothy Basanov)
+ */
+interface CycleDetectingLock<ID> {
+
+ /**
+ * Takes a lock in a blocking fashion in case no potential deadlocks are detected.
+ * If the lock was successfully owned, returns an empty map indicating no detected potential
+ * deadlocks.
+ *
+ * Otherwise, a map indicating threads involved in a potential deadlock are returned.
+ * Map is ordered by dependency cycle and lists locks for each thread that are part of
+ * the loop in order. Returned map is created atomically.
+ *
+ * In case no cycle is detected performance is O(threads creating singletons),
+ * in case cycle is detected performance is O(singleton locks).
+ */
+ ListMultimap<Long, ID> lockOrDetectPotentialLocksCycle();
+
+ /**
+ * Unlocks previously locked lock.
+ */
+ void unlock();
+
+ /**
+ * Wraps locks so they would never cause a deadlock. On each
+ * {@link CycleDetectingLock#lockOrDetectPotentialLocksCycle} we check for dependency cycles
+ * within locks created by the same factory. Either we detect a cycle and return it
+ * or take it atomically.
+ *
+ * <p>Important to note that we do not prevent deadlocks in the client code. As an example:
+ * Thread A takes lock L and creates singleton class CA depending on the singleton class CB.
+ * Meanwhile thread B is creating class CB and is waiting on the lock L. Issue happens
+ * due to client code creating interdependent classes and using locks, where
+ * no guarantees on the creation order from Guice are provided.
+ *
+ * <p>Instances of these locks are not intended to be exposed outside of {@link SingletonScope}.
+ */
+ class CycleDetectingLockFactory<ID> {
+
+ /**
+ * Specifies lock that thread is currently waiting on to own it.
+ * Used only for purposes of locks cycle detection.
+ *
+ * Key: thread id
+ * Value: lock that is being waited on
+ *
+ * Element is added inside {@link #lockOrDetectPotentialLocksCycle()} before {@link Lock#lock}
+ * is called. Element is removed inside {@link #lockOrDetectPotentialLocksCycle()} after
+ * {@link Lock#lock} and synchronously with adding it to {@link #locksOwnedByThread}.
+ *
+ * Same lock can be added for several threads in case all of them are trying to
+ * take it.
+ *
+ * Guarded by {@code this}.
+ */
+ private Map<Long, ReentrantCycleDetectingLock> lockThreadIsWaitingOn = Maps.newHashMap();
+
+ /**
+ * Lists locks that thread owns.
+ * Used only to populate locks in a potential cycle when it is detected.
+ *
+ * Key: thread id
+ * Value: stack of locks that were owned.
+ *
+ * Element is added inside {@link #lockOrDetectPotentialLocksCycle()} after {@link Lock#lock}
+ * is called. Element is removed inside {@link #unlock()} synchronously with
+ * {@link Lock#unlock()} call.
+ *
+ * Same lock can only be present several times for the same thread as locks are
+ * reentrant. Lock can not be owned by several different threads as the same time.
+ *
+ * Guarded by {@code this}.
+ */
+ private final Multimap<Long, ReentrantCycleDetectingLock> locksOwnedByThread =
+ MultimapBuilder.linkedHashKeys().linkedHashSetValues().build();
+
+ /**
+ * Creates new lock within this factory context. We can guarantee that locks created by
+ * the same factory would not deadlock.
+ *
+ * @param newLockId lock id that would be used to report lock cycles if detected
+ */
+ CycleDetectingLock<ID> create(ID newLockId) {
+ return new ReentrantCycleDetectingLock(newLockId, new ReentrantLock());
+ }
+
+ /** The implementation for {@link CycleDetectingLock}. */
+ class ReentrantCycleDetectingLock implements CycleDetectingLock<ID> {
+
+ /** Underlying lock used for actual waiting when no potential deadlocks are detected. */
+ private final Lock lockImplementation;
+ /** User id for this lock. */
+ private final ID userLockId;
+ /**
+ * Thread id for the thread that owned this lock. Nullable.
+ * Guarded by {@code CycleDetectingLockFactory.this}.
+ */
+ private Long lockOwnerThreadId = null;
+ /**
+ * Number of times that thread owned this lock.
+ * Guarded by {@code CycleDetectingLockFactory.this}.
+ */
+ private int lockReentranceCount = 0;
+
+ ReentrantCycleDetectingLock(ID userLockId, Lock lockImplementation) {
+ this.userLockId = Preconditions.checkNotNull(userLockId, "userLockId");
+ this.lockImplementation = Preconditions.checkNotNull(
+ lockImplementation, "lockImplementation");
+ }
+
+ @Override public ListMultimap<Long, ID> lockOrDetectPotentialLocksCycle() {
+ final long currentThreadId = Thread.currentThread().getId();
+ synchronized (CycleDetectingLockFactory.this) {
+ checkState();
+ ListMultimap<Long, ID> locksInCycle = detectPotentialLocksCycle();
+ if (!locksInCycle.isEmpty()) {
+ // potential deadlock is found, we don't try to take this lock
+ return locksInCycle;
+ }
+
+ lockThreadIsWaitingOn.put(currentThreadId, this);
+ }
+
+ // this may be blocking, but we don't expect it to cause a deadlock
+ lockImplementation.lock();
+
+ synchronized (CycleDetectingLockFactory.this) {
+ // current thread is no longer waiting on this lock
+ lockThreadIsWaitingOn.remove(currentThreadId);
+ checkState();
+
+ // mark it as owned by us
+ lockOwnerThreadId = currentThreadId;
+ lockReentranceCount++;
+ // add this lock to the list of locks owned by a current thread
+ locksOwnedByThread.put(currentThreadId, this);
+ }
+ // no deadlock is found, locking successful
+ return ImmutableListMultimap.of();
+ }
+
+ @Override public void unlock() {
+ final long currentThreadId = Thread.currentThread().getId();
+ synchronized (CycleDetectingLockFactory.this) {
+ checkState();
+ Preconditions.checkState(lockOwnerThreadId != null,
+ "Thread is trying to unlock a lock that is not locked");
+ Preconditions.checkState(lockOwnerThreadId == currentThreadId,
+ "Thread is trying to unlock a lock owned by another thread");
+
+ // releasing underlying lock
+ lockImplementation.unlock();
+
+ // be sure to release the lock synchronously with updating internal state
+ lockReentranceCount--;
+ if (lockReentranceCount == 0) {
+ // we no longer own this lock
+ lockOwnerThreadId = null;
+ Preconditions.checkState(locksOwnedByThread.remove(currentThreadId, this),
+ "Internal error: Can not find this lock in locks owned by a current thread");
+ if (locksOwnedByThread.get(currentThreadId).isEmpty()) {
+ // clearing memory
+ locksOwnedByThread.removeAll(currentThreadId);
+ }
+ }
+ }
+ }
+
+ /** Check consistency of an internal state. */
+ void checkState() throws IllegalStateException {
+ final long currentThreadId = Thread.currentThread().getId();
+ Preconditions.checkState(!lockThreadIsWaitingOn.containsKey(currentThreadId),
+ "Internal error: Thread should not be in a waiting thread on a lock now");
+ if (lockOwnerThreadId != null) {
+ // check state of a locked lock
+ Preconditions.checkState(lockReentranceCount >= 0,
+ "Internal error: Lock ownership and reentrance count internal states do not match");
+ Preconditions.checkState(locksOwnedByThread.get(lockOwnerThreadId).contains(this),
+ "Internal error: Set of locks owned by a current thread and lock "
+ + "ownership status do not match");
+ } else {
+ // check state of a non locked lock
+ Preconditions.checkState(lockReentranceCount == 0,
+ "Internal error: Reentrance count of a non locked lock is expect to be zero");
+ Preconditions.checkState(!locksOwnedByThread.values().contains(this),
+ "Internal error: Non locked lock should not be owned by any thread");
+ }
+ }
+
+ /**
+ * Algorithm to detect a potential lock cycle.
+ *
+ * For lock's thread owner check which lock is it trying to take.
+ * Repeat recursively. When current thread is found a potential cycle is detected.
+ *
+ * @see CycleDetectingLock#lockOrDetectPotentialLocksCycle()
+ */
+ private ListMultimap<Long, ID> detectPotentialLocksCycle() {
+ final long currentThreadId = Thread.currentThread().getId();
+ if (lockOwnerThreadId == null || lockOwnerThreadId == currentThreadId) {
+ // if nobody owns this lock, lock cycle is impossible
+ // if a current thread owns this lock, we let Guice to handle it
+ return ImmutableListMultimap.of();
+ }
+
+ ListMultimap<Long, ID> potentialLocksCycle =
+ MultimapBuilder.linkedHashKeys().arrayListValues().build();
+ // lock that is a part of a potential locks cycle, starts with current lock
+ ReentrantCycleDetectingLock lockOwnerWaitingOn = this;
+ // try to find a dependency path between lock's owner thread and a current thread
+ while (lockOwnerWaitingOn != null && lockOwnerWaitingOn.lockOwnerThreadId != null) {
+ Long threadOwnerThreadWaits = lockOwnerWaitingOn.lockOwnerThreadId;
+ // in case locks cycle exists lock we're waiting for is part of it
+ potentialLocksCycle.putAll(threadOwnerThreadWaits,
+ getAllLockIdsAfter(threadOwnerThreadWaits, lockOwnerWaitingOn));
+
+ if (threadOwnerThreadWaits == currentThreadId) {
+ // owner thread depends on current thread, cycle detected
+ return potentialLocksCycle;
+ }
+ // going for the next thread we wait on indirectly
+ lockOwnerWaitingOn = lockThreadIsWaitingOn.get(threadOwnerThreadWaits);
+ }
+ // no dependency path from an owner thread to a current thread
+ return ImmutableListMultimap.of();
+ }
+
+ /** Return locks owned by a thread after a lock specified, inclusive. */
+ private List<ID> getAllLockIdsAfter(long threadId, ReentrantCycleDetectingLock lock) {
+ List<ID> ids = Lists.newArrayList();
+ boolean found = false;
+ Collection<ReentrantCycleDetectingLock> ownedLocks = locksOwnedByThread.get(threadId);
+ Preconditions.checkNotNull(ownedLocks,
+ "Internal error: No locks were found taken by a thread");
+ for (ReentrantCycleDetectingLock ownedLock : ownedLocks) {
+ if (ownedLock == lock) {
+ found = true;
+ }
+ if (found) {
+ ids.add(ownedLock.userLockId);
+ }
+ }
+ Preconditions.checkState(found, "Internal error: We can not find locks that "
+ + "created a cycle that we detected");
+ return ids;
+ }
+
+ @Override public String toString() {
+ // copy is made to prevent a data race
+ // no synchronization is used, potentially stale data, should be good enough
+ Long localLockOwnerThreadId = this.lockOwnerThreadId;
+ if (localLockOwnerThreadId != null) {
+ return String.format("CycleDetectingLock[%s][locked by %s]",
+ userLockId, localLockOwnerThreadId);
+ } else {
+ return String.format("CycleDetectingLock[%s][unlocked]", userLockId);
+ }
+ }
+ }
+ }
+}
diff --git a/core/src/com/google/inject/internal/DelegatingInvocationHandler.java b/core/src/com/google/inject/internal/DelegatingInvocationHandler.java
index a7b042b..32441ff 100644
--- a/core/src/com/google/inject/internal/DelegatingInvocationHandler.java
+++ b/core/src/com/google/inject/internal/DelegatingInvocationHandler.java
@@ -16,24 +16,34 @@
package com.google.inject.internal;
+
+import com.google.common.base.Preconditions;
+
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
class DelegatingInvocationHandler<T> implements InvocationHandler {
+ private volatile boolean initialized;
+
private T delegate;
public Object invoke(Object proxy, Method method, Object[] args)
throws Throwable {
- if (delegate == null) {
- throw new IllegalStateException("This is a proxy used to support"
- + " circular references. The object we're"
- + " proxying is not constructed yet. Please wait until after"
- + " injection has completed to use this object.");
- }
-
try {
+ // checking volatile field for synchronization
+ Preconditions.checkState(initialized,
+ "This is a proxy used to support"
+ + " circular references. The object we're"
+ + " proxying is not constructed yet. Please wait until after"
+ + " injection has completed to use this object.");
+ Preconditions.checkNotNull(delegate,
+ "This is a proxy used to support"
+ + " circular references. The object we're "
+ + " proxying is initialized to null."
+ + " No methods can be called.");
+
// TODO: method.setAccessible(true); ?
// this would fix visibility errors when we proxy a
// non-public interface.
@@ -47,11 +57,8 @@
}
}
- public T getDelegate() {
- return delegate;
- }
-
void setDelegate(T delegate) {
this.delegate = delegate;
+ initialized = true;
}
}
diff --git a/core/src/com/google/inject/internal/Errors.java b/core/src/com/google/inject/internal/Errors.java
index 5ce3b42..7527e2a 100644
--- a/core/src/com/google/inject/internal/Errors.java
+++ b/core/src/com/google/inject/internal/Errors.java
@@ -812,6 +812,9 @@
Key<?> key = (Key<?>) source;
formatter.format(" while locating %s%n", convert(key, elementSource));
+ } else if (source instanceof Thread) {
+ formatter.format(" in thread %s%n", source);
+
} else {
formatter.format(" at %s%s%n", source, modules);
}
diff --git a/core/src/com/google/inject/internal/InheritingState.java b/core/src/com/google/inject/internal/InheritingState.java
index db6a7a6..18363f4 100644
--- a/core/src/com/google/inject/internal/InheritingState.java
+++ b/core/src/com/google/inject/internal/InheritingState.java
@@ -60,13 +60,10 @@
private final List<ModuleAnnotatedMethodScannerBinding> scannerBindings = Lists.newArrayList();
private final WeakKeySet blacklistedKeys;
private final Object lock;
- private final Object singletonCreationLock;
InheritingState(State parent) {
this.parent = checkNotNull(parent, "parent");
this.lock = (parent == State.NONE) ? this : parent.lock();
- this.singletonCreationLock =
- (parent == State.NONE) ? new Object() : parent.singletonCreationLock();
this.blacklistedKeys = new WeakKeySet(lock);
}
@@ -190,10 +187,6 @@
return lock;
}
- public Object singletonCreationLock() {
- return singletonCreationLock;
- }
-
public Map<Class<? extends Annotation>, Scope> getScopes() {
ImmutableMap.Builder<Class<? extends Annotation>, Scope> builder = ImmutableMap.builder();
for (Map.Entry<Class<? extends Annotation>, ScopeBinding> entry : scopes.entrySet()) {
diff --git a/core/src/com/google/inject/internal/InjectorImpl.java b/core/src/com/google/inject/internal/InjectorImpl.java
index 3d868eb..54ce8a3 100644
--- a/core/src/com/google/inject/internal/InjectorImpl.java
+++ b/core/src/com/google/inject/internal/InjectorImpl.java
@@ -57,6 +57,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.ConcurrentMap;
/**
* Default {@link Injector} implementation.
@@ -709,8 +710,7 @@
@SuppressWarnings("unchecked")
Key<? extends Provider<T>> providerKey = (Key<? extends Provider<T>>) Key.get(providerType);
ProvidedByInternalFactory<T> internalFactory =
- new ProvidedByInternalFactory<T>(rawType, providerType,
- providerKey, !options.disableCircularProxies);
+ new ProvidedByInternalFactory<T>(rawType, providerType, providerKey);
Object source = rawType;
BindingImpl<T> binding = LinkedProviderBindingImpl.createWithInitializer(
this,
@@ -1051,7 +1051,31 @@
return getProvider(type).get();
}
+ /** @see #getGlobalInternalContext */
private final ThreadLocal<Object[]> localContext;
+ /**
+ * Synchronization: map value is modified only for the current thread,
+ * it's ok to read map values of other threads. It can change between your
+ * calls.
+ *
+ * @see #getGlobalInternalContext
+ */
+ private static final ConcurrentMap<Thread, InternalContext> globalInternalContext =
+ Maps.newConcurrentMap();
+
+ /**
+ * Provides access to the internal context for the current injector of all threads.
+ * One does not need to use this from Guice source code as context could be passed on the stack.
+ * It is required for custom scopes which are called from Guice and sometimes do require
+ * access to current internal context, but it is not passed in. Contrary to {@link #localContext}
+ * it is not used to store injector-specific state, but to provide easy access to the current
+ * state.
+ *
+ * @return unmodifiable map
+ */
+ static Map<Thread, InternalContext> getGlobalInternalContext() {
+ return Collections.unmodifiableMap(globalInternalContext);
+ }
/** Looks up thread local context. Creates (and removes) a new context if necessary. */
<T> T callInContext(ContextualCallable<T> callable) throws ErrorsException {
@@ -1060,17 +1084,30 @@
reference = new Object[1];
localContext.set(reference);
}
+ Thread currentThread = Thread.currentThread();
if (reference[0] == null) {
- reference[0] = new InternalContext();
+ reference[0] = new InternalContext(options);
+ globalInternalContext.put(currentThread, (InternalContext) reference[0]);
try {
- return callable.call((InternalContext)reference[0]);
+ return callable.call((InternalContext) reference[0]);
} finally {
- // Only clear the context if this call created it.
+ // Only clear contexts if this call created them.
reference[0] = null;
+ globalInternalContext.remove(currentThread);
}
} else {
- // Someone else will clean up this context.
- return callable.call((InternalContext)reference[0]);
+ Object previousGlobalInternalContext = globalInternalContext.get(currentThread);
+ globalInternalContext.put(currentThread, (InternalContext) reference[0]);
+ try {
+ // Someone else will clean up this local context.
+ return callable.call((InternalContext) reference[0]);
+ } finally {
+ if (previousGlobalInternalContext != null) {
+ globalInternalContext.put(currentThread, (InternalContext) previousGlobalInternalContext);
+ } else {
+ globalInternalContext.remove(currentThread);
+ }
+ }
}
}
diff --git a/core/src/com/google/inject/internal/InternalContext.java b/core/src/com/google/inject/internal/InternalContext.java
index 1493c37..0af1969 100644
--- a/core/src/com/google/inject/internal/InternalContext.java
+++ b/core/src/com/google/inject/internal/InternalContext.java
@@ -20,6 +20,7 @@
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.inject.Key;
+import com.google.inject.internal.InjectorImpl.InjectorOptions;
import com.google.inject.spi.Dependency;
import com.google.inject.spi.DependencyAndSource;
@@ -35,6 +36,8 @@
*/
final class InternalContext {
+ private final InjectorOptions options;
+
private Map<Object, ConstructionContext<?>> constructionContexts = Maps.newHashMap();
/** Keeps track of the type that is currently being requested for injection. */
@@ -43,6 +46,14 @@
/** Keeps track of the hierarchy of types needed during injection. */
private final DependencyStack state = new DependencyStack();
+ InternalContext(InjectorOptions options) {
+ this.options = options;
+ }
+
+ public InjectorOptions getInjectorOptions() {
+ return options;
+ }
+
@SuppressWarnings("unchecked")
public <T> ConstructionContext<T> getConstructionContext(Object key) {
ConstructionContext<T> constructionContext
@@ -65,13 +76,13 @@
state.add(dependency, source);
return previous;
}
-
+
/** Pops the current state & sets the new dependency. */
public void popStateAndSetDependency(Dependency<?> newDependency) {
state.pop();
this.dependency = newDependency;
}
-
+
/** Adds to the state without setting the dependency. */
public void pushState(Key<?> key, Object source) {
state.add(key, source);
@@ -81,7 +92,7 @@
public void popState() {
state.pop();
}
-
+
/** Returns the current dependency chain (all the state). */
public List<DependencyAndSource> getDependencyChain() {
ImmutableList.Builder<DependencyAndSource> builder = ImmutableList.builder();
diff --git a/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java b/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java
index b534f89..c02c70e 100644
--- a/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java
+++ b/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java
@@ -34,16 +34,15 @@
public InternalFactoryToInitializableAdapter(
Initializable<? extends javax.inject.Provider<? extends T>> initializable,
- Object source, boolean allowProxy,
- ProvisionListenerStackCallback<T> provisionCallback) {
- super(source, allowProxy);
+ Object source, ProvisionListenerStackCallback<T> provisionCallback) {
+ super(source);
this.provisionCallback = checkNotNull(provisionCallback, "provisionCallback");
this.initializable = checkNotNull(initializable, "provider");
}
public T get(Errors errors, InternalContext context, Dependency<?> dependency, boolean linked)
throws ErrorsException {
- return circularGet(initializable.get(errors), errors, context, dependency, linked,
+ return circularGet(initializable.get(errors), errors, context, dependency,
provisionCallback);
}
diff --git a/core/src/com/google/inject/internal/ProvidedByInternalFactory.java b/core/src/com/google/inject/internal/ProvidedByInternalFactory.java
index 0a0191d..1591c50 100644
--- a/core/src/com/google/inject/internal/ProvidedByInternalFactory.java
+++ b/core/src/com/google/inject/internal/ProvidedByInternalFactory.java
@@ -42,9 +42,8 @@
ProvidedByInternalFactory(
Class<?> rawType,
Class<? extends Provider<?>> providerType,
- Key<? extends Provider<T>> providerKey,
- boolean allowProxy) {
- super(providerKey, allowProxy);
+ Key<? extends Provider<T>> providerKey) {
+ super(providerKey);
this.rawType = rawType;
this.providerType = providerType;
this.providerKey = providerKey;
@@ -68,7 +67,7 @@
errors = errors.withSource(providerKey);
Provider<? extends T> provider = providerBinding.getInternalFactory().get(
errors, context, dependency, true);
- return circularGet(provider, errors, context, dependency, linked, provisionCallback);
+ return circularGet(provider, errors, context, dependency, provisionCallback);
} finally {
context.popState();
}
diff --git a/core/src/com/google/inject/internal/ProviderInternalFactory.java b/core/src/com/google/inject/internal/ProviderInternalFactory.java
index e0e7d1d..8574535 100644
--- a/core/src/com/google/inject/internal/ProviderInternalFactory.java
+++ b/core/src/com/google/inject/internal/ProviderInternalFactory.java
@@ -32,31 +32,26 @@
*/
abstract class ProviderInternalFactory<T> implements InternalFactory<T> {
- private final boolean allowProxy;
protected final Object source;
- ProviderInternalFactory(Object source, boolean allowProxy) {
+ ProviderInternalFactory(Object source) {
this.source = checkNotNull(source, "source");
- this.allowProxy = allowProxy;
}
protected T circularGet(final Provider<? extends T> provider, final Errors errors,
- InternalContext context, final Dependency<?> dependency, boolean linked,
+ InternalContext context, final Dependency<?> dependency,
ProvisionListenerStackCallback<T> provisionCallback)
throws ErrorsException {
- Class<?> expectedType = dependency.getKey().getTypeLiteral().getRawType();
final ConstructionContext<T> constructionContext = context.getConstructionContext(this);
// We have a circular reference between constructors. Return a proxy.
if (constructionContext.isConstructing()) {
- if (!allowProxy) {
- throw errors.circularProxiesDisabled(expectedType).toException();
- } else {
+ Class<?> expectedType = dependency.getKey().getTypeLiteral().getRawType();
// TODO: if we can't proxy this object, can we proxy the other object?
@SuppressWarnings("unchecked")
- T proxyType = (T) constructionContext.createProxy(errors, expectedType);
+ T proxyType = (T) constructionContext.createProxy(
+ errors, context.getInjectorOptions(), expectedType);
return proxyType;
- }
}
// Optimization: Don't go through the callback stack if no one's listening.
diff --git a/core/src/com/google/inject/internal/Scoping.java b/core/src/com/google/inject/internal/Scoping.java
index 10afcba..334aaef 100644
--- a/core/src/com/google/inject/internal/Scoping.java
+++ b/core/src/com/google/inject/internal/Scoping.java
@@ -239,14 +239,9 @@
Scope scope = scoping.getScopeInstance();
- try {
- SingletonScope.singletonCreationPerRootInjectorLock.set(injector.state.singletonCreationLock());
- Provider<T> scoped
- = scope.scope(key, new ProviderToInternalFactoryAdapter<T>(injector, creator));
- return new InternalFactoryToProviderAdapter<T>(scoped, source);
- } finally {
- SingletonScope.singletonCreationPerRootInjectorLock.set(null);
- }
+ Provider<T> scoped
+ = scope.scope(key, new ProviderToInternalFactoryAdapter<T>(injector, creator));
+ return new InternalFactoryToProviderAdapter<T>(scoped, source);
}
/**
diff --git a/core/src/com/google/inject/internal/SingletonScope.java b/core/src/com/google/inject/internal/SingletonScope.java
index e98a030..fe6287a 100644
--- a/core/src/com/google/inject/internal/SingletonScope.java
+++ b/core/src/com/google/inject/internal/SingletonScope.java
@@ -1,16 +1,73 @@
package com.google.inject.internal;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
import com.google.inject.Injector;
import com.google.inject.Key;
-import com.google.inject.OutOfScopeException;
import com.google.inject.Provider;
import com.google.inject.ProvisionException;
import com.google.inject.Scope;
import com.google.inject.Scopes;
import com.google.inject.Singleton;
+import com.google.inject.internal.CycleDetectingLock.CycleDetectingLockFactory;
+import com.google.inject.spi.Dependency;
+import com.google.inject.spi.DependencyAndSource;
+import com.google.inject.spi.Message;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
/**
* One instance per {@link Injector}. Also see {@code @}{@link Singleton}.
+ *
+ * Introduction from the author:
+ * Implementation of this class seems unreasonably complicated at the first sight.
+ * I fully agree with you, that the beast below is very complex
+ * and it's hard to reason on how does it work or not.
+ * Still I want to assure you that hundreds(?) of hours were thrown
+ * into making this code simple, while still maintaining Singleton contract.
+ *
+ * Anyway, why is it so complex? Singleton scope does not seem to be that unique.
+ * 1) Guice has never truly expected to be used in multi threading environment
+ * with many Injectors working alongside each other. There is almost no
+ * code with Guice that propagates state between threads. And Singleton
+ * scope is The exception.
+ * 2) Guice supports circular dependencies and thus manages proxy objects.
+ * There is no interface that allows user defined Scopes to create proxies,
+ * it is expected to be done by Guice. Singleton scope needs to be
+ * able to detect circular dependencies spanning several threads,
+ * therefore Singleton scope needs to be able to create these proxies.
+ * 3) To make things worse, Guice has a very tricky definition for a binding
+ * resolution when Injectors are in in a parent/child relationship.
+ * And Scope does not have access to this information by design,
+ * the only real action that Scope can do is to call or not to call a creator.
+ * 4) There is no readily available code in Guice that can detect a potential
+ * deadlock, and no code for handling dependency cycles spanning several threads.
+ * This is significantly harder as all the dependencies in a thread at runtime
+ * can be represented with a list, where in a multi threaded environment
+ * we have more complex dependency trees.
+ * 5) Guice has a pretty strong contract regarding Garbage Collection,
+ * which often prevents us from linking objects directly.
+ * So simple domain specific code can not be written and intermediary
+ * id objects need to be managed.
+ * 6) Guice is relatively fast and we should not make things worse.
+ * We're trying our best to optimize synchronization for speed and memory.
+ * Happy path should be almost as fast as in a single threaded solution
+ * and should not take much more memory.
+ * 7) Error message generation in Guice was not meant to be used like this and to work around
+ * its APIs we need a lot of code. Additional complexity comes from inherent data races
+ * as message is only generated when failure occurs on proxy object generation.
+ * Things get ugly pretty fast.
+ *
+ * @see #scope(Key, Provider)
+ * @see CycleDetectingLock
+ *
+ * @author timofeyb (Timothy Basanov)
*/
public class SingletonScope implements Scope {
@@ -18,70 +75,259 @@
private static final Object NULL = new Object();
/**
- * Lock to use for new instances creation. This allows a per-root-Injector singleton lock,
- * instead of a global lock across the JVM. Is set only during call to {@link #scope}.
+ * Allows us to detect when circular proxies are necessary. It's only used during singleton
+ * instance initialization, after initialization direct access through volatile field is used.
*
- * This is necessary because users have coded to a single {@link Scopes#SINGLETON} instance,
- * and we cannot change that. Additionally, we can't reference the injector from a Key or
- * Provider (the only variables available to the {@link #scope} method). Therefore, we rely
- * on the injector implementation to explicitly set/unset the lock surrounding
- * creation of the Provider the scope creates.
+ * NB: Factory uses {@link Key}s as a user locks ids, different injectors can
+ * share them. Cycles are detected properly as cycle detection does not rely on user locks ids,
+ * but error message generated could be less than ideal.
*
- * @see {@link Scoping#scope(Key, InjectorImpl, InternalFactory, Object, Scoping)} for details.
+ * TODO(user): we may use one factory per injector tree for optimization reasons
*/
- static final ThreadLocal<Object> singletonCreationPerRootInjectorLock =
- new ThreadLocal<Object>();
+ private static final CycleDetectingLockFactory<Key<?>> cycleDetectingLockFactory =
+ new CycleDetectingLockFactory<Key<?>>();
+ /**
+ * Provides singleton scope with the following properties:
+ * - creates no more than one instance per Key as a creator is used no more than once,
+ * - result is cached and returned quickly on subsequent calls,
+ * - exception in a creator is not treated as instance creation and is not cached,
+ * - creates singletons in parallel whenever possible,
+ * - waits for dependent singletons to be created even across threads and when dependencies
+ * are shared as long as no circular dependencies are detected,
+ * - returns circular proxy only when circular dependencies are detected,
+ * - aside from that, blocking synchronization is only used for proxy creation and initialization,
+ * @see CycleDetectingLockFactory
+ */
public <T> Provider<T> scope(final Key<T> key, final Provider<T> creator) {
- // lock is referenced from anonymous class instance
- final Object rootInjectorLock = singletonCreationPerRootInjectorLock.get();
- if (rootInjectorLock == null) {
- throw new OutOfScopeException("Singleton scope should only be used from Injector");
- }
+ /**
+ * Locking strategy:
+ * - volatile instance: double-checked locking for quick exit when scope is initialized,
+ * - constructionContext: manipulations with proxies list or instance initialization
+ * - creationLock: singleton instance creation,
+ * -- allows to guarantee only one instance per singleton,
+ * -- special type of a lock, that prevents potential deadlocks,
+ * -- guards constructionContext for all operations except proxy creation
+ */
return new Provider<T>() {
- /*
+ /**
* The lazily initialized singleton instance. Once set, this will either have type T or will
- * be equal to NULL.
+ * be equal to NULL. Would never be reset to null.
*/
- private volatile Object instance;
+ volatile Object instance;
- // DCL on a volatile is safe as of Java 5, which we obviously require.
+ /**
+ * Circular proxies are used when potential deadlocks are detected. Guarded by itself.
+ * ConstructionContext is not thread-safe, so each call should be synchronized.
+ */
+ final ConstructionContext<T> constructionContext = new ConstructionContext<T>();
+
+ /** For each binding there is a separate lock that we hold during object creation. */
+ final CycleDetectingLock<Key<?>> creationLock = cycleDetectingLockFactory.create(key);
+
@SuppressWarnings("DoubleCheckedLocking")
public T get() {
- if (instance == null) {
- /*
- * Use a pretty coarse lock. We don't want to run into deadlocks
- * when two threads try to load circularly-dependent objects.
- * Maybe one of these days we will identify independent graphs of
- * objects and offer to load them in parallel.
- *
- * This block is re-entrant for circular dependencies.
- */
- synchronized (rootInjectorLock) {
- if (instance == null) {
- T provided = creator.get();
+ // cache volatile variable for the usual case of already initialized object
+ final Object initialInstance = instance;
+ if (initialInstance == null) {
+ // instance is not initialized yet
- // don't remember proxies; these exist only to serve circular dependencies
- if (Scopes.isCircularProxy(provided)) {
- return provided;
+ // acquire lock for current binding to initialize an instance
+ final ListMultimap<Long, Key<?>> locksCycle =
+ creationLock.lockOrDetectPotentialLocksCycle();
+ if (locksCycle.isEmpty()) {
+ // this thread now owns creation of an instance
+ try {
+ // intentionally reread volatile variable to prevent double initialization
+ if (instance == null) {
+ // creator throwing an exception can cause circular proxies created in
+ // different thread to never be resolved, just a warning
+ T provided = creator.get();
+ Object providedNotNull = provided == null ? NULL : provided;
+
+ // scope called recursively can initialize instance as a side effect
+ if (instance == null) {
+ // instance is still not initialized, se we can proceed
+
+ // don't remember proxies created by Guice on circular dependency
+ // detection within the same thread; they are not real instances to cache
+ if (Scopes.isCircularProxy(provided)) {
+ return provided;
+ }
+
+ synchronized (constructionContext) {
+ // guarantee thread-safety for instance and proxies initialization
+ instance = providedNotNull;
+ constructionContext.setProxyDelegates(provided);
+ }
+ } else {
+ // safety assert in case instance was initialized
+ Preconditions.checkState(instance == providedNotNull,
+ "Singleton is called recursively returning different results");
+ }
}
-
- Object providedOrSentinel = (provided == null) ? NULL : provided;
- if (instance != null && instance != providedOrSentinel) {
- throw new ProvisionException(
- "Provider was reentrant while creating a singleton");
+ } catch (RuntimeException e) {
+ // something went wrong, be sure to clean a construction context
+ // this helps to prevent potential memory leaks in circular proxies list
+ synchronized (constructionContext) {
+ constructionContext.finishConstruction();
}
+ throw e;
+ } finally {
+ // always release our creation lock, even on failures
+ creationLock.unlock();
+ }
+ } else {
+ // potential deadlock detected, creation lock is not taken by this thread
+ synchronized (constructionContext) {
+ // guarantee thread-safety for instance and proxies initialization
+ if (instance == null) {
+ // InjectorImpl.callInContext() sets this context when scope is called from Guice
+ Map<Thread, InternalContext> globalInternalContext =
+ InjectorImpl.getGlobalInternalContext();
+ InternalContext internalContext = globalInternalContext.get(Thread.currentThread());
- instance = providedOrSentinel;
+ // creating a proxy to satisfy circular dependency across several threads
+ Dependency<?> dependency = Preconditions.checkNotNull(
+ internalContext.getDependency(),
+ "globalInternalContext.get(currentThread()).getDependency()");
+ Class<?> rawType = dependency.getKey().getTypeLiteral().getRawType();
+
+ try {
+ @SuppressWarnings("unchecked")
+ T proxy = (T) constructionContext.createProxy(
+ new Errors(), internalContext.getInjectorOptions(), rawType);
+ return proxy;
+ } catch (ErrorsException e) {
+ // best effort to create a rich error message
+ List<Message> exceptionErrorMessages = e.getErrors().getMessages();
+ // we expect an error thrown
+ Preconditions.checkState(exceptionErrorMessages.size() == 1);
+ // explicitly copy the map to guarantee iteration correctness
+ // it's ok to have a data race with other threads that are locked
+ Message cycleDependenciesMessage = createCycleDependenciesMessage(
+ ImmutableMap.copyOf(globalInternalContext),
+ locksCycle,
+ exceptionErrorMessages.get(0));
+ // adding stack trace generated by us in addition to a standard one
+ throw new ProvisionException(ImmutableList.of(
+ cycleDependenciesMessage, exceptionErrorMessages.get(0)));
+ }
+ }
}
}
- }
+ // at this point we're sure that singleton was initialized,
+ // reread volatile variable to catch all corner cases
- Object localInstance = instance;
- // This is safe because instance has type T or is equal to NULL
- @SuppressWarnings("unchecked")
- T returnedInstance = (localInstance != NULL) ? (T) localInstance : null;
- return returnedInstance;
+ // caching volatile variable to minimize number of reads performed
+ final Object initializedInstance = instance;
+ Preconditions.checkState(initializedInstance != null,
+ "Internal error: Singleton is not initialized contrary to our expectations");
+ @SuppressWarnings("unchecked")
+ T initializedTypedInstance = (T) initializedInstance;
+ return initializedInstance == NULL ? null : initializedTypedInstance;
+ } else {
+ // singleton is already initialized and local cache can be used
+ @SuppressWarnings("unchecked")
+ T typedInitialIntance = (T) initialInstance;
+ return initialInstance == NULL ? null : typedInitialIntance;
+ }
+ }
+
+ /**
+ * Helper method to create beautiful and rich error descriptions. Best effort and slow.
+ * Tries its best to provide dependency information from injectors currently available
+ * in a global internal context.
+ *
+ * <p>The main thing being done is creating a list of Dependencies involved into
+ * lock cycle across all the threads involved. This is a structure we're creating:
+ * <pre>
+ * { Current Thread, C.class, B.class, Other Thread, B.class, C.class, Current Thread }
+ * To be inserted in the beginning by Guice: { A.class, B.class, C.class }
+ * </pre>
+ * When we're calling Guice to create A and it fails in the deadlock while trying to
+ * create C, which is being created by another thread, which waits for B. List would
+ * be reversed before printing it to the end user.
+ */
+ private Message createCycleDependenciesMessage(
+ Map<Thread, InternalContext> globalInternalContext,
+ ListMultimap<Long, Key<?>> locksCycle,
+ Message proxyCreationError) {
+ // this is the main thing that we'll show in an error message,
+ // current thread is populate by Guice
+ List<Object> sourcesCycle = Lists.newArrayList();
+ sourcesCycle.add(Thread.currentThread());
+ // temp map to speed up look ups
+ Map<Long, Thread> threadById = Maps.newHashMap();
+ for (Thread thread : globalInternalContext.keySet()) {
+ threadById.put(thread.getId(), thread);
+ }
+ for (long lockedThreadId : locksCycle.keySet()) {
+ Thread lockedThread = threadById.get(lockedThreadId);
+ List<Key<?>> lockedKeys = Collections.unmodifiableList(locksCycle.get(lockedThreadId));
+ if (lockedThread == null) {
+ // thread in a lock cycle is already terminated
+ continue;
+ }
+ List<DependencyAndSource> dependencyChain = null;
+ boolean allLockedKeysAreFoundInDependencies = false;
+ // thread in a cycle is still present
+ InternalContext lockedThreadInternalContext = globalInternalContext.get(lockedThread);
+ if (lockedThreadInternalContext != null) {
+ dependencyChain = lockedThreadInternalContext.getDependencyChain();
+
+ // check that all of the keys are still present in dependency chain in order
+ List<Key<?>> lockedKeysToFind = Lists.newLinkedList(lockedKeys);
+ // check stack trace of the thread
+ for (DependencyAndSource d : dependencyChain) {
+ Dependency<?> dependency = d.getDependency();
+ if (dependency == null) {
+ continue;
+ }
+ if (dependency.getKey().equals(lockedKeysToFind.get(0))) {
+ lockedKeysToFind.remove(0);
+ if (lockedKeysToFind.isEmpty()) {
+ // everything is found!
+ allLockedKeysAreFoundInDependencies = true;
+ break;
+ }
+ }
+ }
+ }
+ if (allLockedKeysAreFoundInDependencies) {
+ // all keys are present in a dependency chain of a thread's last injector,
+ // highly likely that we just have discovered a dependency
+ // chain that is part of a lock cycle starting with the first lock owned
+ Key<?> firstLockedKey = lockedKeys.get(0);
+ boolean firstLockedKeyFound = false;
+ for (DependencyAndSource d : dependencyChain) {
+ Dependency<?> dependency = d.getDependency();
+ if (dependency == null) {
+ continue;
+ }
+ if (firstLockedKeyFound) {
+ sourcesCycle.add(dependency);
+ sourcesCycle.add(d.getBindingSource());
+ } else if (dependency.getKey().equals(firstLockedKey)) {
+ firstLockedKeyFound = true;
+ // for the very first one found we don't care why, so no dependency is added
+ sourcesCycle.add(d.getBindingSource());
+ }
+ }
+ } else {
+ // something went wrong and not all keys are present in a state of an injector
+ // that was used last for a current thread.
+ // let's add all keys we're aware of, still better than nothing
+ sourcesCycle.addAll(lockedKeys);
+ }
+ // mentions that a tread is a part of a cycle
+ sourcesCycle.add(lockedThread);
+ }
+ return new Message(
+ sourcesCycle,
+ String.format("Encountered circular dependency spanning several threads. %s",
+ proxyCreationError.getMessage()),
+ null);
}
@Override
diff --git a/core/src/com/google/inject/internal/State.java b/core/src/com/google/inject/internal/State.java
index 8a828f2..32c2da6 100644
--- a/core/src/com/google/inject/internal/State.java
+++ b/core/src/com/google/inject/internal/State.java
@@ -203,12 +203,6 @@
Object lock();
/**
- * Returns the shared lock for all injector's singletons. This is a low-granularity lock
- * to guarantee singleton creation semantics.
- */
- Object singletonCreationLock();
-
- /**
* Returns all the scope bindings at this level and parent levels.
*/
Map<Class<? extends Annotation>, Scope> getScopes();
diff --git a/core/test/com/google/inject/ScopesTest.java b/core/test/com/google/inject/ScopesTest.java
index 59aa596..15c2d2e 100644
--- a/core/test/com/google/inject/ScopesTest.java
+++ b/core/test/com/google/inject/ScopesTest.java
@@ -23,6 +23,7 @@
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.inject.name.Named;
import com.google.inject.spi.Element;
@@ -38,15 +39,18 @@
import java.lang.annotation.Target;
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.Callable;
import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
/**
* @author crazybob@google.com (Bob Lee)
@@ -409,16 +413,6 @@
assertNull(injector.getInstance(String.class));
}
- public void testSingletonScopeCreationOutsideOfScopingThrows() {
- try {
- Scopes.SINGLETON.scope(Key.get(String.class), null);
- fail();
- } catch (OutOfScopeException expected) {
- Asserts.assertContains(expected.getMessage(),
- "Singleton scope should only be used from Injector");
- }
- }
-
class RememberProviderScope implements Scope {
final Map<Key<?>, Provider<?>> providers = Maps.newHashMap();
public <T> Provider<T> scope(Key<T> key, Provider<T> unscoped) {
@@ -720,7 +714,8 @@
bind(c).toProvider(Providers.of("c")).in(Scopes.NO_SCOPE);
bind(d).toProvider(Providers.of("d")).in(Singleton.class);
install(new PrivateModule() {
- @Override protected void configure() {
+ @Override
+ protected void configure() {
bind(f).toProvider(Providers.of("f")).in(Singleton.class);
expose(f);
}
@@ -805,21 +800,47 @@
assertEquals(2, ThrowingSingleton.nextInstanceId);
}
- static interface F {}
+ /**
+ * Should only be created by {@link SBarrierProvider}.
+ *
+ * <p>{@code S} stands for synchronization.
+ *
+ * @see SBarrierProvider
+ */
+ static class S {
- static class FImpl implements F {}
+ private S(int preventInjectionWithoutProvider) {
+ }
+ }
- static class FProvider implements Provider<F> {
+ /**
+ * Provides all the instances of S simultaneously using {@link CyclicBarrier} with {@code
+ * nThreads}. Intended to be used for threads synchronization during injection.
+ */
+ static class SBarrierProvider implements Provider<S> {
- final CyclicBarrier bothThreadsAreInProvider = new CyclicBarrier(2);
+ final CyclicBarrier barrier;
+ volatile boolean barrierPassed = false;
- public F get() {
+ SBarrierProvider(int nThreads) {
+ barrier = new CyclicBarrier(nThreads, new Runnable() {
+ public void run() {
+ // would finish before returning from await() for any thread
+ barrierPassed = true;
+ }
+ });
+ }
+
+ public S get() {
try {
- bothThreadsAreInProvider.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ if (!barrierPassed) {
+ // only if we're triggering barrier for the first time
+ barrier.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ }
} catch (Exception e) {
throw new RuntimeException(e);
}
- return new FImpl();
+ return new S(0);
}
}
@@ -827,107 +848,345 @@
* Tests that different injectors should not affect each other.
*
* <p>This creates a second thread to work in parallel, to create two instances of
- * {@link F} as the same time. If the lock if not granular enough (i.e. JVM-wide)
+ * {@link S} as the same time. If the lock if not granular enough (i.e. JVM-wide)
* then they would block each other creating a deadlock and await timeout.
*/
public void testInjectorsDontDeadlockOnSingletons() throws Exception {
- final FProvider provider = new FProvider();
+ final Provider<S> provider = new SBarrierProvider(2);
final Injector injector = Guice.createInjector(new AbstractModule() {
@Override
protected void configure() {
- bind(F.class).toProvider(provider).in(Scopes.SINGLETON);
+ Thread.currentThread().setName("S.class[1]");
+ bind(S.class).toProvider(provider).in(Scopes.SINGLETON);
}
});
final Injector secondInjector = Guice.createInjector(new AbstractModule() {
@Override
protected void configure() {
- bind(F.class).toProvider(provider).in(Scopes.SINGLETON);
+ Thread.currentThread().setName("S.class[2]");
+ bind(S.class).toProvider(provider).in(Scopes.SINGLETON);
}
});
- Future<?> secondThreadResult = Executors.newSingleThreadExecutor().submit(new Runnable() {
- public void run() {
- secondInjector.getInstance(F.class);
+ Future<S> secondThreadResult = Executors.newSingleThreadExecutor().submit(new Callable<S>() {
+ public S call() {
+ return secondInjector.getInstance(S.class);
}
});
- injector.getInstance(F.class);
+ S firstS = injector.getInstance(S.class);
+ S secondS = secondThreadResult.get();
- // verify no thrown exceptions
- secondThreadResult.get();
+ assertNotSame(firstS, secondS);
}
- static interface G {}
+ @ImplementedBy(GImpl.class)
+ interface G {
- static interface H {}
+ }
- static class GHImpl implements G, H {}
+ @Singleton
+ static class GImpl implements G {
- static class GHProvider {
+ final H h;
- final CyclicBarrier bothThreadsAreInProvider = new CyclicBarrier(2);
+ /**
+ * Relies on Guice implementation to inject S first and H later, which provides a barrier .
+ */
+ @Inject
+ GImpl(S synchronizationBarrier, H h) {
+ this.h = h;
+ }
+ }
- final Provider<G> gProvider = new Provider<G>() {
- public G get() {
- awaitBothThreadsInProvider();
- return new GHImpl();
- }
- };
+ @ImplementedBy(HImpl.class)
+ interface H {
- final Provider<H> hProvider = new Provider<H>() {
- public H get() {
- awaitBothThreadsInProvider();
- return new GHImpl();
- }
- };
+ }
- void awaitBothThreadsInProvider() {
- try {
- bothThreadsAreInProvider.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
- fail();
- } catch (TimeoutException expected) {
- // first thread to arrive should timeout as we never enter providers from both threads
- } catch (BrokenBarrierException expected) {
- // second thread to arrive should fail as barrier is broken by the first thread's timeout
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ @Singleton
+ static class HImpl implements H {
+
+ final G g;
+
+ /**
+ * Relies on Guice implementation to inject S first and G later, which provides a barrier .
+ */
+ @Inject
+ HImpl(S synchronizationBarrier, G g) throws Exception {
+ this.g = g;
}
}
/**
- * Tests that injectors int the same hierarchy do not create singletons in parallel.
+ * Tests that injector can create two singletons with circular dependency in parallel.
*
- * <p>This creates a second thread to work in parallel, to create instance of
- * {@link G} and {@link H} as the same time. Both instances are created by injectors belonging
- * to the same hierarchy. If the lock is too narrow (i.e. per Injector or per binding)
- * instances would be created in parallel and test fail.
+ * <p>This creates two threads to work in parallel, to create instances of
+ * {@link G} and {@link H}. Creation is synchronized by injection of {@link S},
+ * first thread would block until second would be inside a singleton creation as well.
+ *
+ * <p>Both instances are created by sibling injectors, that share singleton scope.
+ * Verifies that exactly one circular proxy object is created.
*/
- public void testSiblingInjectorsGettingDifferentSingletonsDontDeadlock() throws Exception {
- final GHProvider ghProvider = new GHProvider();
- final Injector parentInjector = Guice.createInjector();
-
- Future<?> secondThreadResult = Executors.newSingleThreadExecutor().submit(new Runnable() {
- public void run() {
- parentInjector.createChildInjector(new AbstractModule() {
- @Override
- protected void configure() {
- bind(H.class).toProvider(ghProvider.hProvider).in(Scopes.SINGLETON);
- }
- }).getInstance(H.class);
+ public void testSiblingInjectorGettingCircularSingletonsOneCircularProxy() throws Exception {
+ final Provider<S> provider = new SBarrierProvider(2);
+ final Injector injector = Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(S.class).toProvider(provider);
}
});
- parentInjector.createChildInjector(new AbstractModule() {
+ Future<G> firstThreadResult = Executors.newSingleThreadExecutor().submit(new Callable<G>() {
+ public G call() {
+ Thread.currentThread().setName("G.class");
+ return injector.createChildInjector().getInstance(G.class);
+ }
+ });
+ Future<H> secondThreadResult = Executors.newSingleThreadExecutor().submit(new Callable<H>() {
+ public H call() {
+ Thread.currentThread().setName("H.class");
+ return injector.createChildInjector().getInstance(H.class);
+ }
+ });
+
+ // using separate threads to avoid potential deadlock on the main thread
+ // waiting twice as much to be sure that both would time out in their respective barriers
+ GImpl g = (GImpl) firstThreadResult.get(DEADLOCK_TIMEOUT_SECONDS * 3, TimeUnit.SECONDS);
+ HImpl h = (HImpl) secondThreadResult.get(DEADLOCK_TIMEOUT_SECONDS * 3, TimeUnit.SECONDS);
+
+ // Check that G and H created are not proxied
+ assertTrue(!Scopes.isCircularProxy(g) && !Scopes.isCircularProxy(h));
+
+ // Check that we have no more than one circular proxy created
+ assertFalse(Scopes.isCircularProxy(g.h) && Scopes.isCircularProxy(h.g));
+
+ // Check that non-proxy variable points to another singleton
+ assertTrue(g.h == h || h.g == g);
+
+ // Check correct proxy initialization as default equals implementation would
+ assertEquals(g.h, h);
+ assertEquals(h.g, g);
+ }
+
+ @Singleton
+ static class I0 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ I0(I1 i) {
+ }
+ }
+
+ @Singleton
+ static class I1 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ I1(S synchronizationBarrier, I2 i) {
+ }
+ }
+
+ @Singleton
+ static class I2 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ I2(J1 j) {
+ }
+ }
+
+ @Singleton
+ static class J0 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ J0(J1 j) {
+ }
+ }
+
+ @Singleton
+ static class J1 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ J1(S synchronizationBarrier, J2 j) {
+ }
+ }
+
+ @Singleton
+ static class J2 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ J2(K1 k) {
+ }
+ }
+
+ @Singleton
+ static class K0 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ K0(K1 k) {
+ }
+ }
+
+ @Singleton
+ static class K1 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ K1(S synchronizationBarrier, K2 k) {
+ }
+ }
+
+ @Singleton
+ static class K2 {
+
+ /**
+ * Relies on Guice implementation to inject S first, which provides a barrier .
+ */
+ @Inject
+ K2(I1 i) {
+ }
+ }
+
+ /**
+ * Check that circular dependencies on non-interfaces are correctly resolved in multi-threaded
+ * case. And that an error message constructed is a good one.
+ *
+ * <p>I0 -> I1 -> I2 -> J1 and J0 -> J1 -> J2 -> K1 and K0 -> K1 -> K2,
+ * where I1, J1 and K1 are created in parallel.
+ *
+ * <p>Creation is synchronized by injection of {@link S}, first thread would block until second
+ * would be inside a singleton creation as well.
+ *
+ * <p>Verifies that provision results in an error, that spans two threads and
+ * has a dependency cycle.
+ */
+
+ public void testUnresolvableSingletonCircularDependencyErrorMessage() throws Exception {
+ final Provider<S> provider = new SBarrierProvider(3);
+ final Injector injector = Guice.createInjector(new AbstractModule() {
@Override
protected void configure() {
- bind(G.class).toProvider(ghProvider.gProvider).in(Scopes.SINGLETON);
+ bind(S.class).toProvider(provider);
}
- }).getInstance(G.class);
+ });
- // verify no thrown exceptions
- secondThreadResult.get();
+ Future<I0> firstThreadResult = Executors.newSingleThreadExecutor().submit(new Callable<I0>() {
+ public I0 call() {
+ Thread.currentThread().setName("I0.class");
+ return injector.getInstance(I0.class);
+ }
+ });
+ Future<J0> secondThreadResult = Executors.newSingleThreadExecutor().submit(new Callable<J0>() {
+ public J0 call() {
+ Thread.currentThread().setName("J0.class");
+ return injector.getInstance(J0.class);
+ }
+ });
+ Future<K0> thirdThreadResult = Executors.newSingleThreadExecutor().submit(new Callable<K0>() {
+ public K0 call() {
+ Thread.currentThread().setName("K0.class");
+ return injector.getInstance(K0.class);
+ }
+ });
+
+ // using separate threads to avoid potential deadlock on the main thread
+ // waiting twice as much to be sure that both would time out in their respective barriers
+ Throwable firstException = null;
+ Throwable secondException = null;
+ Throwable thirdException = null;
+ try {
+ firstThreadResult.get(DEADLOCK_TIMEOUT_SECONDS * 3, TimeUnit.SECONDS);
+ fail();
+ } catch (ExecutionException e) {
+ firstException = e.getCause();
+ }
+ try {
+ secondThreadResult.get(DEADLOCK_TIMEOUT_SECONDS * 3, TimeUnit.SECONDS);
+ fail();
+ } catch (ExecutionException e) {
+ secondException = e.getCause();
+ }
+ try {
+ thirdThreadResult.get(DEADLOCK_TIMEOUT_SECONDS * 3, TimeUnit.SECONDS);
+ fail();
+ } catch (ExecutionException e) {
+ thirdException = e.getCause();
+ }
+
+ // verification of error messages generated
+ assertEquals(firstException.getClass(), ProvisionException.class);
+ assertEquals(secondException.getClass(), ProvisionException.class);
+ assertEquals(thirdException.getClass(), ProvisionException.class);
+ List<String> errorMessages = Lists.newArrayList(
+ String.format("%s\n%s\n%s",
+ firstException.getMessage(), secondException.getMessage(), thirdException.getMessage())
+ .split("\\n\\n"));
+ Collections.sort(errorMessages, new Comparator<String>() {
+ @Override
+ public int compare(String s1, String s2) {
+ return s2.length() - s1.length();
+ }
+ });
+ // this is brittle, but turns out that second to longest message spans all threads
+ String errorMessage = errorMessages.get(1);
+ assertContains(errorMessage,
+ "Encountered circular dependency spanning several threads. Tried proxying "
+ + this.getClass().getName());
+ assertFalse("Both I0 and J0 can not be a part of a dependency cycle",
+ errorMessage.contains(I0.class.getName()) && errorMessage.contains(J0.class.getName()));
+ assertFalse("Both J0 and K0 can not be a part of a dependency cycle",
+ errorMessage.contains(J0.class.getName()) && errorMessage.contains(K0.class.getName()));
+ assertFalse("Both K0 and I0 can not be a part of a dependency cycle",
+ errorMessage.contains(K0.class.getName()) && errorMessage.contains(I0.class.getName()));
+
+ List<String> firstErrorLineForThread = new ArrayList<String>();
+ boolean addNextLine = false;
+ for (String errorLine : errorMessage.split("\\n")) {
+ if (errorLine.contains("in thread")) {
+ addNextLine = true;
+ firstErrorLineForThread.add(errorLine);
+ } else if (addNextLine) {
+ addNextLine = false;
+ firstErrorLineForThread.add(errorLine);
+ }
+ }
+ assertEquals("we expect to see [T1, $A, T2, $B, T3, $C, T1, $A]",
+ 8, firstErrorLineForThread.size());
+ assertEquals("first four elements should be different",
+ 6, new HashSet<String>(firstErrorLineForThread.subList(0, 6)).size());
+ assertEquals(firstErrorLineForThread.get(6), firstErrorLineForThread.get(0));
+ assertEquals(firstErrorLineForThread.get(7), firstErrorLineForThread.get(1));
+ assertFalse("K0 thread could not be blocked by J0",
+ firstErrorLineForThread.get(0).contains("J0")
+ && firstErrorLineForThread.get(2).contains("K0"));
+ assertFalse("J0 thread could not be blocked by I0",
+ firstErrorLineForThread.get(0).contains("I0")
+ && firstErrorLineForThread.get(2).contains("J0"));
+ assertFalse("I0 thread could not be blocked by K0",
+ firstErrorLineForThread.get(0).contains("K0")
+ && firstErrorLineForThread.get(2).contains("I0"));
}
}
diff --git a/core/test/com/google/inject/internal/CycleDetectingLockTest.java b/core/test/com/google/inject/internal/CycleDetectingLockTest.java
new file mode 100644
index 0000000..0d10824
--- /dev/null
+++ b/core/test/com/google/inject/internal/CycleDetectingLockTest.java
@@ -0,0 +1,101 @@
+package com.google.inject.internal;
+
+import com.google.inject.internal.CycleDetectingLock.CycleDetectingLockFactory;
+
+import junit.framework.TestCase;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class CycleDetectingLockTest extends TestCase {
+
+ static final long DEADLOCK_TIMEOUT_SECONDS = 1;
+
+ /**
+ * Verifies that graph of threads' dependencies is not static and is calculated in runtime using
+ * information about specific locks.
+ *
+ * <pre>
+ * T1: Waits on S1
+ * T2: Locks B, sends S1, waits on S2
+ * T1: Locks A, start locking B, sends S2, waits on S3
+ * T2: Unlocks B, start locking A, sends S3, finishes locking A, unlocks A
+ * T1: Finishes locking B, unlocks B, unlocks A
+ * </pre>
+ *
+ * <p>This should succeed, even though T1 was locked on T2 and T2 is locked on T1 when T2 locks
+ * A. Incorrect implementation detects a cycle waiting on S3.
+ */
+
+ public void testSingletonThreadsRuntimeCircularDependency() throws Exception {
+ final CyclicBarrier signal1 = new CyclicBarrier(2);
+ final CyclicBarrier signal2 = new CyclicBarrier(2);
+ final CyclicBarrier signal3 = new CyclicBarrier(2);
+ CycleDetectingLockFactory<String> lockFactory = new CycleDetectingLockFactory<String>();
+ final CycleDetectingLock<String> lockA =
+ lockFactory.new ReentrantCycleDetectingLock("A", new ReentrantLock() {
+ @Override
+ public void lock() {
+ if (Thread.currentThread().getName().equals("T2")) {
+ try {
+ signal3.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ } else {
+ assertEquals("T1", Thread.currentThread().getName());
+ }
+ super.lock();
+ }
+ });
+ final CycleDetectingLock<String> lockB =
+ lockFactory.new ReentrantCycleDetectingLock("B", new ReentrantLock() {
+ @Override
+ public void lock() {
+ if (Thread.currentThread().getName().equals("T1")) {
+ try {
+ signal2.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ signal3.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ } else {
+ assertEquals("T2", Thread.currentThread().getName());
+ }
+ super.lock();
+ }
+ });
+ Future<Void> firstThreadResult = Executors.newSingleThreadExecutor().submit(
+ new Callable<Void>() {
+ public Void call() throws Exception {
+ Thread.currentThread().setName("T1");
+ signal1.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ assertTrue(lockA.lockOrDetectPotentialLocksCycle().isEmpty());
+ assertTrue(lockB.lockOrDetectPotentialLocksCycle().isEmpty());
+ lockB.unlock();
+ lockA.unlock();
+ return null;
+ }
+ });
+ Future<Void> secondThreadResult = Executors.newSingleThreadExecutor().submit(
+ new Callable<Void>() {
+ public Void call() throws Exception {
+ Thread.currentThread().setName("T2");
+ assertTrue(lockB.lockOrDetectPotentialLocksCycle().isEmpty());
+ signal1.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ signal2.await(DEADLOCK_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+ lockB.unlock();
+ assertTrue(lockA.lockOrDetectPotentialLocksCycle().isEmpty());
+ lockA.unlock();
+ return null;
+ }
+ });
+
+ firstThreadResult.get();
+ secondThreadResult.get();
+ }
+}
