We're mostly in agreement. (Also, I'm not actually an ldap reviewer.) On Wed, Sep 11, 2019 at 11:02 AM Pavel Rappo <pavel.ra...@oracle.com> 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 other than concurrency we are not usually concerned > with this level of detail. I think you *are* in the domain of concurrency! > a. is not using any synchronization aid to make sure both threads wait for > each other (probably, the timeout value accommodates that) > Thread.join is a synchronization aid! But there's no shortage of others. We typically use a CountDownLatch to synchronize with another thread. > b. uses a number of tiny low-level methods like newStartedThread, > awaitTermination, millisElapsedSince, manual time assertions, etc. > c. has assertions spread across 2 threads > I don't see that as a problem, as long as every assertion failure propagates properly to become a test failure. > > (b) probably allows you to reuse components in places unrelated to > timeouts. At the same time you don't seem to have any higher-level reusable > component (i.e. my guess is that this code is more or less repeated in > other places in that test suite, which is not necessarily a bad thing). > > However, I fully agree with you that this functionality should be a > utility method of the test library. > > ------------------------- > [1] I assume this is an excerpt from CountDownLatchTest.java. If so, then > for the reader's convenience this method could be seen in its context at > http://hg.openjdk.java.net/jdk/jdk/file/d52f77f0acb5/test/jdk/java/util/concurrent/tck/CountDownLatchTest.java#l198 > [2] I'm not saying that those things are wrong or that I disagree with any > of that. It's just an observation from reading this example. > >
