RFR: 8014814 (str) StringBuffer "null" is not appended

[Martin Buchholz]

Note that my pending change
http://cr.openjdk.java.net/~martin/webrevs/openjdk8/getChars/getChars.patch
does the same kind of thing, but without recursive lock acquisitions.

I'm curious why a recursive lock acquisition would be considered "very"
cheap.  Is there some hotspot magic, or is it simply that we have another
write to a cache line that is already probably owned by the cpu by virtue
of the previous cas to acquire?


On Sun, May 19, 2013 at 10:48 PM, David Holmes <david.holmes at oracle.com>wrote:

> The change put through for JDK-8013395 (StringBuffer toString cache) has
> exposed a previously unnoticed bug in the StringBuffer.append(**CharSequence
> cs) method. That method claimed to achieve synchronization (and then
> correct toStringCache behaviour) by the super.append method calling other
> StringBuffer methods after narrowing of cs to a specific type. But that is
> incorrect if cs==null as in that case the AbstractStringBuilder.**appendNull
> method is called directly, with no calls to an overridden StringBuffer
> method. (I have verified that none of the other methods claiming to not
> need sync suffer from a similar flaw - this is an isolated case.)
>
> Consequently we started failing some existing append(null) tests.
>
> The fix is quite simple: append(CharSequence) behaves as for other append
> methods and is declared synchronized and clears the cache explicitly. The
> existing test is extended to check append(null).
>
> webrev:
>
> http://cr.openjdk.java.net/~**dholmes/8014814/webrev/<http://cr.openjdk.java.net/~dholmes/8014814/webrev/>
>
> This fix does mean that recursive synchronization will now be used for
> append(CharSequence) but recursive synchronization is very cheap. An
> alternative fix suggested by Alan Bateman in the bug report is to override
> appendNull and add the synchronization there. That requires a change to the
> accessibility of AbstractStringBuilder.**appendNull so I chose the more
> constrained fix. Alan's fix will also introduce nested synchronization, but
> only for the append(null) case. As I said I don't think performance will be
> a concern here.
>
> Testing (in progress): JPRT -testset core, SQE test that caught this
>
> Thanks,
> David
>