RFR  8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect
martinrb at google.com
Thu Sep 12 02:54:41 UTC 2019
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.rappo at oracle.com> wrote:
> I agree that this example  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.
>  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
>  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.
More information about the core-libs-dev