Re: 9 RFR: 8178147: Race conditions in timeout handling code in http/2 incubator client
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.
Re: 9 RFR: 8178147: Race conditions in timeout handling code in http/2 incubator client
> On 11 Apr 2017, at 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/ Thanks for going the extra mile on this one. Good catch, and the fix looks good to me. -Chris.
JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code
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
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.
