Making Singleton's creation lock less coarse.

Singleton is defined as a scope which creates no more than one
object per injector. It's highly confusing when you catch a deadlock
between two unrelated injectors due to the same class injection.

Problem is demonstrated using a test that recreates scenario when
one thread injecting class can block other thread to use its own
injector.

Proposed solution is to use Injector-wide locks in a singleton.
ThreadLocal as a way to store current state.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=78469951

Author: timofeyb

core/src/com/google/inject/Scopes.java

@@ -17,8 +17,8 @@
 package com.google.inject;
 
 import com.google.inject.internal.CircularDependencyProxy;
-import com.google.inject.internal.InternalInjectorCreator;
 import com.google.inject.internal.LinkedBindingImpl;
+import com.google.inject.internal.SingletonScope;
 import com.google.inject.spi.BindingScopingVisitor;
 import com.google.inject.spi.ExposedBinding;
 
@@ -33,71 +33,10 @@ public class Scopes {
 
   private Scopes() {}
 
-  /** A sentinel value representing null. */
-  private static final Object NULL = new Object();
-
   /**
    * One instance per {@link Injector}. Also see {@code @}{@link Singleton}.
    */
-  public static final Scope SINGLETON = new Scope() {
-    public <T> Provider<T> scope(final Key<T> key, final Provider<T> creator) {
-      return new Provider<T>() {
-        /*
-         * The lazily initialized singleton instance. Once set, this will either have type T or will
-         * be equal to NULL.
-         */
-        private volatile Object instance;
-
-        // 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 (InternalInjectorCreator.class) {
-              if (instance == null) {
-                T provided = creator.get();
-
-                // don't remember proxies; these exist only to serve circular dependencies
-                if (isCircularProxy(provided)) {
-                  return provided;
-                }
-
-                Object providedOrSentinel = (provided == null) ? NULL : provided;
-                if (instance != null && instance != providedOrSentinel) {
-                  throw new ProvisionException(
-                      "Provider was reentrant while creating a singleton");
-                }
-
-                instance = providedOrSentinel;
-              }
-            }
-          }
-
-          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;
-        }
-
-        @Override
-        public String toString() {
-          return String.format("%s[%s]", creator, SINGLETON);
-        }
-      };
-    }
-
-    @Override public String toString() {
-      return "Scopes.SINGLETON";
-    }
-  };
+  public static final Scope SINGLETON = new SingletonScope();
 
   /**
    * No scope; the same as not applying any scope at all.  Each time the

core/src/com/google/inject/internal/InheritingState.java

@@ -58,10 +58,13 @@ final class InheritingState implements State {
   private final List<ProvisionListenerBinding> provisionListenerBindings = 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);
   }
 
@@ -172,6 +175,10 @@ public Object lock() {
     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()) {

core/src/com/google/inject/internal/Scoping.java

@@ -239,9 +239,14 @@ static <T> InternalFactory<? extends T> scope(Key<T> key, InjectorImpl injector,
 
     Scope scope = scoping.getScopeInstance();
 
-    Provider<T> scoped
-        = scope.scope(key, new ProviderToInternalFactoryAdapter<T>(injector, creator));
-    return new InternalFactoryToProviderAdapter<T>(scoped, source);
+    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);
+    }
   }
 
   /**

core/src/com/google/inject/internal/SingletonScope.java

@@ -0,0 +1,97 @@
+package com.google.inject.internal;
+
+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;
+
+/**
+ * One instance per {@link Injector}. Also see {@code @}{@link Singleton}.
+ */
+public class SingletonScope implements Scope {
+
+  /** A sentinel value representing null. */
+  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}.
+   *
+   * 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.
+   *
+   * @see {@link Scoping#scope(Key, InjectorImpl, InternalFactory, Object, Scoping)} for details.
+   */
+  static final ThreadLocal<Object> singletonCreationPerRootInjectorLock =
+      new ThreadLocal<Object>();
+
+  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");
+    }
+    return new Provider<T>() {
+      /*
+       * The lazily initialized singleton instance. Once set, this will either have type T or will
+       * be equal to NULL.
+       */
+      private volatile Object instance;
+
+      // 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;
+              }
+
+              Object providedOrSentinel = (provided == null) ? NULL : provided;
+              if (instance != null && instance != providedOrSentinel) {
+                throw new ProvisionException(
+                    "Provider was reentrant while creating a singleton");
+              }
+
+              instance = providedOrSentinel;
+            }
+          }
+        }
+
+        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;
+      }
+
+      @Override
+      public String toString() {
+        return String.format("%s[%s]", creator, Scopes.SINGLETON);
+      }
+    };
+  }
+
+  @Override public String toString() {
+    return "Scopes.SINGLETON";
+  }
+}

core/src/com/google/inject/internal/State.java

@@ -120,6 +120,10 @@ public Object lock() {
       throw new UnsupportedOperationException();
     }
 
+    public Object singletonCreationLock() {
+      throw new UnsupportedOperationException();
+    }
+
     public Map<Class<? extends Annotation>, Scope> getScopes() {
       return ImmutableMap.of();
     }
@@ -184,6 +188,12 @@ 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.
    */