Skip to content

Commit

Permalink
Implement more granular locks for a Singleton scope in Guice.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
timofeyb authored and sameb committed Apr 21, 2015
1 parent 6c85843 commit 5e6c933
Show file tree
Hide file tree
Showing 19 changed files with 1,139 additions and 207 deletions.
4 changes: 2 additions & 2 deletions core/src/com/google/inject/internal/BindingProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public Boolean visit(ProviderInstanceBinding<? extends T> binding) {
// 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);
Expand All @@ -127,7 +127,7 @@ public Boolean visit(ProviderKeyBinding<? extends T> binding) {
// 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(
Expand Down
5 changes: 2 additions & 3 deletions core/src/com/google/inject/internal/BoundProviderFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,8 @@ final class BoundProviderFactory<T> extends ProviderInternalFactory<T> implement
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;
Expand All @@ -60,7 +59,7 @@ public T get(Errors errors, InternalContext context, Dependency<?> dependency, b
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();
}
Expand Down
17 changes: 12 additions & 5 deletions core/src/com/google/inject/internal/ConstructionContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand All @@ -73,6 +75,9 @@ public Object createProxy(Errors errors, Class<?> expectedType) throws ErrorsExc
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));
Expand All @@ -83,6 +88,8 @@ public void setProxyDelegates(T delegate) {
for (DelegatingInvocationHandler<T> handler : invocationHandlers) {
handler.setDelegate(delegate);
}
// initialization of each handler can happen no more than once
invocationHandlers = null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>) injector.constructors.get(constructorInjectionPoint, errors);
factory.provisionCallback =
Expand Down Expand Up @@ -246,7 +245,6 @@ public int hashCode() {
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;

Expand All @@ -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);
}
}
}
13 changes: 5 additions & 8 deletions core/src/com/google/inject/internal/ConstructorInjector.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,16 @@ ConstructionProxy<T> getConstructionProxy() {
* 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,
Expand All @@ -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<T>() {
public T call() throws ErrorsException {
Expand Down
Loading

0 comments on commit 5e6c933

Please sign in to comment.