Thanks for the review.
I have updated the Charset.java to use the init-on-depand holder idiom.
I don't think MSISO2022JP/ISO2022_JP_2 are really worth the cost of adding
two more classes, so I leave them asis.
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.
http://cr.openjdk.java.net/~sherman/8012326/webrev/
-Sherman
On 05/08/2013 12:49 PM, Mandy Chung wrote:
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
