9 RFR: 8178147: Race conditions in timeout handling code in http/2 incubator client

[Daniel Fuchs]

Hi Chris,

Here is the new webrev - where I have incorporated your feedback.

http://cr.openjdk.java.net/~dfuchs/webrev_8170940/webrev.01/

If I don't hear from you that's what I plan to push on Monday.

best regards,

-- daniel

On 07/04/2017 15:12, Daniel Fuchs wrote:
> On 07/04/2017 14:31, Chris Hegarty wrote:
>> Daniel,
>>
>> On 06/04/17 11:32, Daniel Fuchs wrote:
>>> ...
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8170940/webrev.00/
>>
>> Looks good Daniel. Just a few comments.
>>
>> 1) Http1Exchange.java
>>
>>   Can 'operations' now be made private, and not a synchronizedList?
>>   Now that it is operated on only within synchronized blocks.
>
> Yes - will do and retest before pushing. Good catch.
>
>> 2) Exchange.java  L197 : synchronized(this)
>>
>>   I'm not sure what this synchronized block gets you? The code
>>   is racy, and both fields are volatile. Can this be removed?
>
> No. This is the double-locking idiom and the synchronized is necessary
> for atomic behavior (in order to get a consistent snapshot of the
> two fields).
> It's equivalent to a CAS where the compare would be done on two fields
> and the set on only one of them.
>
> best,
>
> -- daniel
>
>>
>> -Chris.
>