RFR 8012326: Deadlock occurs when Charset.availableCharsets() is called by several threads at the same time

Tue May 14 20:00:53 UTC 2013

Thanks.  Looks good.
Mandy

On 5/14/13 12:51 PM, Xueming Shen wrote:
> Thanks Mandy!
>
> Webrev has been updated to put the "final" back as suggested.
>
> http://cr.openjdk.java.net/~sherman/8012326/webrev/
>
> https://jbs.oracle.com/bugs/browse/JDK-8014565 has been filed for future
> enhancement of ExtededCharsets class.
>
> -Sherman
>
> On 05/14/2013 12:29 PM, Mandy Chung wrote:
>> On 5/13/13 7:02 PM, Xueming Shen wrote:
>>> Hi Mandy,
>>>
>>> Here is the updated simple version as we chatted. I will leave the 
>>> ExtendedCharsets
>>> rewrite for next round.
>>>
>>> http://cr.openjdk.java.net/~sherman/8012326/webrev/ 
>>> <http://cr.openjdk.java.net/%7Esherman/8012326/webrev/>
>>>
>>
>> thanks for the update.  This looks fine.
>>
>> Minor nits:  the static fields in the new ISO2022_JP_2.CoderHolder 
>> and MSISO2022JP.CoderHolder classes can retain to be final in this 
>> version (I think it was removed for your previous version).
>>
>> I agree with you that we should clean up and simplify the 
>> ExtendedCharsets synchronization and what you have is a good start.  
>> Can you file a new bug for the future cleanup?
>>
>> Mandy
>>
>>> Thanks!
>>> -Sherman
>>>
>>>
>>> On 5/13/13 3:01 PM, Xueming Shen wrote:
>>>> Thanks Mandy for reviewing.
>>>>
>>>> Webrev has been updated to
>>>>
>>>> (1) added the sync back to charset/deleteCharset(). As you 
>>>> suggested, there
>>>> is race condition risk that the map is being accessed while the system
>>>> initialization completes.
>>>> (2) added the "holder" pattern for msiso2022_jp and iso2022_jp_2, 
>>>> as suggested.
>>>> (3) keep the AbstractCharsetProvider in repo for now.
>>>>
>>>> I have not eliminate the static reference to s.n.c.ExtendedCharsets 
>>>> class, yet,
>>>> since it is only invoked/referred after we have checked 
>>>> Class.forName. Is it
>>>> really a problem here? for example, the hotspo re-arrange the order of
>>>> execution?
>>>>
>>>> http://cr.openjdk.java.net/~sherman/8012326/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Esherman/8012326/webrev/>
>>>>
>>>> Thanks!
>>>> -Sherman
>>>>
>>>> http://cr.openjdk.java.net/~sherman/8012326/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Esherman/8012326/webrev/>
>>>>
>>>> On 05/10/2013 04:55 PM, Mandy Chung wrote:
>>>>> Sherman,
>>>>>
>>>>> The charsetForName, charsets and aliasesFor methods call init() 
>>>>> first and
>>>>> then access the maps with no synchronization.  While the maps are 
>>>>> being
>>>>> accessed by one thread and system initialization completes, 
>>>>> another thread
>>>>> calls charsetForName() that calls init() which will update the 
>>>>> maps.  So
>>>>> the maps is being updated while being accessed by another thread.  So
>>>>> I think the original synchronized(this) is still needed for the
>>>>> charsetForName, charsets and aliasesFor methods for correctness.
>>>>>
>>>>> The maps are only updated once and also only if ExtendedCharsets 
>>>>> provider
>>>>> is instantiated before booted and "sun.nio.cs.map" system property 
>>>>> is set.
>>>>> I think it's worth further clean up this 2-phase instance 
>>>>> initialization
>>>>> since ExtendedCharsets is now a singleton. Would 
>>>>> Charset.availableCharsets()
>>>>> and ExtendedCharsets.charsets() not be called during system 
>>>>> initialization?
>>>>> If we can restrict that they can't be called, maybe you can move 
>>>>> the 2nd
>>>>> phase initialization to ExtendedCharsetsHolder (i.e. calling 
>>>>> init() method)
>>>>> and perhapsExtendedCharsets.aliasesFor so that ExtendedCharsets 
>>>>> instance
>>>>> methods no longer need to call init. Just one thought.
>>>>>
>>>>> You propose to remove AbstractCharsetProvider class.  I don't have 
>>>>> the
>>>>> history to comment on whether you should keep/remove the
>>>>> AbstractCharsetProvider for other charset providers to extend. Just
>>>>> looking at the current implementation, it's not needed.
>>>>>
>>>>> More comments inlined below.
>>>>>
>>>>> On 5/10/2013 10:53 AM, Xueming Shen wrote:
>>>>>> Thanks for the review.
>>>>>>
>>>>>> I have updated the Charset.java to use the init-on-depand holder 
>>>>>> idiom.
>>>>>>
>>>>>
>>>>> L440: return sun.nio.cs.ext.ExtendedCharsets.getInstance();
>>>>>
>>>>> You would need to use 'epc' and invoke "getInstance" method via 
>>>>> reflection
>>>>> to eliminate the static reference to s.n.c.ExtendedCharsets class as
>>>>> it might not be present.
>>>>>
>>>>> I suggest to add these 2 methods in the ExtendedProviderHolder class:
>>>>>         static Charset charsetForName(String charsetName) {
>>>>>             return (extendedProvider != null)
>>>>>                         ? 
>>>>> extendedProvider.charsetForName(charsetName) : null;
>>>>>         }
>>>>>
>>>>>         static Iterator<Charset> charsets() {
>>>>>             return (extendedProvider != null)
>>>>>                         ? extendedProvider.charsets()
>>>>>                         : Collections.<Charset>emptyIterator();
>>>>>         }
>>>>>
>>>>>
>>>>> The lookup2() and availableCharsets() methods can be simplified to
>>>>> something like this:
>>>>>
>>>>>          if ((cs = standardProvider.charsetForName(charsetName)) 
>>>>> != null ||
>>>>> -            (cs = lookupExtendedCharset(charsetName)) != null ||
>>>>> +            (cs = 
>>>>> ExtendedProviderHolder.charsetForName(charsetName)) != null ||
>>>>>              (cs = lookupViaProviders(charsetName)) != null)
>>>>>
>>>>>
>>>>> +                    put(ExtendedProviderHolder.charsets(), m);
>>>>>                      for (Iterator<CharsetProvider> i = 
>>>>> providers(); i.hasNext();) {
>>>>>                          CharsetProvider cp = i.next();
>>>>>                          put(cp.charsets(), m);
>>>>>
>>>>>> I don't think MSISO2022JP/ISO2022_JP_2 are really worth the cost 
>>>>>> of adding
>>>>>> two more classes, so I leave them asis.
>>>>>>
>>>>>
>>>>> Are you concerned of the static footprint of the new class?
>>>>>
>>>>> I think the code is much cleaner but you may think it's overkill:
>>>>>    static class Holder {
>>>>>       static DoubleByte.Decoder DEC0208 = ....
>>>>>       static DoubleByte.Encoder ENC0208 = ....
>>>>>    }
>>>>>
>>>>> Or can you just cache new JIS_X_0208_MS932()or new JIS_X_0212()?
>>>>>
>>>>>> Agreed that the ExtendedCahrsets change can be separated, but 
>>>>>> since we're here
>>>>>> already, It would be convenient to just do them together and the 
>>>>>> "cleaner' code
>>>>>> then can be back-port together later when requested.
>>>>>
>>>>> It's fine with me.  I suggest to look into the possibility 
>>>>> separating the second phase update to the extended charsets if you 
>>>>> want to remove the synchronization on the instance methods.
>>>>>
>>>>> Mandy
>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~sherman/8012326/webrev/ 
>>>>>> <http://cr.openjdk.java.net/%7Esherman/8012326/webrev/>
>>>>>>
>>>>>> -Sherman
>>>>>>
>>>>
>>>
>>
>