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

[Peter Levart]

On 05/20/2013 09:16 AM, Peter Levart wrote:
> 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

But I don't know if the fix for "8010849: (str) Optimize 
StringBuilder.append(null)" is affected by above change (it would 
introduce a call-site with dispatch to two subclasses for the null value 
case in append(CharSequence))...

Regards, Peter

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