Fix test and trace log message #5645
Conversation
| @@ -90,13 +90,15 @@ public void testFastLockWithTimeout() throws Throwable { | |||
| public void testTryLockWithTimeoutAfterLockWithSmallTimeout() throws Throwable { | |||
| assertTrue(await(lock.tryLock())); | |||
| CompletableFuture<Boolean> tryLock = lock.tryLock(1, TimeUnit.NANOSECONDS); | |||
| TimeUnit.NANOSECONDS.sleep(1); | |||
There was a problem hiding this comment.
My suggestion was to do assertFalse(await(tryLock)); first and await(lock.unlock()); later, without other delays.
There was a problem hiding this comment.
But is that case I'm not testing the fact that as the unlock arrives later (without waiting) the request is already done and expired.
There was a problem hiding this comment.
maybe the confusion comes becauseI should test isCompleted() true, because it should be completed after 1 nano second and await should not be needed. WDYT ?
There was a problem hiding this comment.
If tryLock() needs a remote call, we can't say exactly how much it will take. In a best case scenario, assuming no big GC pauses, we could wait for only 100ms for tryLock to finish. But we cannot assume that it will finish synchronously.
There was a problem hiding this comment.
it is happening locally, not remotely.
There was a problem hiding this comment.
I expire the CompletableFuture in the local ClusteredLockImpl after the timeout
There was a problem hiding this comment.
I gave the example of a remote call because the difference is bigger there, but even locally a thread can easily be taken off the CPU for 10ms: maybe it's blocking to log something (very common with trace logging enabled), maybe a GC has started, or maybe there are just not enough CPU cores for all the test threads that are running at the same time.
IMO we cannot make any assumptions about timings at this resolution, even a sleep(100) is just a recipe for random failures. If you want to check how much tryLock blocked for, you can still do that, but I'd use a margin of at least 500ms. And since a test shouldn't always sleep for 500ms, you should first check that tryLock() failed, then check the time it took, and then unlock the lock (in a finally block).
|
Needs rebase |
|
@danberindei there are test failures but nothing related I think |
|
Merged, thanks |
|
Thanks Tristan ;) |
|
@danberindei if you fell guilty, you can merge the other one ! :) |
https://issues.jboss.org/browse/ISPN-8615
https://issues.jboss.org/browse/ISPN-8581