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

[David Holmes]

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

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