Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-12 Thread Martin Buchholz
On Thu, Sep 12, 2019 at 10:53 AM Pavel Rappo wrote: > Martin, > > May I add you to the list of people who reviewed this change? I mean the > concurrency-related portion of the change, specifically in the test. Not > the LDAP part. > Sure!

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-12 Thread Pavel Rappo
Martin, May I add you to the list of people who reviewed this change? I mean the concurrency-related portion of the change, specifically in the test. Not the LDAP part. > On 12 Sep 2019, at 16:21, Martin Buchholz wrote: > > > > On Thu, Sep 12, 2019 at 4:36 AM Pavel Rappo wrote: > > I

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-12 Thread Daniel Fuchs
Hi Pavel, The changes in webrev.02 look good to me as well. Thanks for renaming the constants in the test (I mean TOLERANCE vs RIGHT_MARGIN and LEFT_MARGIN) - that makes for a much better read. best regards, -- daniel On 12/09/2019 13:26, Rob McKenna wrote: Here's the updated version of the

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-12 Thread Martin Buchholz
On Thu, Sep 12, 2019 at 4:36 AM Pavel Rappo wrote: > > I should have expressed myself more clear. I meant that the main thread > and the thread created (and started) by `newStartedThread` (the test tread) > do not wait for *each other* before they begin. This might be needed to > make sure that

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-12 Thread Rob McKenna
This looks great to me. Thanks for taking a look at it. -Rob On 12/09/19 12:36, Pavel Rappo wrote: > > On 12 Sep 2019, at 03:54, Martin Buchholz wrote: > > > > We're mostly in agreement. > > Good to hear and I agree! > > > On Wed, Sep 11, 2019 at 11:02 AM Pavel Rappo >

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-12 Thread Pavel Rappo
> On 12 Sep 2019, at 03:54, Martin Buchholz wrote: > > We're mostly in agreement. Good to hear and I agree! > On Wed, Sep 11, 2019 at 11:02 AM Pavel Rappo > wrote: > > > a. is not using any synchronization aid to make sure both threads wait for > each other

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-11 Thread Martin Buchholz
We're mostly in agreement. (Also, I'm not actually an ldap reviewer.) On Wed, Sep 11, 2019 at 11:02 AM Pavel Rappo wrote: > I agree that this example [1] looks readable (depending on a reader, of > course). I think it looks readable mostly because it is very explicit. > However, in a domain

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-11 Thread Pavel Rappo
Martin, > On 10 Sep 2019, at 17:40, Martin Buchholz wrote: > > Here's a canonical example of an "expected timeout" test that seems natural > and readable to me. > timeoutMillis() returns 12ms, adjusted by timeout factor. Ldap might need a > bigger value. > > > /** > * timed await

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-10 Thread Martin Buchholz
Here's a canonical example of an "expected timeout" test that seems natural and readable to me. timeoutMillis() returns 12ms, adjusted by timeout factor. Ldap might need a bigger value. /** * timed await times out if not counted down before timeout */ public void

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-10 Thread Martin Buchholz
On Tue, Sep 10, 2019 at 7:25 AM Pavel Rappo wrote: > > On 6 Sep 2019, at 20:02, Martin Buchholz wrote: > > > > Martin's rules for test methods: > > - never have more than 100 millisecond total expected runtime > > - never have more than 12 millisecond blocking time for any single > operation >

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-10 Thread Pavel Rappo
> On 6 Sep 2019, at 20:02, Martin Buchholz wrote: > > Martin's rules for test methods: > - never have more than 100 millisecond total expected runtime > - never have more than 12 millisecond blocking time for any single operation > - yet be able to survive a one-time 5 second thread suspension

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-06 Thread Martin Buchholz
Martin's rules for test methods: - never have more than 100 millisecond total expected runtime - never have more than 12 millisecond blocking time for any single operation - yet be able to survive a one-time 5 second thread suspension at any time --- adjusting wait time looks like an

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-06 Thread Pavel Rappo
> On 6 Sep 2019, at 17:11, Martin Buchholz wrote: > > Bigger picture: all of this timeout-fiddling infrastructure should be in a > test library. It's not easy! > > test/jdk/java/util/concurrent/tck effectively has its own test library, for > historical reasons. Agreed. It is not an easy

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-06 Thread Pavel Rappo
> On 6 Sep 2019, at 15:59, Martin Buchholz wrote: > > I took another look at LdapTimeoutTest.java. Many thanks! > I was surprised to see RIGHT_MARGIN. In jsr166 we succeed in having one > timeout that is long enough to "never happen". I'm still advocating the 10 > second value. > > I was

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-06 Thread Martin Buchholz
Bigger picture: all of this timeout-fiddling infrastructure should be in a test library. It's not easy! test/jdk/java/util/concurrent/tck effectively has its own test library, for historical reasons. On Fri, Sep 6, 2019 at 7:59 AM Martin Buchholz wrote: > I took another look at

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-06 Thread Martin Buchholz
I took another look at LdapTimeoutTest.java. I was surprised to see RIGHT_MARGIN. In jsr166 we succeed in having one timeout that is long enough to "never happen". I'm still advocating the 10 second value. I was surprised to see LEFT_MARGIN. We just fixed Thread.sleep, so we have no known

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-09-06 Thread Pavel Rappo
Martin, thanks for having a look at it. I'd appreciate if you could have a look at the timeout measuring mechanics in assertCompletion/assertIncompletion specifically, maybe to spot something that is grossly inadequate. I tried to accommodate some usual suspects of timeout measurements

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-08-30 Thread Martin Buchholz
Not really a review, but: For many years we've been using 10 seconds (scaled by timeout factor) as a duration long enough that a timeout is a real failure. Which is close to your own 20 seconds. Of course, no value is surely safe. Probably, parallel testing infrastructure for timeouts should be

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-08-30 Thread Daniel Fuchs
On 30/08/2019 13:54, Pavel Rappo wrote: Updated, http://cr.openjdk.java.net/~prappo/8151678/webrev.01/ Changes look good! best regards, -- daniel

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-08-30 Thread Pavel Rappo
Updated, http://cr.openjdk.java.net/~prappo/8151678/webrev.01/ > On 30 Aug 2019, at 12:28, Daniel Fuchs wrote: > > Hi Pavel, > > Looks probably OK ;-) > > One thing though is that it's not always guaranteed that > InetAddress.getByName("localhost") will resolve to the same > address than

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

2019-08-30 Thread Daniel Fuchs
Hi Pavel, Looks probably OK ;-) One thing though is that it's not always guaranteed that InetAddress.getByName("localhost") will resolve to the same address than that returned by InetAddess.getLoopbackAddress(), so it may be better to use the literal address (which may be an IPv6 literal