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

Ulf Zibis Ulf.Zibis at CoSoCo.de
Thu Jun 21 21:33:25 UTC 2012

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 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 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. *

+ * While StringBuffer is designed to be safe to use + * concurrently from multiple threads, the source data passed to create, + * i.e. StringBuffer(source), append(source), + * or insert(source), if shared across threads, must ensure + * that create/append/insert 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 + * create/append/insert call, or because the source data is + * immutable, or because the source data is not shared across threads. + *

* 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

More information about the core-libs-dev mailing list