Re: Scopes.SINGLETON uses a global lock issue 183
I read the bug report and the longer discussion. That singletons are classloader-wide does seem wrong (reminds me of the many woes caused by the AWT tree lock). Seems like the bug is the fact that Scopes.SINGLETON is a static field (not a small amount of irony to that in Guice :-)). I mean, effectively, there's no difference between synchronized(Injector.class) and synchronized(foo) - either is a handy public field that was lying around and able to be a monitor; using Injector.class is just slightly less obviously evil. Otherwise you could have a singleton with respect to this injector and its parents (or just this injector) scope that was private to the injector and expose it via an instance method on, say, AbstractModule (still need a way to expose it to those who implement Module directly, though) - which would function identically to the situation now, except that two unrelated injectors could not interfere with each other. bind(Foo.class).in(singletonScope()); There just needs to be a way to give child injectors a reference to the parent injector's lock. -Tim -- You received this message because you are subscribed to the Google Groups google-guice group. To unsubscribe from this group and stop receiving emails from it, send an email to google-guice+unsubscr...@googlegroups.com. To post to this group, send email to google-guice@googlegroups.com. Visit this group at http://groups.google.com/group/google-guice. For more options, visit https://groups.google.com/groups/opt_out.
Re: Scopes.SINGLETON uses a global lock issue 183
I like it Tim Romain Le jeudi 17 octobre 2013 11:50:43 UTC+2, Tim Boudreau a écrit : I read the bug report and the longer discussion. That singletons are classloader-wide does seem wrong (reminds me of the many woes caused by the AWT tree lock). Seems like the bug is the fact that Scopes.SINGLETON is a static field (not a small amount of irony to that in Guice :-)). I mean, effectively, there's no difference between synchronized(Injector.class) and synchronized(foo) - either is a handy public field that was lying around and able to be a monitor; using Injector.class is just slightly less obviously evil. Otherwise you could have a singleton with respect to this injector and its parents (or just this injector) scope that was private to the injector and expose it via an instance method on, say, AbstractModule (still need a way to expose it to those who implement Module directly, though) - which would function identically to the situation now, except that two unrelated injectors could not interfere with each other. bind(Foo.class).in(singletonScope()); There just needs to be a way to give child injectors a reference to the parent injector's lock. -Tim -- You received this message because you are subscribed to the Google Groups google-guice group. To unsubscribe from this group and stop receiving emails from it, send an email to google-guice+unsubscr...@googlegroups.com. To post to this group, send email to google-guice@googlegroups.com. Visit this group at http://groups.google.com/group/google-guice. For more options, visit https://groups.google.com/groups/opt_out.
Re: Scopes.SINGLETON uses a global lock issue 183
Because in production stage the singleton are instantiated before the
injector is returned where in development stage (default) the singleton
(excepted the eager) are lazily instantiated. So in production mode we have
(if I'm not wrong) a safety initialization.
Romain.
Le mercredi 16 octobre 2013 06:55:38 UTC+2, Sam Berlin a écrit :
What is it about production mode that makes you think it isn't necessary
(whereas it would be necessary with development mode)?
sam
On Oct 16, 2013 12:27 AM, Romain Gilles romain...@gmail.comjavascript:
wrote:
Hi all,
I add a comment
#19https://code.google.com/p/google-guice/issues/detail?id=183#c19 on
this issue since April. I would like to know if it's a stupid comment or
not. In fact we fall in some case in a deadlock situation with the class
level lock used by the Singleton scope. And I think in case of production
stage the lock and event the volatile instance field are not required. For
me only development mode require this kind of protection and we always
create our Injector in production stage.
Does it make sens for you?
Here a code snippet of Singleton Scope (guice 3.0) :
public static final Scope SINGLETON = new Scope() {
public T ProviderT scope(final KeyT key, final ProviderT
creator) {
return new ProviderT() {
/*
* 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 (provided instanceof CircularDependencyProxy) {
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;
}
Regards,
Romain.
--
You received this message because you are subscribed to the Google Groups
google-guice group.
To unsubscribe from this group and stop receiving emails from it, send an
email to google-guice...@googlegroups.com javascript:.
To post to this group, send email to google...@googlegroups.comjavascript:
.
Visit this group at http://groups.google.com/group/google-guice.
For more options, visit https://groups.google.com/groups/opt_out.
--
You received this message because you are subscribed to the Google Groups
google-guice group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-guice+unsubscr...@googlegroups.com.
To post to this group, send email to google-guice@googlegroups.com.
Visit this group at http://groups.google.com/group/google-guice.
For more options, visit https://groups.google.com/groups/opt_out.
Re: Scopes.SINGLETON uses a global lock issue 183
Unfortunately, it's still very possible to discover singletons after
initialization (by someone injecting the injector and calling .get on it
for a just-in-time binding to a singleton). It's also unsafe during
injector creation if any of the singletons do any thread-stuff in their
constructor and delegate to other singleton things (by, say, injecting a
ProviderT and passing the provider off to the thread so that creation is
done from another thread).
sam
On Oct 16, 2013 5:23 AM, Romain Gilles romain.gil...@gmail.com wrote:
Because in production stage the singleton are instantiated before the
injector is returned where in development stage (default) the singleton
(excepted the eager) are lazily instantiated. So in production mode we have
(if I'm not wrong) a safety initialization.
Romain.
Le mercredi 16 octobre 2013 06:55:38 UTC+2, Sam Berlin a écrit :
What is it about production mode that makes you think it isn't necessary
(whereas it would be necessary with development mode)?
sam
On Oct 16, 2013 12:27 AM, Romain Gilles romain...@gmail.com wrote:
Hi all,
I add a comment
#19https://code.google.com/p/google-guice/issues/detail?id=183#c19 on
this issue since April. I would like to know if it's a stupid comment or
not. In fact we fall in some case in a deadlock situation with the class
level lock used by the Singleton scope. And I think in case of production
stage the lock and event the volatile instance field are not required. For
me only development mode require this kind of protection and we always
create our Injector in production stage.
Does it make sens for you?
Here a code snippet of Singleton Scope (guice 3.0) :
public static final Scope SINGLETON = new Scope() {
public T ProviderT scope(final KeyT key, final ProviderT
creator) {
return new ProviderT() {
/*
* 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 (provided instanceof CircularDependencyProxy) {
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;
}
Regards,
Romain.
--
You received this message because you are subscribed to the Google
Groups google-guice group.
To unsubscribe from this group and stop receiving emails from it, send
an email to google-guice...@**googlegroups.com.
To post to this group, send email to google...@googlegroups.com.
Visit this group at
http://groups.google.com/**group/google-guicehttp://groups.google.com/group/google-guice
.
For more options, visit
https://groups.google.com/**groups/opt_outhttps://groups.google.com/groups/opt_out
.
--
You received this message because you are subscribed to the Google Groups
google-guice group.
To unsubscribe from this group and stop receiving emails from it, send an
email to google-guice+unsubscr...@googlegroups.com.
To post to this group, send email to google-guice@googlegroups.com.
Visit this group at http://groups.google.com/group/google-guice.
For more options, visit https://groups.google.com/groups/opt_out.
--
You received this message because you are subscribed to the Google Groups
google-guice group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-guice+unsubscr...@googlegroups.com.
To post to this group, send email to google-guice@googlegroups.com.
Visit this group at
Re: Scopes.SINGLETON uses a global lock issue 183
Ok, now I see why I'm so wrong in my understanding. I our use cases I
didn't notice this kind of use cases. We try to follow the guice best
practices our constructors are side effect free , we delegate the
initialization of the application to a specific step after injector
creation and we don't use JIT bindings.
But you have to support the worst cases and therefore the current solution
is the right one.
Thank you for your help,
Romain.
Le mercredi 16 octobre 2013 14:25:33 UTC+2, Sam Berlin a écrit :
Unfortunately, it's still very possible to discover singletons after
initialization (by someone injecting the injector and calling .get on it
for a just-in-time binding to a singleton). It's also unsafe during
injector creation if any of the singletons do any thread-stuff in their
constructor and delegate to other singleton things (by, say, injecting a
ProviderT and passing the provider off to the thread so that creation is
done from another thread).
sam
On Oct 16, 2013 5:23 AM, Romain Gilles romain...@gmail.comjavascript:
wrote:
Because in production stage the singleton are instantiated before the
injector is returned where in development stage (default) the singleton
(excepted the eager) are lazily instantiated. So in production mode we have
(if I'm not wrong) a safety initialization.
Romain.
Le mercredi 16 octobre 2013 06:55:38 UTC+2, Sam Berlin a écrit :
What is it about production mode that makes you think it isn't necessary
(whereas it would be necessary with development mode)?
sam
On Oct 16, 2013 12:27 AM, Romain Gilles romain...@gmail.com wrote:
Hi all,
I add a comment
#19https://code.google.com/p/google-guice/issues/detail?id=183#c19 on
this issue since April. I would like to know if it's a stupid comment or
not. In fact we fall in some case in a deadlock situation with the class
level lock used by the Singleton scope. And I think in case of production
stage the lock and event the volatile instance field are not required. For
me only development mode require this kind of protection and we always
create our Injector in production stage.
Does it make sens for you?
Here a code snippet of Singleton Scope (guice 3.0) :
public static final Scope SINGLETON = new Scope() {
public T ProviderT scope(final KeyT key, final ProviderT
creator) {
return new ProviderT() {
/*
* 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 (provided instanceof CircularDependencyProxy) {
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;
}
Regards,
Romain.
--
You received this message because you are subscribed to the Google
Groups google-guice group.
To unsubscribe from this group and stop receiving emails from it, send
an email to google-guice...@**googlegroups.com.
To post to this group, send email to google...@googlegroups.com.
Visit this group at
http://groups.google.com/**group/google-guicehttp://groups.google.com/group/google-guice
.
For more options, visit
https://groups.google.com/**groups/opt_outhttps://groups.google.com/groups/opt_out
.
--
You received this message because you are subscribed to the Google Groups
google-guice group.
To unsubscribe from this group and stop receiving emails from it, send an
email to
Re: Scopes.SINGLETON uses a global lock issue 183
What is it about production mode that makes you think it isn't necessary
(whereas it would be necessary with development mode)?
sam
On Oct 16, 2013 12:27 AM, Romain Gilles romain.gil...@gmail.com wrote:
Hi all,
I add a comment
#19https://code.google.com/p/google-guice/issues/detail?id=183#c19 on
this issue since April. I would like to know if it's a stupid comment or
not. In fact we fall in some case in a deadlock situation with the class
level lock used by the Singleton scope. And I think in case of production
stage the lock and event the volatile instance field are not required. For
me only development mode require this kind of protection and we always
create our Injector in production stage.
Does it make sens for you?
Here a code snippet of Singleton Scope (guice 3.0) :
public static final Scope SINGLETON = new Scope() {
public T ProviderT scope(final KeyT key, final ProviderT
creator) {
return new ProviderT() {
/*
* 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 (provided instanceof CircularDependencyProxy) {
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;
}
Regards,
Romain.
--
You received this message because you are subscribed to the Google Groups
google-guice group.
To unsubscribe from this group and stop receiving emails from it, send an
email to google-guice+unsubscr...@googlegroups.com.
To post to this group, send email to google-guice@googlegroups.com.
Visit this group at http://groups.google.com/group/google-guice.
For more options, visit https://groups.google.com/groups/opt_out.
--
You received this message because you are subscribed to the Google Groups
google-guice group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-guice+unsubscr...@googlegroups.com.
To post to this group, send email to google-guice@googlegroups.com.
Visit this group at http://groups.google.com/group/google-guice.
For more options, visit https://groups.google.com/groups/opt_out.
