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

[Xueming Shen]

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