Increase lock granularity of singleton scope

[GoogleCodeExporter]

To increase performance and prevent deadlock, we should investigate using
finer-grained locks while creating singleton objects.

Original issue reported on code.google.com by limpbizkit on 7 Jan 2008 at 6:31

[GoogleCodeExporter]

This test program demonstrates a deadlock due to the global lock used by 
singleton scope:

public class DeadlockExample extends TestCase {

  @Singleton
  private static class B { }

  @Singleton
  public static class A {

    @Inject
    public A(final Injector injector) throws InterruptedException {
      Thread thread2 = new Thread("B") {
        public void run() {
          // this blocks thread2, since main holds the lock on Injector.class:
          injector.getInstance(B.class);
        }
      };
      thread2.start();

      // this blocks main, since thread2 is blocked
      thread2.join();
    }
  }

  public void testDeadlock() {
    Guice.createInjector().getInstance(A.class);
  }
}

Original comment by limpbizkit on 7 Jan 2008 at 6:32

• Changed state: New

[GoogleCodeExporter]

The lock is actually so coarse-grained to prevent deadlocks.

Say, for example, that we synchronized only on the provider for the singleton, 
and we
have the following classes:

  class C {
    @Inject(D d) {}
  }

  class D {
    @Inject(C c) {}
  }

Class C would have its own lock, and D would have its own. If one thread calls
Injector.getInstance(C.class) and another calls Injector.getInstance(D.class), 
we
could deadlock. This is a much more likely scenario than the example above.

In the example above, while Guice is calling your code, you spawn a thread, 
block the
current thread and access the Injector from a different thread. We should 
probably
just prohibit some or all of these activities. Deadlock aside, Guice relies on 
the
thread local context being present to manage circular dependencies, etc.

Now, if your application had nothing but static dependencies, we could make the 
lock
per singleton situation work. Before creating a singleton, we would crawl the
singleton's dependency graph and find all transitively dependent singletons. 
Then, we
would sort the singletons by their keys, and grab all the lock in that order. 
This
would ensure that we always grab the locks in the same order and you don't run 
into
situations like the C/D example above.

Unfortunately, this doesn't work if the user calls Injector.getInstance() or 
even
Provider.get() for another singleton--we can't know to grab the lock for that
singleton ahead of time and we're back in the C/D situation.

Original comment by crazybob...@gmail.com on 7 Jan 2008 at 7:05

• Changed state: Invalid

[GoogleCodeExporter]

Cool, this is reasonable. 

We should still change the locks to be synchronized on the injector instance 
and not the injector class. 
Otherwise it would be possible to receive deadlocks due to conflicts between 
unrelated injectors.

Original comment by limpbizkit on 8 Jan 2008 at 8:59