Codereview request for 6898310: (cs) Charset cache lookups should be synchronized

[Xueming Shen]

On 09/02/2011 06:00 AM, Rémi Forax wrote:
> On 09/02/2011 01:34 PM, Alan Bateman wrote:
>> Rémi Forax wrote:
>>>
>>> Arghhh, next() can return null !
>>>
>>> CharsetProvider provider = ...
>>> Iterator<Charset> it = provider.charsets();
>>> Iterator<Charset> it2 = provider.charsets();
>>> Charset charset = it.next();
>>> provider.deleteCharset(charset.name(), ...)
>>> System.out.println(it2.next());   // print null
>>>
>>> even if I'm not sure a lot of CharsetProvider actually calls 
>>> deleteCharset
>>> there is a possible bug here.
>>>
>> deleteCharset isn't a public method so I don't think this can happen.
>
> I doesn't happen currently in the JDK because deleteCharset() (and 
> charset())
> are only called in ExtendedCharset.init().

deleteCharset() was added later to workaround an "unusual" request for some
Japanese charsets, and the AbstractcharsetProvider was shared by the ext and
standard charset provider back then, so the "hook" was added in. In 
normal case,
the provider is really not supposed to add a bunch of charsets during 
the initialization
and then oops, I would like to delete some of them.  So deleteCharset() 
is really
not supposed to be used by anyone else, look it as an ugly 
implementation detail.
Given the standard charset no longer uses the AbstractCharsetProvider, 
it might
be the time to consider to have a clean implementation for the extended 
charset
provider in JDK8. Yes, this is on the jdk8 to-do list.

-Sherman

>
>> I don't disagree that that the iterator code should be re-written, 
>> just thinking it's separate from fixing the race condition.
>
> A way to solve the problem is to split AbstractCharsetProvider in two 
> objects
> i.e we should create a new object named CharsetProviderView that contains
> deleteCharset() and charset() and provide this object as parameter of 
> init.
>
> The idea is that during the initialization (in init()) calling 
> deleteCharset/charset is safe,
> not after.
>
>>
>> -Alan.
>>
>
> Rémi
>