RFR: 8166842: String.hashCode() has a non-benign data race

[Carsten Varming]

Dear David,

See inline.

On Wed, Sep 28, 2016 at 7:47 PM, David Holmes <david.holmes at oracle.com>
wrote:

> On 29/09/2016 3:44 AM, Carsten Varming wrote:
>
>> Dear Vitaly and David,
>>
>> Looking at your webrev the original code is:
>>
>> public int hashCode() {
>>   if (hash == 0 && value.length > 0) {
>>     hash = isLatin1() ? StringLatin1.hashCode(value)
>>   }
>>   return hash;
>> }
>>
>> There is a constructor:
>>
>> public String(String original) {
>>   this.value = original.value;
>>   this.coder = original.coder;
>>   this.hash = original.hash;
>> }
>>
>> that might write zero to the mutable field "hash".
>>
>> The object created by this constructor might be shared using plain reads
>> and writes between two threads[1] and the write of 0 in the constructor
>> might be interleaved with the reads and write in hashCode. Does this
>> capture the problem?
>>
>
> Because String has final fields there is a freeze action at the end of
> construction so that String instances are always safely published even if
> not "safely published".
>

I always thought that the freeze action only freezes final fields. The hash
field in String is not final and example 17.5-1 is applicable as far as I
can see (
https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5).
Has the memory model changed in JDK9 to invalidate example 17.5-1 or I am
missing something about String.

Carsten


> David
>
>
> [1]: Meaning the is no happens-before relationship established between
>> object construction and another thread calling hashCode on the object.
>>
>> Carsten
>>
>> On Wed, Sep 28, 2016 at 10:13 AM, Vitaly Davidovich <vitalyd at gmail.com
>> <mailto:vitalyd at gmail.com>> wrote:
>>
>>     On Wednesday, September 28, 2016, David Holmes
>>     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>>
>>     wrote:
>>
>>     > On 28/09/2016 10:44 PM, Peter Levart wrote:
>>     >
>>     >> Hi,
>>     >>
>>     >> According to discussion here:
>>     >>
>>     >> http://cs.oswego.edu/pipermail/concurrency-interest/2016-
>>     <http://cs.oswego.edu/pipermail/concurrency-interest/2016->
>>     >> September/015414.html
>>     >>
>>     >>
>>     >> it seems compact strings introduced (at least theoretical)
>> non-benign
>>     >> data race into String.hasCode() method.
>>     >>
>>     >> Here is a proposed patch:
>>     >>
>>     >> http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String
>>     <http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String>.
>>
>>     >> hashCode/webrev.01/
>>     >>
>>     >
>>     > I'm far from convinced that the bug exists - theoretical or
>>     otherwise -
>>     > but the "fix" is harmless.
>>     >
>>     > When we were working on JSR-133 one of the conceptual models is
>>     that every
>>     > write to a variable goes into the set of values that a read may
>>     potentially
>>     > return (so no out-of-thin-air for example). happens-before
>> establishes
>>     > constraints on which value can legally be returned - the most
>>     recent. An
>>     > additional property was that once a value was returned, a later
>>     read could
>>     > not return an earlier value - in essence once a read returns a
>>     given value,
>>     > all earlier written values are removed from the set of potential
>>     values
>>     > that can be read.
>>     >
>>     > Your bug requires that the code act as-if written:
>>     >
>>     > int temp = hash;
>>     > if (temp == 0) {
>>     >    hash = ...
>>     > }
>>     > return temp;
>>
>>     It's the other way I think:
>>
>>     int temp = hash; // read 0
>>     if (hash == 0) // reread a non 0
>>          hash = temp = ...
>>      return temp // return 0
>>
>>     It's unlikely but what prohibits that?
>>
>>     >
>>     > and I do not believe that is allowed.
>>     >
>>     > David
>>     >
>>     >
>>     >> For the bug:
>>     >>
>>     >>     https://bugs.openjdk.java.net/browse/JDK-8166842
>>     <https://bugs.openjdk.java.net/browse/JDK-8166842>
>>     >>
>>     >>
>>     >>
>>     >> JDK 8 did not have this problem, so no back-porting necessary.
>>     >>
>>     >> Regards, Peter
>>     >>
>>     >>
>>
>>     --
>>     Sent from my phone
>>
>>
>>