Review Request: CR 7100996 - (spec str) IndexOutOfBoundsException when using a StringBuffer from multiple threads

[Ulf Zibis]

Hi again,

looking closer, I have some suggestions.

The term "create" is not valid code, so should not be tagged as such, same for the separating slashes.

The following phrase sounds wrong to me, even I'm not english native speaker:
"... the source data passed to create, ... , must ensure that ..."
1. Shouldn't the ',' be after the word "data" ?
2. How can source data ensure something? I think, only the caller code can ensure something.

"... call, or because the source data is immutable, or because the source data is not shared across 
threads."
... could be shorter and has superfluous ',' :
"... call or because the source data is immutable or not shared across threads."

While being here, maybe replace all <code> tags in the entire class by {@code}, same for {@link}.

So my suggestion:
While {@code StringBuffer} is designed to be safe to use
concurrently from multiple threads, the caller must ensure that
constructor/{@code append}/{@code insert} of {@code StringBuffer} has
[alternative: the constructor or {@code append}/{@code insert} methods of {@code StringBuffer} have]
a consistent and unchanging view on the source data
for the duration of the operation.  This could
be satisfied by the caller holding a lock on the source data during the
[alternative: synchronizing on the source data]
constructor/{@code append}/{@code insert} call or because
the source data is immutable or not shared across threads.

-Ulf


Am 21.06.2012 20:55, schrieb Mike Duigou:
> Great addition!
>
> I believe you should be using {@code} rather than <code> tags.
>
> I would say "constructors" rather than "create".
>
> I would add the word "operation" after the first instance of "create/append/insert"
>
> I would change "This could be done" to "This could be satisfied"
>
> Mike
>
> On Jun 21 2012, at 11:10 , Jim Gish wrote:
>
>> Taking all the previous comments into consideration, here's an update:
>>
>> diff -r fc575c78f5d3 src/share/classes/java/lang/StringBuffer.java
>> --- a/src/share/classes/java/lang/StringBuffer.java    Sun Jun 10 10:29:27 2012 +0100
>> +++ b/src/share/classes/java/lang/StringBuffer.java    Thu Jun 21 14:09:17 2012 -0400
>> @@ -63,6 +63,16 @@
>>   * appending or inserting from a source sequence) this class synchronizes
>>   * only on the string buffer performing the operation, not on the source.
>>   * <p>
>> + * While <code>StringBuffer</code> is designed to be safe to use
>> + * concurrently from multiple threads, the source data passed to create,
>> + * i.e. <code>StringBuffer(source)</code>, <code>append(source)</code>,
>> + * or <code>insert(source)</code>, if shared across threads, must ensure
>> + * that <code>create/append/insert</code> has a consistent and unchanging
>> + * view of the source data for the duration of the operation.  This could
>> + * be done by the caller holding a lock during the
>> + * <code>create/append/insert</code> call, or because the source data is
>> + * immutable, or because the source data is not shared across threads.
>> + * <p>
>>   * Every string buffer has a capacity. As long as the length of the
>>   * character sequence contained in the string buffer does not exceed
>>   * the capacity, it is not necessary to allocate a new internal
>>
>> Thanks,
>>    Jim
>>
>> On 06/21/2012 12:49 PM, David Schlosnagle wrote:
>>> On Thu, Jun 21, 2012 at 11:59 AM, Jim Gish<jim.gish at oracle.com>  wrote:
>>>> + * across threads, must ensure that StringBuffer.add/insert has a
>>> Jim,
>>>
>>> You may want to replace "add" with "append" if you are intending to
>>> reference the actual method name and not the generic operation.
>>> Additionally, you may want to use {@code ...} to show this context.
>>>
>>> Thanks,
>>> Dave
>> -- 
>> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
>> Oracle Java Platform Group | Core Libraries Team
>> 35 Network Drive
>> Burlington, MA 01803
>> jim.gish at oracle.com
>>
>