> On 6 Sep 2019, at 15:59, Martin Buchholz <marti...@google.com> 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 surprised to see LEFT_MARGIN. We just fixed Thread.sleep, so we have > no known problems with JDK methods returning early - you can trust timed get! > You start measuring, by calling nanoTime, before you start the activity you > are measuring, so there should be no need for LEFT_MARGIN. You raised many good points. Let me try to address them. 1. RIGHT_MARGIN is not used for checking that the activity has stuck infinitely (assertIncompletion). INFINITY_MILLIS is used for that. RIGHT_MARGIN is used for checking that the activity takes some predefined amount of time (roughly). 2. As for the numeric value of INFINITY_MILLIS, the reason I chose 20 seconds is twofold. Firstly, the code under test is subject to different timeouts. Every timeout value should be distinctive. Otherwise, how would I differentiate between them? For example, if I chose INFINITY_MILLIS to be 10 seconds how would I know if the code is stuck due to the read timeout of 10 seconds or the "infinite timeout"? Secondly, I must take into account slow machines. Really slow virtual machines. Hence, minimal timeouts (read/connect) have a magnitude of seconds and tens of seconds and the "rightmost", infinite timeout, is 20 seconds. 3. LEFT_MARGIN might no longer be needed due to the fact that no timed methods return early (actually there is a comment about it inside those two assert methods). > We have some fresh thread-awaiting code here: > http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/JSR166TestCase.java?view=markup#l1443 Interesting. > Instead of communicating startTime from the test thread back to the main > thread, I would do my loMillis checking in the test thread, and hiMillis > checking in the main thread, like e.g. compare with a fresh test method > testTimedOffer > http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/tck/ArrayBlockingQueueTest.java?view=markup#l394 Understood. However, that might be a matter of taste. I prefer to have all the calculations and error handling in one place. Unless there's a good reason I wouldn't change it. > Timeouts should be adjusted via Utils.adjustTimeout That makes perfect sense. I never knew this method existed. Thanks! > On Fri, Sep 6, 2019 at 4:31 AM Pavel Rappo <pavel.ra...@oracle.com> wrote: > 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 failures. > I understand that since we're not working with real-time systems, my attempts > to build bullet-proof measurement mechanics are futile. > > -Pavel > > > On 30 Aug 2019, at 18:19, Martin Buchholz <marti...@google.com> wrote: > > > > 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 a test > > library method. I do something similar in JSR166TestCase > > > > /** > > * Runs all the given actions in parallel, failing if any fail. > > * Useful for running multiple variants of tests that are > > * necessarily individually slow because they must block. > > */ > > void testInParallel(Action ... actions) { > > ExecutorService pool = Executors.newCachedThreadPool(); > > try (PoolCleaner cleaner = cleaner(pool)) { > > > > On Fri, Aug 30, 2019 at 6:23 AM Daniel Fuchs <daniel.fu...@oracle.com> > > wrote: > > 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 >
