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

[Xueming Shen]

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