Re: RFR 8080911: sun/security/krb5/auto/UseCacheAndStoreKey.java timed out intermittently
I would use the Initialization-on-demand holder idiom [1] instead, ex:
private static class SingletonHolder {
private static final Config singleton = new Config();
}
public static Config getInstance() throws KrbException {
return SingletonHolder.singleton;
}
This way you avoid synchronized altogether.
--Sean
[1] http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
On 05/26/2015 12:50 AM, Xuelei Fan wrote:
I do not like class level synchronization because it may not work as
expected, especially if the synchronization can be used by other codes.
However, your update does not change this behavior. The fix looks fine
to me. Please go ahead if you don't want to use object level
synchronization.
Xuelei
On 5/26/2015 10:40 AM, Weijun Wang wrote:
On 5/26/2015 9:22 AM, Xuelei Fan wrote:
On 5/26/2015 9:06 AM, Weijun Wang wrote:
On 5/26/2015 7:59 AM, Xuelei Fan wrote:
synchronized on class looks a little bit unsafe to me.
Why? Isn't it the same as making a static method synchronized? [1]
Other code may be also able to lock on class.
Code 1:
lock on MyClass.class
Code 2:
lock on MyClass.class
Code 1 and 2 know nothing about each other. The behavior may be weird.
I don't think it is a good practice.
So you are not a fan of all synchronized methods? :-)
I thought it's quite common to use synchronized static methods to
prevent simultaneous access to a static field. While a method in another
class can lock on thisClass.class (in this case the class is internal),
I don't see why it wants to do this. This is not casual, it's just evil.
As singleton is
a static variable, creating the instance during initialization looks
safer.
- private static Config singleton = null;
+ private static Config singleton = new Config();
This line might throw an exception. Also, do you intend to make
getInstance() simply returning singleton? This means if the first call
to new Config() throws an exception, getInstance() will not try to
reconstruct it. This might not be common in production, but I don't want
to make any behavior change.
I see you point. Maybe, you can lock on a object.
A private static final Object? Yes I can, but is it worth doing?
Thanks
Max
Xuelei
--Max
[1]
https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.4.3.6
Xuelei
On 5/25/2015 10:16 PM, Weijun Wang wrote:
Hi All
Please review a code change at
http://cr.openjdk.java.net/~weijun/8080911/webrev.00/
I've limit the synchronized block to Config creation only and
therefore
won't deadlock with EType's class initialization.
Noreg-hard. The EType call is at class initialization and only run
once
in a VM session, which is extremely difficult to catch.
Thanks
Max
Re: RFR 8080911: sun/security/krb5/auto/UseCacheAndStoreKey.java timed out intermittently
On 05/26/2015 09:45 AM, Weijun Wang wrote:
But here it's not a real singleton (not final), the refresh() method can
reassign an instance to it.
Oh. I missed that. Forget my suggestion then.
--Sean
--Max
On 5/26/2015 8:51 PM, Sean Mullan wrote:
I would use the Initialization-on-demand holder idiom [1] instead, ex:
private static class SingletonHolder {
private static final Config singleton = new Config();
}
public static Config getInstance() throws KrbException {
return SingletonHolder.singleton;
}
This way you avoid synchronized altogether.
--Sean
[1] http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
On 05/26/2015 12:50 AM, Xuelei Fan wrote:
I do not like class level synchronization because it may not work as
expected, especially if the synchronization can be used by other codes.
However, your update does not change this behavior. The fix looks
fine
to me. Please go ahead if you don't want to use object level
synchronization.
Xuelei
On 5/26/2015 10:40 AM, Weijun Wang wrote:
On 5/26/2015 9:22 AM, Xuelei Fan wrote:
On 5/26/2015 9:06 AM, Weijun Wang wrote:
On 5/26/2015 7:59 AM, Xuelei Fan wrote:
synchronized on class looks a little bit unsafe to me.
Why? Isn't it the same as making a static method synchronized? [1]
Other code may be also able to lock on class.
Code 1:
lock on MyClass.class
Code 2:
lock on MyClass.class
Code 1 and 2 know nothing about each other. The behavior may be
weird.
I don't think it is a good practice.
So you are not a fan of all synchronized methods? :-)
I thought it's quite common to use synchronized static methods to
prevent simultaneous access to a static field. While a method in
another
class can lock on thisClass.class (in this case the class is internal),
I don't see why it wants to do this. This is not casual, it's just
evil.
As singleton is
a static variable, creating the instance during initialization looks
safer.
- private static Config singleton = null;
+ private static Config singleton = new Config();
This line might throw an exception. Also, do you intend to make
getInstance() simply returning singleton? This means if the first
call
to new Config() throws an exception, getInstance() will not try to
reconstruct it. This might not be common in production, but I don't
want
to make any behavior change.
I see you point. Maybe, you can lock on a object.
A private static final Object? Yes I can, but is it worth doing?
Thanks
Max
Xuelei
--Max
[1]
https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.4.3.6
Xuelei
On 5/25/2015 10:16 PM, Weijun Wang wrote:
Hi All
Please review a code change at
http://cr.openjdk.java.net/~weijun/8080911/webrev.00/
I've limit the synchronized block to Config creation only and
therefore
won't deadlock with EType's class initialization.
Noreg-hard. The EType call is at class initialization and only run
once
in a VM session, which is extremely difficult to catch.
Thanks
Max
Re: RFR 8080911: sun/security/krb5/auto/UseCacheAndStoreKey.java timed out intermittently
synchronized on class looks a little bit unsafe to me. As singleton is a static variable, creating the instance during initialization looks safer. - private static Config singleton = null; + private static Config singleton = new Config(); Xuelei On 5/25/2015 10:16 PM, Weijun Wang wrote: Hi All Please review a code change at http://cr.openjdk.java.net/~weijun/8080911/webrev.00/ I've limit the synchronized block to Config creation only and therefore won't deadlock with EType's class initialization. Noreg-hard. The EType call is at class initialization and only run once in a VM session, which is extremely difficult to catch. Thanks Max
Re: RFR 8080911: sun/security/krb5/auto/UseCacheAndStoreKey.java timed out intermittently
On 5/26/2015 7:59 AM, Xuelei Fan wrote: synchronized on class looks a little bit unsafe to me. Why? Isn't it the same as making a static method synchronized? [1] As singleton is a static variable, creating the instance during initialization looks safer. - private static Config singleton = null; + private static Config singleton = new Config(); This line might throw an exception. Also, do you intend to make getInstance() simply returning singleton? This means if the first call to new Config() throws an exception, getInstance() will not try to reconstruct it. This might not be common in production, but I don't want to make any behavior change. --Max [1] https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.4.3.6 Xuelei On 5/25/2015 10:16 PM, Weijun Wang wrote: Hi All Please review a code change at http://cr.openjdk.java.net/~weijun/8080911/webrev.00/ I've limit the synchronized block to Config creation only and therefore won't deadlock with EType's class initialization. Noreg-hard. The EType call is at class initialization and only run once in a VM session, which is extremely difficult to catch. Thanks Max
Re: RFR 8080911: sun/security/krb5/auto/UseCacheAndStoreKey.java timed out intermittently
On 5/26/2015 9:06 AM, Weijun Wang wrote: On 5/26/2015 7:59 AM, Xuelei Fan wrote: synchronized on class looks a little bit unsafe to me. Why? Isn't it the same as making a static method synchronized? [1] Other code may be also able to lock on class. Code 1: lock on MyClass.class Code 2: lock on MyClass.class Code 1 and 2 know nothing about each other. The behavior may be weird. I don't think it is a good practice. As singleton is a static variable, creating the instance during initialization looks safer. - private static Config singleton = null; + private static Config singleton = new Config(); This line might throw an exception. Also, do you intend to make getInstance() simply returning singleton? This means if the first call to new Config() throws an exception, getInstance() will not try to reconstruct it. This might not be common in production, but I don't want to make any behavior change. I see you point. Maybe, you can lock on a object. Xuelei --Max [1] https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.4.3.6 Xuelei On 5/25/2015 10:16 PM, Weijun Wang wrote: Hi All Please review a code change at http://cr.openjdk.java.net/~weijun/8080911/webrev.00/ I've limit the synchronized block to Config creation only and therefore won't deadlock with EType's class initialization. Noreg-hard. The EType call is at class initialization and only run once in a VM session, which is extremely difficult to catch. Thanks Max
Re: RFR 8080911: sun/security/krb5/auto/UseCacheAndStoreKey.java timed out intermittently
On 5/26/2015 9:22 AM, Xuelei Fan wrote: On 5/26/2015 9:06 AM, Weijun Wang wrote: On 5/26/2015 7:59 AM, Xuelei Fan wrote: synchronized on class looks a little bit unsafe to me. Why? Isn't it the same as making a static method synchronized? [1] Other code may be also able to lock on class. Code 1: lock on MyClass.class Code 2: lock on MyClass.class Code 1 and 2 know nothing about each other. The behavior may be weird. I don't think it is a good practice. So you are not a fan of all synchronized methods? :-) I thought it's quite common to use synchronized static methods to prevent simultaneous access to a static field. While a method in another class can lock on thisClass.class (in this case the class is internal), I don't see why it wants to do this. This is not casual, it's just evil. As singleton is a static variable, creating the instance during initialization looks safer. - private static Config singleton = null; + private static Config singleton = new Config(); This line might throw an exception. Also, do you intend to make getInstance() simply returning singleton? This means if the first call to new Config() throws an exception, getInstance() will not try to reconstruct it. This might not be common in production, but I don't want to make any behavior change. I see you point. Maybe, you can lock on a object. A private static final Object? Yes I can, but is it worth doing? Thanks Max Xuelei --Max [1] https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.4.3.6 Xuelei On 5/25/2015 10:16 PM, Weijun Wang wrote: Hi All Please review a code change at http://cr.openjdk.java.net/~weijun/8080911/webrev.00/ I've limit the synchronized block to Config creation only and therefore won't deadlock with EType's class initialization. Noreg-hard. The EType call is at class initialization and only run once in a VM session, which is extremely difficult to catch. Thanks Max
Re: RFR 8080911: sun/security/krb5/auto/UseCacheAndStoreKey.java timed out intermittently
I do not like class level synchronization because it may not work as expected, especially if the synchronization can be used by other codes. However, your update does not change this behavior. The fix looks fine to me. Please go ahead if you don't want to use object level synchronization. Xuelei On 5/26/2015 10:40 AM, Weijun Wang wrote: On 5/26/2015 9:22 AM, Xuelei Fan wrote: On 5/26/2015 9:06 AM, Weijun Wang wrote: On 5/26/2015 7:59 AM, Xuelei Fan wrote: synchronized on class looks a little bit unsafe to me. Why? Isn't it the same as making a static method synchronized? [1] Other code may be also able to lock on class. Code 1: lock on MyClass.class Code 2: lock on MyClass.class Code 1 and 2 know nothing about each other. The behavior may be weird. I don't think it is a good practice. So you are not a fan of all synchronized methods? :-) I thought it's quite common to use synchronized static methods to prevent simultaneous access to a static field. While a method in another class can lock on thisClass.class (in this case the class is internal), I don't see why it wants to do this. This is not casual, it's just evil. As singleton is a static variable, creating the instance during initialization looks safer. - private static Config singleton = null; + private static Config singleton = new Config(); This line might throw an exception. Also, do you intend to make getInstance() simply returning singleton? This means if the first call to new Config() throws an exception, getInstance() will not try to reconstruct it. This might not be common in production, but I don't want to make any behavior change. I see you point. Maybe, you can lock on a object. A private static final Object? Yes I can, but is it worth doing? Thanks Max Xuelei --Max [1] https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.4.3.6 Xuelei On 5/25/2015 10:16 PM, Weijun Wang wrote: Hi All Please review a code change at http://cr.openjdk.java.net/~weijun/8080911/webrev.00/ I've limit the synchronized block to Config creation only and therefore won't deadlock with EType's class initialization. Noreg-hard. The EType call is at class initialization and only run once in a VM session, which is extremely difficult to catch. Thanks Max
