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

[Michael McMahon]

Looks good Daniel. Good catch with the timeout ordering bug.

- Michael

On 11/04/2017, 12:51, Daniel Fuchs wrote:
> Hi,
>
> Please find below a new webrev that fixes an additional timeout
> ordering issue I discovered while testing:
>
> http://cr.openjdk.java.net/~dfuchs/webrev_8170940/webrev.02/
>
> While doing some further tests after incorporating Chris's feedback
> I noticed that the new test and the TimeoutOrdering test both started
> failing again on Windows (hard to tell why it wasn't failing when
> I tested before and why it started failing now - I rebased the fix
> on the newest jdk9/dev sources - anyway I identified the root of
> the issue - see below).
>
> The root cause lies in the TimeoutEvent, which implements compareTo
> to order events according to their deadline, in a way which is not
> consistent with equals (compareTo will return 0 if two events have
> the same deadline, even though they can be different events - that is
> compareTo is 0 but equals is false).
>
> This in itself wouldn't be an issue except that our HttpClientImpl
> uses a TreeSet to store the events in an ordered fashion, and
> TreeSet requires that compareTo and equals be consistent.
>
> So if by misfortune two TimedEvent happen to have the same deadline,
> results become unpredictable and one of them could be eliminated from
> the set, causing the corresponding request to never time out.
> (This is likely to be the real cause of 8170940 BTW).
>
> I modified TimeoutEvent to acquire an additional id distributed by
> an AtomicLong - and change dcompareTo to use that to further order
> two different events when their deadline is equal.
>
> With that I no longer see failures on windows.
>
> best regards,
>
> -- daniel
>
> On 07/04/2017 18:07, Chris Hegarty wrote:
>>
>>> On 7 Apr 2017, at 17:51, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>>
>>> Hi Chris,
>>>
>>> Here is the new webrev - where I have incorporated your feedback.
>>>
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8170940/webrev.01/
>>
>> Looks good to me Daniel.
>>
>> -Chris.
>>
>