Open GoogleCodeExporter opened 9 years ago
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
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
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
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
> 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
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
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
[deleted comment]
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
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
Original comment by cgruber@google.com
on 16 Jul 2012 at 6:03
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:
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
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
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
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
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
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
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
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
Original issue reported on code.google.com by
zorze...@gmail.com
on 26 Feb 2008 at 1:12