From 5e6c93348c4250012801b6e41753789d760f06e4 Mon Sep 17 00:00:00 2001 From: timofeyb Date: Mon, 20 Apr 2015 12:45:07 -0700 Subject: [PATCH] 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 --- .../inject/internal/BindingProcessor.java | 4 +- .../inject/internal/BoundProviderFactory.java | 5 +- .../inject/internal/ConstructionContext.java | 17 +- .../internal/ConstructorBindingImpl.java | 4 +- .../inject/internal/ConstructorInjector.java | 13 +- .../inject/internal/CycleDetectingLock.java | 292 ++++++++++++ .../internal/DelegatingInvocationHandler.java | 29 +- .../com/google/inject/internal/Errors.java | 3 + .../inject/internal/InheritingState.java | 7 - .../google/inject/internal/InjectorImpl.java | 51 ++- .../inject/internal/InternalContext.java | 17 +- ...InternalFactoryToInitializableAdapter.java | 7 +- .../internal/ProvidedByInternalFactory.java | 7 +- .../internal/ProviderInternalFactory.java | 15 +- .../com/google/inject/internal/Scoping.java | 11 +- .../inject/internal/SingletonScope.java | 340 ++++++++++++-- .../src/com/google/inject/internal/State.java | 6 - core/test/com/google/inject/ScopesTest.java | 417 ++++++++++++++---- .../internal/CycleDetectingLockTest.java | 101 +++++ 19 files changed, 1139 insertions(+), 207 deletions(-) create mode 100644 core/src/com/google/inject/internal/CycleDetectingLock.java create mode 100644 core/test/com/google/inject/internal/CycleDetectingLockTest.java diff --git a/core/src/com/google/inject/internal/BindingProcessor.java b/core/src/com/google/inject/internal/BindingProcessor.java index 8e83d18b87..b3c869e334 100644 --- a/core/src/com/google/inject/internal/BindingProcessor.java +++ b/core/src/com/google/inject/internal/BindingProcessor.java @@ -111,7 +111,7 @@ public Boolean visit(ProviderInstanceBinding binding) { // always visited with Binding @SuppressWarnings("unchecked") InternalFactory factory = new InternalFactoryToInitializableAdapter( - initializable, source, !injector.options.disableCircularProxies, + initializable, source, injector.provisionListenerStore.get((ProviderInstanceBinding)binding)); InternalFactory scopedFactory = Scoping.scope(key, injector, factory, source, scoping); @@ -127,7 +127,7 @@ public Boolean visit(ProviderKeyBinding binding) { // always visited with Binding @SuppressWarnings("unchecked") BoundProviderFactory boundProviderFactory = new BoundProviderFactory( - injector, providerKey, source, !injector.options.disableCircularProxies, + injector, providerKey, source, injector.provisionListenerStore.get((ProviderKeyBinding) binding)); bindingData.addCreationListener(boundProviderFactory); InternalFactory scopedFactory = Scoping.scope( diff --git a/core/src/com/google/inject/internal/BoundProviderFactory.java b/core/src/com/google/inject/internal/BoundProviderFactory.java index 27ae1aa47d..9f523dfa79 100644 --- a/core/src/com/google/inject/internal/BoundProviderFactory.java +++ b/core/src/com/google/inject/internal/BoundProviderFactory.java @@ -38,9 +38,8 @@ final class BoundProviderFactory extends ProviderInternalFactory implement InjectorImpl injector, Key> providerKey, Object source, - boolean allowProxy, ProvisionListenerStackCallback provisionCallback) { - super(source, allowProxy); + super(source); this.provisionCallback = checkNotNull(provisionCallback, "provisionCallback"); this.injector = injector; this.providerKey = providerKey; @@ -60,7 +59,7 @@ public T get(Errors errors, InternalContext context, Dependency dependency, b try { errors = errors.withSource(providerKey); javax.inject.Provider 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 8a5be0133d..ced388f645 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 @@ public void finishConstruction() { 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 @@ public Object createProxy(Errors errors, Class expectedType) throws ErrorsExc DelegatingInvocationHandler invocationHandler = new DelegatingInvocationHandler(); 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 @@ public void setProxyDelegates(T delegate) { for (DelegatingInvocationHandler 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 1e8bf78f69..8fb2103567 100644 --- a/core/src/com/google/inject/internal/ConstructorBindingImpl.java +++ b/core/src/com/google/inject/internal/ConstructorBindingImpl.java @@ -133,7 +133,6 @@ private static boolean hasAtInject(Constructor cxtor) { @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) injector.constructors.get(constructorInjectionPoint, errors); factory.provisionCallback = @@ -246,7 +245,6 @@ public int hashCode() { private static class Factory implements InternalFactory { private final boolean failIfNotLinked; private final Key key; - private boolean allowCircularProxy; private ConstructorInjector constructorInjector; private ProvisionListenerStackCallback provisionCallback; @@ -267,7 +265,7 @@ public T get(Errors errors, InternalContext context, Dependency dependency, b // 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 ca7bb27754..1ff4be1f3e 100644 --- a/core/src/com/google/inject/internal/ConstructorInjector.java +++ b/core/src/com/google/inject/internal/ConstructorInjector.java @@ -59,19 +59,16 @@ ConstructionProxy getConstructionProxy() { * it may return a proxy. */ Object construct(final Errors errors, final InternalContext context, - Class expectedType, boolean allowProxy, + Class expectedType, ProvisionListenerStackCallback provisionCallback) throws ErrorsException { final ConstructionContext 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 @@ Object construct(final Errors errors, final InternalContext context, 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() { 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 0000000000..9c46c44e1c --- /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. + * + *

Is an inherent part of {@link SingletonScope}, moved into a upper level class due + * to its size and complexity. + * + * @param 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 { + + /** + * 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 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. + * + *

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. + * + *

Instances of these locks are not intended to be exposed outside of {@link SingletonScope}. + */ + class CycleDetectingLockFactory { + + /** + * 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 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 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 create(ID newLockId) { + return new ReentrantCycleDetectingLock(newLockId, new ReentrantLock()); + } + + /** The implementation for {@link CycleDetectingLock}. */ + class ReentrantCycleDetectingLock implements CycleDetectingLock { + + /** 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 lockOrDetectPotentialLocksCycle() { + final long currentThreadId = Thread.currentThread().getId(); + synchronized (CycleDetectingLockFactory.this) { + checkState(); + ListMultimap 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 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 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 getAllLockIdsAfter(long threadId, ReentrantCycleDetectingLock lock) { + List ids = Lists.newArrayList(); + boolean found = false; + Collection 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 a7b042b733..32441ff626 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 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 Object invoke(Object proxy, Method method, Object[] args) } } - 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 5ce3b4290b..7527e2a1d8 100644 --- a/core/src/com/google/inject/internal/Errors.java +++ b/core/src/com/google/inject/internal/Errors.java @@ -812,6 +812,9 @@ public static void formatSource(Formatter formatter, Object source, ElementSourc 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 db6a7a67a1..18363f483b 100644 --- a/core/src/com/google/inject/internal/InheritingState.java +++ b/core/src/com/google/inject/internal/InheritingState.java @@ -60,13 +60,10 @@ final class InheritingState implements State { private final List 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 @@ public Object lock() { return lock; } - public Object singletonCreationLock() { - return singletonCreationLock; - } - public Map, Scope> getScopes() { ImmutableMap.Builder, Scope> builder = ImmutableMap.builder(); for (Map.Entry, 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 3d868ebaac..54ce8a3c85 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 @@ BindingImpl createProvidedByBinding(Key key, Scoping scoping, @SuppressWarnings("unchecked") Key> providerKey = (Key>) Key.get(providerType); ProvidedByInternalFactory internalFactory = - new ProvidedByInternalFactory(rawType, providerType, - providerKey, !options.disableCircularProxies); + new ProvidedByInternalFactory(rawType, providerType, providerKey); Object source = rawType; BindingImpl binding = LinkedProviderBindingImpl.createWithInitializer( this, @@ -1051,7 +1051,31 @@ public T getInstance(Class type) { return getProvider(type).get(); } + /** @see #getGlobalInternalContext */ private final ThreadLocal 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 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 getGlobalInternalContext() { + return Collections.unmodifiableMap(globalInternalContext); + } /** Looks up thread local context. Creates (and removes) a new context if necessary. */ T callInContext(ContextualCallable callable) throws ErrorsException { @@ -1060,17 +1084,30 @@ T callInContext(ContextualCallable callable) throws ErrorsException { 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 1493c37d12..0af1969714 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> constructionContexts = Maps.newHashMap(); /** Keeps track of the type that is currently being requested for injection. */ @@ -43,6 +46,14 @@ final class InternalContext { /** 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 ConstructionContext getConstructionContext(Object key) { ConstructionContext constructionContext @@ -65,13 +76,13 @@ public Dependency pushDependency(Dependency dependency, Object source) { 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 pushState(Key key, Object source) { public void popState() { state.pop(); } - + /** Returns the current dependency chain (all the state). */ public List getDependencyChain() { ImmutableList.Builder builder = ImmutableList.builder(); diff --git a/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java b/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java index b534f8974c..c02c70eaf7 100644 --- a/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java +++ b/core/src/com/google/inject/internal/InternalFactoryToInitializableAdapter.java @@ -34,16 +34,15 @@ final class InternalFactoryToInitializableAdapter extends ProviderInternalFac public InternalFactoryToInitializableAdapter( Initializable> initializable, - Object source, boolean allowProxy, - ProvisionListenerStackCallback provisionCallback) { - super(source, allowProxy); + Object source, ProvisionListenerStackCallback 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 0a0191db7c..1591c5082d 100644 --- a/core/src/com/google/inject/internal/ProvidedByInternalFactory.java +++ b/core/src/com/google/inject/internal/ProvidedByInternalFactory.java @@ -42,9 +42,8 @@ class ProvidedByInternalFactory extends ProviderInternalFactory ProvidedByInternalFactory( Class rawType, Class> providerType, - Key> providerKey, - boolean allowProxy) { - super(providerKey, allowProxy); + Key> providerKey) { + super(providerKey); this.rawType = rawType; this.providerType = providerType; this.providerKey = providerKey; @@ -68,7 +67,7 @@ public T get(Errors errors, InternalContext context, Dependency dependency, bool errors = errors.withSource(providerKey); Provider 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 e0e7d1dfca..8574535dac 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 implements InternalFactory { - 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 provider, final Errors errors, - InternalContext context, final Dependency dependency, boolean linked, + InternalContext context, final Dependency dependency, ProvisionListenerStackCallback provisionCallback) throws ErrorsException { - Class expectedType = dependency.getKey().getTypeLiteral().getRawType(); final ConstructionContext 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 10afcba346..334aaef404 100644 --- a/core/src/com/google/inject/internal/Scoping.java +++ b/core/src/com/google/inject/internal/Scoping.java @@ -239,14 +239,9 @@ static InternalFactory scope(Key key, InjectorImpl injector, Scope scope = scoping.getScopeInstance(); - try { - SingletonScope.singletonCreationPerRootInjectorLock.set(injector.state.singletonCreationLock()); - Provider scoped - = scope.scope(key, new ProviderToInternalFactoryAdapter(injector, creator)); - return new InternalFactoryToProviderAdapter(scoped, source); - } finally { - SingletonScope.singletonCreationPerRootInjectorLock.set(null); - } + Provider scoped + = scope.scope(key, new ProviderToInternalFactoryAdapter(injector, creator)); + return new InternalFactoryToProviderAdapter(scoped, source); } /** diff --git a/core/src/com/google/inject/internal/SingletonScope.java b/core/src/com/google/inject/internal/SingletonScope.java index e98a0303ec..fe6287aa87 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 @@ public class SingletonScope implements Scope { 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 singletonCreationPerRootInjectorLock = - new ThreadLocal(); + private static final CycleDetectingLockFactory> cycleDetectingLockFactory = + new CycleDetectingLockFactory>(); + /** + * 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 Provider scope(final Key key, final Provider 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() { - /* + /** * 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. + */ + volatile Object instance; + + /** + * Circular proxies are used when potential deadlocks are detected. Guarded by itself. + * ConstructionContext is not thread-safe, so each call should be synchronized. */ - private volatile Object instance; + final ConstructionContext constructionContext = new ConstructionContext(); + + /** For each binding there is a separate lock that we hold during object creation. */ + final CycleDetectingLock> creationLock = cycleDetectingLockFactory.create(key); - // DCL on a volatile is safe as of Java 5, which we obviously require. @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(); - - // don't remember proxies; these exist only to serve circular dependencies - if (Scopes.isCircularProxy(provided)) { - return provided; - } + // cache volatile variable for the usual case of already initialized object + final Object initialInstance = instance; + if (initialInstance == null) { + // instance is not initialized yet + + // acquire lock for current binding to initialize an instance + final ListMultimap> 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; - Object providedOrSentinel = (provided == null) ? NULL : provided; - if (instance != null && instance != providedOrSentinel) { - throw new ProvisionException( - "Provider was reentrant while creating a singleton"); + // 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"); + } } + } 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 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 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 + + // 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; } + } - 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; + /** + * 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. + * + *

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: + * 

+       * { 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 }
+       *
+ * 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 globalInternalContext, + ListMultimap> locksCycle, + Message proxyCreationError) { + // this is the main thing that we'll show in an error message, + // current thread is populate by Guice + List sourcesCycle = Lists.newArrayList(); + sourcesCycle.add(Thread.currentThread()); + // temp map to speed up look ups + Map threadById = Maps.newHashMap(); + for (Thread thread : globalInternalContext.keySet()) { + threadById.put(thread.getId(), thread); + } + for (long lockedThreadId : locksCycle.keySet()) { + Thread lockedThread = threadById.get(lockedThreadId); + List> lockedKeys = Collections.unmodifiableList(locksCycle.get(lockedThreadId)); + if (lockedThread == null) { + // thread in a lock cycle is already terminated + continue; + } + List 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> 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 8a828f2ee8..32c2da6173 100644 --- a/core/src/com/google/inject/internal/State.java +++ b/core/src/com/google/inject/internal/State.java @@ -202,12 +202,6 @@ TypeConverterBinding getConverter( */ 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. */ diff --git a/core/test/com/google/inject/ScopesTest.java b/core/test/com/google/inject/ScopesTest.java index 59aa596d3e..15c2d2ecbf 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 @@ protected void configure() {} 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, Provider> providers = Maps.newHashMap(); public Provider scope(Key key, Provider unscoped) { @@ -720,7 +714,8 @@ public void testIsScopedNegative() { 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 @@ public void testSingletonConstructorThrows() { assertEquals(2, ThrowingSingleton.nextInstanceId); } - static interface F {} + /** + * Should only be created by {@link SBarrierProvider}. + * + *

{@code S} stands for synchronization. + * + * @see SBarrierProvider + */ + static class S { - static class FImpl implements F {} + private S(int preventInjectionWithoutProvider) { + } + } - static class FProvider implements Provider { + /** + * 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 { - 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 @@ public F get() { * Tests that different injectors should not affect each other. * *

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 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 secondThreadResult = Executors.newSingleThreadExecutor().submit(new Callable() { + 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 gProvider = new Provider() { - public G get() { - awaitBothThreadsInProvider(); - return new GHImpl(); - } - }; + @ImplementedBy(HImpl.class) + interface H { - final Provider hProvider = new Provider() { - 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. * - *

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. + *

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. + * + *

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(); + public void testSiblingInjectorGettingCircularSingletonsOneCircularProxy() throws Exception { + final Provider provider = new SBarrierProvider(2); + final Injector injector = Guice.createInjector(new AbstractModule() { + @Override + protected void configure() { + bind(S.class).toProvider(provider); + } + }); - 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); + Future firstThreadResult = Executors.newSingleThreadExecutor().submit(new Callable() { + public G call() { + Thread.currentThread().setName("G.class"); + return injector.createChildInjector().getInstance(G.class); + } + }); + Future secondThreadResult = Executors.newSingleThreadExecutor().submit(new Callable() { + public H call() { + Thread.currentThread().setName("H.class"); + return injector.createChildInjector().getInstance(H.class); } }); - parentInjector.createChildInjector(new AbstractModule() { + // 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. + * + *

I0 -> I1 -> I2 -> J1 and J0 -> J1 -> J2 -> K1 and K0 -> K1 -> K2, + * where I1, J1 and K1 are created in parallel. + * + *

Creation is synchronized by injection of {@link S}, first thread would block until second + * would be inside a singleton creation as well. + * + *

Verifies that provision results in an error, that spans two threads and + * has a dependency cycle. + */ + + public void testUnresolvableSingletonCircularDependencyErrorMessage() throws Exception { + final Provider 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); + } + }); + + Future firstThreadResult = Executors.newSingleThreadExecutor().submit(new Callable() { + public I0 call() { + Thread.currentThread().setName("I0.class"); + return injector.getInstance(I0.class); + } + }); + Future secondThreadResult = Executors.newSingleThreadExecutor().submit(new Callable() { + public J0 call() { + Thread.currentThread().setName("J0.class"); + return injector.getInstance(J0.class); + } + }); + Future thirdThreadResult = Executors.newSingleThreadExecutor().submit(new Callable() { + public K0 call() { + Thread.currentThread().setName("K0.class"); + return injector.getInstance(K0.class); } - }).getInstance(G.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(); + } - // verify no thrown exceptions - secondThreadResult.get(); + // verification of error messages generated + assertEquals(firstException.getClass(), ProvisionException.class); + assertEquals(secondException.getClass(), ProvisionException.class); + assertEquals(thirdException.getClass(), ProvisionException.class); + List errorMessages = Lists.newArrayList( + String.format("%s\n%s\n%s", + firstException.getMessage(), secondException.getMessage(), thirdException.getMessage()) + .split("\\n\\n")); + Collections.sort(errorMessages, new Comparator() { + @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 firstErrorLineForThread = new ArrayList(); + 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(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 0000000000..0d10824f91 --- /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. + * + * 

+   *   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
+   *
+ * + *

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 lockFactory = new CycleDetectingLockFactory(); + final CycleDetectingLock 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 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 firstThreadResult = Executors.newSingleThreadExecutor().submit( + new Callable() { + 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 secondThreadResult = Executors.newSingleThreadExecutor().submit( + new Callable() { + 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(); + } +}