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

[Ulf Zibis]

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
>