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

[Mandy Chung]

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

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/
>>
>> Thanks!
>> -Sherman
>>
>> http://cr.openjdk.java.net/~sherman/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/
>>>>
>>>> -Sherman
>>>>
>>
>