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

[David Holmes]

On 28/09/2016 11:26 PM, Aleksey Shipilev wrote:
> Peter, David,
>
> Would you mind discussing the theoretical topics on
> concurrency-interest@, for the benefits of others who don't track
> high-traffic OpenJDK list?

Well, noone responded to my comment on c-i, and I didn't expect a patch 
to appear on core-libs-dev so quickly.

David

> Thanks,
> -Aleksey
>
> On 09/28/2016 03:24 PM, Peter Levart wrote:
>>
>> On 09/28/2016 03:05 PM, David Holmes wrote:
>>> On 28/09/2016 10:44 PM, Peter Levart wrote:
>>>> Hi,
>>>>
>>>> According to discussion here:
>>>>
>>>> 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.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.
>>
>> That would be a nice property, yes.
>>
>>>
>>> Your bug requires that the code act as-if written:
>>>
>>> int temp = hash;
>>> if (temp == 0) {
>>>    hash = ...
>>> }
>>> return temp;
>>>
>>> and I do not believe that is allowed.
>>>
>>> David
>>
>> Well, I can't find anything like that in JMM description. Could you
>> point me to it? Above example only reads once from hash. The code in
>> question is this:
>>
>> if (hash == 0) { // 1st read
>>     hash = ...
>> }
>> return hash; // 2nd read
>>
>> And the bug requires the code to act like:
>>
>> int temp2 = hash; // 2nd read
>> int temp1 = hash; // 1st read
>> if (temp1 == 0) {
>>     return (hash = ...);
>> }
>> return temp2;
>>
>>
>> Is this allowed?
>>
>> Regards, Peter
>>
>>>
>>>>
>>>> For the bug:
>>>>
>>>>     https://bugs.openjdk.java.net/browse/JDK-8166842
>>>>
>>>>
>>>>
>>>> JDK 8 did not have this problem, so no back-porting necessary.
>>>>
>>>> Regards, Peter
>>>>
>>
>