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

Wed May 8 19:49:59 UTC 2013

Hi Sherman,

I reviewed webrev and also checked out webrev.newECP.   While it's good 
to simplify the synchronization needed in ExtendedCharsets, maybe better 
to separpate the ExtendedCharsets change from the fix for 8012326 that 
would require a more detailed review (I can do that next).

Charset.java
    ExtendedCharsets provider only needs to be instantiated once. The 
ECP initialization can be simplified by using the 
initialization-on-demand holder idiom to replace the existing 
initialization logic using the extendedProviderLock and 
extendedProviderProbed.  You could also simply have a holder class to 
define the lookupExtendedCharset (probably better to rename it to 
"charsetForName") and charsets() methods to return one or all available 
charsets from the extended provider respectively.  I think that would be 
cleaner and easy to read.

MSISO2022JP.java and ISO2022_JP_2.java
    Similiarly, it'd be cleaner to use a holder class to initialize the 
DEC02XX and ENC02XX variable.

Mandy

On 5/4/2013 12:05 PM, Xueming Shen wrote:
> Thanks Ulf!
>
> There is another version with a new ExtendedCharsets.java at
>
> http://cr.openjdk.java.net/~sherman/8012326/webrev.newECP/
>
> I merged the stuff in AbstractCharsetProvider into ExtendedCharsets.java.
> The standardcharset provider now uses the FastCharsetProvider, so there
> is no need to have an abstract class anymore, as long as we are not going
> to add a new or separate the charsets.jar. I kinda remember there was
> a plan to further divide the charsets.jar in the past though...
>
> I took the chance to "clean up" the synchronization mechanism in
> ExtendedCharsets. It appears there are two sync needs here.
>
> One is to protect the "cache" inside lookup(), which triggers the race
> condition if the lookup() gets invoked by multiple threads and the
> "cahce" map gets accessed/updated at the same time, this is reported
> and fixed by 4685305 [1], the original fix is to put the sync block in
> AbstractCharsetProvider.charsetForName().  We put in another sync
> block in iterator.next() for 6898310 [2], which is the trigger of this 
> bug.
> In the new version, I "consolidated" them together into lookup()
>
> Another sync need is for the "init()", in which it may update the 
> aliasMap,
> classMap and aliasNameMap via charset() and deleteCharset() if those
> special properties are defined. There are two sources for the charset()/
> deleteCharset(), one is from the constructor, one is from the init(), 
> given
> ExtendedCharsets is now singleton and get initialized at class init, 
> these
> should be no race concern between these two sources, so no need to
> have any sync block inside charset() and deleteCharset(), the only thing
> need to defend is inside init(), and all three public methods invoke the
> init() at the beginning of the method body.
>
> It appears I will still have to keep the same logic in Charset to access
> the ExtendedCharset, as it is need to be "probed", just in case it is not
> "installed"...
>
> Yes, there is also room to improve in FastCharsetProvider...I know I
> need pick some time on it.
>
> -Sherman
>
> On 5/4/13 10:09 AM, Ulf Zibis wrote:
>> Hi Sherman,
>>
>> looks good to me.
>>
>> Maybe you like to compare with webrevs from:
>> https://bugs.openjdk.java.net/show_bug.cgi?id=100092
>> https://bugs.openjdk.java.net/show_bug.cgi?id=100095
>>
>> -Ulf
>>
>> Am 03.05.2013 06:29, schrieb Xueming Shen:
>>> Hi,
>>>
>>> Please help review the proposed fix for 8012326.
>>>
>>> The direct trigger is the fix we put in #6898310 [1], in which we 
>>> added the
>>> synchronization to prevent a possible race condition as described in 
>>> $4685305.
>>>
>>> However it appears the added synchronization triggers another race 
>>> condition as
>>> showed in this stack trace [4] when run the test case [2][3].
>>>
>>> The root cause here is
>>>
>>> (1) The ExtendedCharsetProvider is intended to be used as a 
>>> singleton (as the
>>> probeExtendedProvider + lookupExtendedCharset mechanism in 
>>> Charset.java),
>>> however this is only true for the 
>>> Charset.forName()->lookup()...scenario. Multiple
>>> instances of ExtendedCharsetProvider is being created in 
>>> Charset.availableCharset()
>>> every time it is invoked, via providers()/ServiceLoader.load().
>>>
>>> (2) ISO2022_JP_2 and MSISO2022JP are provided via 
>>> ExtendedCharsetProvider,
>>> however both of them have two static variable 
>>> DEC02{12|08}/ENC02{12|08} that
>>> need to be initialized during the "class initialization", which will 
>>> indirectly trigger
>>> the invocation of ExtendedCharsetProvider.alisesFor(...)
>>>
>>> (3) static method ExtendedCharsets.aliaseFor() has a hacky 
>>> implementation that
>>> goes to AbstractCharsetProvider.alise() for lookup, which needs to 
>>> obtain a lock
>>> on its ExtendedCharesetProvider.instance. This will potential cause 
>>> race condition
>>> if the "instance" it tries to synchronize on is not its "own" 
>>> instance. This is possible
>>> because the constructor of ExtendedCharsetProvider overwrites static 
>>> "instance"
>>> variable with "this".
>>>
>>> Unfortunately as demonstrated  in [4], this appears to be what is 
>>> happening.
>>> The Thread-1/#9 is trying to synchronize on someone else's 
>>> ExtendedCharsetProvider
>>> instance <0xa4824730> (its own instance should be <0xa4835328>, 
>>> which it
>>> holds the monitor in the iterator.next()), it is blocked because 
>>> Thread-0 already holds
>>> the monitor of <0xa4824730> (in its iteratior.next()), but Thread-0 
>>> is blocked at
>>> Class.forName0(), which I think is because Thread-1 is inside it 
>>> already...two theads
>>> are deadlocked.
>>>
>>> A "quick fix/solution" is to remove the direct trigger in 
>>> ISO2022_JP_2 and
>>> MSISO2022JP to lazily-initialize those static variables as showed in 
>>> the
>>> webrev. However while this probably will solve the race condition, 
>>> the multiple
>>> instances of ExtendedCharsetProvider is really not desired. And 
>>> given the
>>> fact we have already hardwired ExtendedCharsetProvider (as well as the
>>> StandardCharset, for performance reason) for charset 
>>> lookup/forName(), the
>>> availableCharsets() should also utilize the "singleton" 
>>> ExtendedCharsetProvider
>>> instanc (extendedCharsetProvider) as well, instead of going to the 
>>> ServiceLoader
>>> every time for a new instance. The suggested change is to use Charset.
>>> extendedCharsetProvide via probeExtendedProvider() for extended charset
>>> iteration (availableCharset()). Meanwhile, since the 
>>> ExtendedCharsetProvider/
>>> charsets.jar should always be in the boot classpath (if installed, 
>>> and we are
>>> looking up via 
>>> Class.forName("sun.nio.cs.ext.ExtededCharsetProvider")), there
>>> is really no need for it to be looked up/loaded via the spi 
>>> mechanism (in
>>> fact it's redundant to load it again after we have lookup/iterate the
>>> hardwired "extendedCharsetProvider". So I removed the spi 
>>> description from
>>> the charsts.jar as well.
>>>
>>> An alternative is to really implement the singleton pattern in ECP, 
>>> however
>>> given the existing mechanism we have in Charset.java and the "hacky" 
>>> init()
>>> implementation we need in ECP, I go with the current approach.
>>>
>>> http://cr.openjdk.java.net/~sherman/8012326/webrev
>>>
>>> Thanks,
>>> Sherman
>>>
>>> [1] http://cr.openjdk.java.net/~sherman/6898310/webrev/
>>> [2] http://cr.openjdk.java.net/~sherman/8012326/runtest.bat
>>> [3] http://cr.openjdk.java.net/~sherman/8012326/Test.java
>>> [4] http://cr.openjdk.java.net/~sherman/8012326/stacktrace
>>>
>>
>