Scopes.SINGLETON uses a global lock

[GoogleCodeExporter]

See the code below. If you run it, it never returns. The issue (as detailed
in the code) has to do with SINGLETON using Injector.class as a lock (i.e.
not expecting nested Injectors).

====================================================

import com.google.inject.Binder;
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.Scopes;

import junit.framework.TestCase;

public class SingletonIsBorkedTest extends TestCase {

  public void testBorked() throws Exception {
    Module module = new Module() {
      public void configure(Binder binder) {
        binder.bind(Foo.class).in(Scopes.SINGLETON);
      }
    };
    // 1) SINGLETON uses Injector.class as a lock, so the main thread 
    // will hold it until an instance of Foo is returned, but...
    Guice.createInjector(module).getInstance(Foo.class);
  }

  public static final class Foo {

    @Inject
    public Foo() {
      Thread thread = new Thread(new Bar());
      // 2) We start a new thread
      thread.start();
      try {
        // 3) (and we'll wait for it to finish)
        thread.join();
      } catch (InterruptedException e) {
        throw new RuntimeException(e);
      }
    }

    public static final class Baz {}

    private static final class Bar implements Runnable {

      public void run() {
        Module module = new Module() {
          public void configure(Binder binder) {
            binder.bind(Baz.class).in(Scopes.SINGLETON);
          }
        };
        // that also wants to create a SINGLETON. Since Injector.class is 
        // the same for the whole JVM, we have a deadlock.
        Guice.createInjector(module).getInstance(Baz.class);
      }
    }    
  }

}

Original issue reported on code.google.com by zorze...@gmail.com on 26 Feb 2008 at 1:12

[GoogleCodeExporter]

Related discussion:
http://groups.google.com/group/google-guice/browse_thread/thread/c4ccd95b7619251
9

Original comment by medo...@gmail.com on 17 Jul 2008 at 6:26

[GoogleCodeExporter]

For what it's worth, Guice is not happy when you start threads in injected 
members. It's extremely difficult to 
support this type of code correctly...

Original comment by limpbizkit on 2 Nov 2008 at 8:40

[GoogleCodeExporter]

Even if I buy that, and at the risk of being pedantic, Foo's constructor needs 
not to
start any thread -- but simply to communicate (directly or indirectly) with 
one. Say
(for simplicity) that there's a static boolean "go" that blocks Baz's "run" 
method
from progressing, and that Foo's constructor has access to an already started
Thread(Baz) (and still "join"s it, of course). Same dead lock.

As a conceptual mark, it seems very clear (to me) that Guice's SINGLETON
implementation *conceptually* makes the (often correct, and even more often 
harmless,
though naive) assumption that there's a single Injector created per ClassLoader.

Still conceptually, it is more correct that one would be forced to create an 
instance
of a "SingletonScope class" to be the Singleton scope for *their* Injector, so 
that
the injector could lock on "this"... That at the cost of some verboseness.

Even though this (as a mandatory change) is not possible at this point (I don't
think), it would behoove us to have a way for people to instantiate their own
"Singleton"s, if they so choose to...

Original comment by zorze...@gmail.com on 4 Nov 2008 at 7:20

[GoogleCodeExporter]

This also impacts single injectors. I think the best policy is to avoid 
creating threads inside Providers, 
@Provides methods and injected members.

  public void testSingletonScope() {
    Injector injector = Guice.createInjector(new AbstractModule() {
      @Override protected void configure() {}

      @Provides @Singleton Long provideTwo(final Provider<Integer> oneProvider) throws Exception {
        return Executors.newSingleThreadExecutor().submit(new Callable<Integer>() {
          public Integer call() throws Exception {
            return oneProvider.get();
          }
        }).get() + 1L;
      }

      @Singleton @Provides Integer provideOne() throws ExecutionException, InterruptedException {
        return 1;
      }
    });

    injector.getInstance(Long.class);
  }

Original comment by limpbizkit on 31 Dec 2008 at 12:39

• Changed title: Scopes.SINGLETON uses a global lock

[GoogleCodeExporter]

> This also impacts single injectors. I think the best policy is to avoid 
creating threads inside Providers, 
> @Provides methods and injected members.

This is an unreasonable restriction. Though you're exactly right that it 
affects single injectors too. The 
problem with the original poster's code is join() inside a constructor--THAT's 
the only thing we need to void. 
Starting a thread is no(t really a) problem.

Otoh we should think about changing the lock to a private monitor as agreed in 
the discussion thread linked 
above.

Original comment by dha...@gmail.com on 31 Dec 2008 at 12:56

[GoogleCodeExporter]

Hi all, I see this defect hasn't had much activity in over a year but wanted to 
point 
out that I just recently encountered a deadlock due to this.  This global lock 
violates 
a known good practice described in Effective Java for never calling 'alien' 
code from 
within a synchronized block.  In this case, the alien code is the constructor 
of the 
class being injected.

Original comment by craig.squire@gmail.com on 25 Feb 2010 at 5:30

[GoogleCodeExporter]

FWIW we've also seen some real-world deadlocks involving this global lock. 
Patching Guice to use a private monitor for singletons (ie. per-singleton) 
resolved the deadlock and didn't introduce any other problems.

Original comment by mccu...@gmail.com on 17 Nov 2010 at 11:46

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Hey, I have the same problem in a different library that I wrote. I've talked 
about a possible solution here:

https://bitbucket.org/cheez/dicpp/src/853517574ea1/lib/src/scopes/singleton.cpp#
cl-13

I think it could also work for Guice.

Original comment by u.int.3...@gmail.com on 10 May 2011 at 2:59

[GoogleCodeExporter]

Another way out is to use the approach described here:

http://tembrel.blogspot.com/2009/11/concurrently-initialized-singletons-in.html

Original comment by tpeie...@gmail.com on 19 Jul 2011 at 6:44

[GoogleCodeExporter]

Original comment by cgruber@google.com on 16 Jul 2012 at 6:03

• Changed state: Accepted
• Added labels: Component-Core

[GoogleCodeExporter]

FWIW this is the patch that we've been using successfully over at sisu-guice

Original comment by mccu...@gmail.com on 29 Jul 2012 at 6:57

Attachments:

• GUICE_183_20120729.txt

[GoogleCodeExporter]

It's not safe to synchronize on an individual singleton. If two threads access 
the injector and trigger initialization of the same singleton, you could end up 
with two instances of the singleton or worse, a deadlock.

The moral of this story is during injection, don't spawn and wait for a thread 
that accesses the injector. This is equivalent to spawning a thread from a 
static initializer; you shouldn't do that either.

Craig, ideally you'd avoid calling user code from a synchronization block, but 
that's not always possible. For example, ConcurrentHashMap calls equals() and 
hashCode() while holding a lock.

Original comment by crazybob...@gmail.com on 29 Jul 2012 at 7:29

• Changed state: WorkingAsIntended

[GoogleCodeExporter]

Just want to add an empirical observation: I've not yet seen any problems in 
the field (duplicates or deadlocks) caused by using a finer-grained lock - 
however I have seen several real-world deadlocks caused by the current 
coarse-grained lock. And while people can add selective synchronization around 
specific injector requests (or higher in the call chain if they prefer) when 
using the instance lock, the only way to avoid the coarse deadlock is to extend 
global locking further across the entire application. (This issue is much more 
apparent when you have multiple injectors running in the same classloader 
space.)

Is the global (class) lock the only solution? Is there any way to use an 
instance lock at the low-level and solve potential duplicates by selectively 
locking at a higher level in the injector? That would then avoid the current 
problem with the global lock affecting multiple (unrelated) injectors that just 
happen to be in the same classloader.

Original comment by mccu...@gmail.com on 29 Jul 2012 at 8:10

[GoogleCodeExporter]

Empirical observation is no substitute for correctness. If we lock at binding 
granularity, users will see duplicate singletons and deadlocks. There's no room 
for debate here.

Today, you'll only see a deadlock if you spawn a thread during injection, 
access the injector from that thread, and then wait for that thread to 
complete. The solution is simple: don't do that. Comparable or better solutions 
almost always exist.

Can we make the lock finer grained? We could lock on the associated Injector 
instance instead. A Scope implementation doesn't currently have access to the 
Injector. You could access it through a thread local, but this would hurt 
performance for everyone, or you might be able to pass in a special Provider 
subclass as the creator which would have a ref to the injector, but it could be 
tricky or impossible to track down all of the edge cases.

Can we do something more clever? You need a) something like STM, which doesn't 
exist for arbitrary Java code, or b) a new design that's 100% statically 
analyzable so you can figure out which locks you'll need ahead of time (this 
would be a very different API than Guice provides).

Original comment by crazybob...@gmail.com on 30 Jul 2012 at 2:41

[GoogleCodeExporter]

Scoping.scope(...) wraps the injector and key inside a 
ProviderToInternalFactoryAdapter - maybe this could be exposed in a controlled 
fashion so the singleton code can do a quick instanceof check and grab the 
injector for synchronization - if the injector was not available/visible then 
it could fall back to use the class-level lock.

This would stop a misbehaving/rogue component from blocking all other injectors 
that just happen to share the same classloader for the Guice library - as might 
happen in an IDE which allows installation of user-contributed plugins.

Original comment by mccu...@gmail.com on 30 Jul 2012 at 3:16

[GoogleCodeExporter]

You can't mix Injector and class-level locks. If two threads grab them in 
different orders, they'll deadlock.

Original comment by crazybob...@gmail.com on 30 Jul 2012 at 1:22

[GoogleCodeExporter]

OK, I've done some additional analysis of our use of the injector (it's 
encapsulated beneath a compatibility layer) and synchronizing at the binding 
level (as in the attached patch) is sufficient and correct for our particular 
use-case, but this is not strong enough proof for the generic case. As 
mentioned before, we've had problems with the class-level singleton lock in our 
particular framework (we have to support a large number of legacy components 
that we don't have control over) but then we can use our patched version as a 
workaround, without affecting users of stock Guice.

Original comment by mccu...@gmail.com on 2 Aug 2012 at 1:37

[GoogleCodeExporter]

Can we imagine to use a different singleton instanciation strategy regarding 
the stage used:
 1. I development we keep this approach.
 2. In production we Use no lock at all because Injector is not accessible before all singleton are created.

Is it correct for you, does it make sens?

Regards,

Romain

Original comment by romain.g...@gmail.com on 18 Apr 2013 at 4:27

[GoogleCodeExporter]

Hi, guys, just a note for a passer-by who would find this thread.
I fixed it in 
https://github.com/google/guice/commit/d7aa953d088f4955789051414bcd6134437afa17

Original comment by timof...@google.com on 29 Oct 2014 at 9:16