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

[Xueming Shen]

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