RFR 8080911: sun/security/krb5/auto/UseCacheAndStoreKey.java timed out intermittently

[Sean Mullan]

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
>>>>>>>
>>>>>
>>>