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

[Peter Levart]

On 05/20/2013 07:48 AM, David Holmes 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/
>
> 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.

Hi David, Alan,

The third possibility could be the following 
AbstractStringBuilder.append(CharSequence) method:


     public AbstractStringBuilder append(CharSequence s) {
         if (s == null || s instanceof String)
             return this.append((String)s);
         if (s instanceof AbstractStringBuilder)
             return this.append((AbstractStringBuilder)s);

         return this.append(s, 0, s.length());
     }


... and no synchronization in StringBuffer.append(CharSequence)...

Regards, Peter

>
> Testing (in progress): JPRT -testset core, SQE test that caught this
>
> Thanks,
> David