Re: 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  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.

JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

Hi,

Please review the code change for below issue.

Bug:   https://bugs.openjdk.java.net/browse/JDK-8165437

Webrev: http://cr.openjdk.java.net/~vtewari/8165437/webrev0.5/index.html

This change will replace the "gettimeofday" to use the monotonic 
increasing clock(i used the existing function "JVM_NanoTime" instead of 
writing my own).


Thanks,

Vyom

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

[Daniel Fuchs]

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