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

Mandy Chung mandy.chung at oracle.com
Fri May 10 23:55:24 UTC 2013


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.


> http://cr.openjdk.java.net/~sherman/8012326/webrev/
> -Sherman

More information about the core-libs-dev mailing list