Request for review: Race conditions in java.nio.charset.Charset

[Ulf Zibis]

Sherman, thanks for your review ...

Am 07.10.2009 19:56, Xueming Shen schrieb:
> Though I did not write the cache code my reading suggests the existing 
> cache
> impl tries to avoid the relatively "expensive" synchronization for 
> most use scenarios,
> your approach however hits the synchronization for each/every lookup 
> invocation.
> So you should at least consider to put the sync block in a separate 
> lookup2.

Agreed:
(But why separate lookup2? I suspect that Charset.forName() would occur 
in excessive loops, so hotspot potentially cold inline it.)

    private static Charset lookup(String charsetName) {
        if (charsetName == null)
            throw new IllegalArgumentException("Null charset name");

        CacheItem ci = cache[0];
        if (ci != null && charsetName.equals(ci.name))
            return ci.charset;

        synchronized (Charset.class) {
            int pos;
            for (pos=1; ; pos++) {
            ...
        }
    }

Is synchronization more expensive, than instantiating new Object[] each 
time + 2 castings (to String + to Charset)?

>
> But personally I doubt we really care this "race condition", it's a 
> cache, a cache miss
> is not a disaster,

OK, but why didn't you you reject Bug Id 6881442 with the same 
argumentation.
... and missing the class's name in Class.name could only happen once, 
in contrast to "my" race condition, which theoretically could happen 
every time.

> in fact in most cases, since the charset has been in the cache
> already, it should be in the providers' "cache" as well,  especially 
> for the bundled
> standard & extended charset provider, the lookup actually is 1 or 2  
> O(1) hashmap
> lookup

... it's 2 hashmap lookup, see 
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6850361
+ 2 hashmap lookup for extended charset + 2n cost if in supplemental 
provider

> (yes, an additional name canonicalization and the "expensive" sync).

+ check extendedProviderProbed + synchronize latter + resolve SoftReference
(in fail: + hashmap lookup for class name + reflective instantiation + 
caching)

>
> So maybe we can do something like
>
> b = cache1;
> cache1 = a;
> cache2 = b;

Hm, if content of cache1 was updated to content, which is hold in a by 
other thread, saving cache1in b doesn't help.
(Maybe I didn't understand your trick.)

>
> to make the situation a little better, or maybe worse:-)
>

And what do you think about breaking the endless loop, while getting the 
default charset, and what about avoiding to cache wrong default charset?

-Ulf