code.onehippo.org is currently readonly. We are migrating to code.bloomreach.com, please continue working there on Monday 14/12. See: https://docs.bloomreach.com/display/engineering/GitLab

Commit f46028aa authored by Ard Schrijvers's avatar Ard Schrijvers

REPO-1886 Correct all tests by using Futures instead of assertions in other threads

A couple of tests have the following pattern:
 @Test
    public void assert_something() throws Exception {
        Thread lockThread = new Thread(() -> {
            fail("I failed");
        });
        lockThread.start();
        lockThread.join();
    }
The problem with this pattern is that although something gets logged like
Exception in thread "Thread-2" java.lang.AssertionError: I failed
the test itself just passes because the Thread the runs the test didn't get the exception

Next to this, also improved the LockManagerBasicTest to not use Thread.sleep
and also include the LockResource in its tests
parent 9cd9c5a2
......@@ -15,18 +15,23 @@
*/
package org.onehippo.repository.lock;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.stream.Stream;
import org.junit.Before;
import org.junit.Test;
import org.onehippo.cms7.services.lock.LockException;
import org.onehippo.cms7.services.lock.LockManagerException;
import org.onehippo.cms7.services.lock.LockResource;
import org.onehippo.repository.lock.memory.MemoryLockManager;
import org.onehippo.testutils.log4j.Log4jInterceptor;
import static java.util.concurrent.Executors.newSingleThreadExecutor;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
......@@ -68,32 +73,67 @@ public class MemoryLockManagerTest {
}
@Test
public void other_thread_cannot_unlock_() throws Exception {
public void other_thread_cannot_unlock_without_lock_resource() throws Exception {
memoryLockManager.lock("123");
Thread lockThread = new Thread(() -> {
final ExecutorService executorService = newSingleThreadExecutor();
final Future<Stream<String>> future = newSingleThreadExecutor().submit(() -> {
// other thread should not successfully unlock and we expect an error to be logged
try (Log4jInterceptor interceptor = Log4jInterceptor.onWarn().trap(MemoryLockManager.class).build()) {
memoryLockManager.unlock("123");
assertTrue(interceptor.messages().anyMatch(m -> m.contains("Thread '"+Thread.currentThread().getName()+"' should never had invoked #unlock(123)")));
return interceptor.messages();
}
// "123" should still be locked
});
try {
assertTrue(memoryLockManager.isLocked("123"));
} catch (LockManagerException e) {
fail("#isLocked check failed");
}
final Stream<String> messages = future.get();
assertTrue(messages.anyMatch(m -> m.contains("should never had invoked #unlock(123)")));
assertTrue(memoryLockManager.isLocked("123"));
assertEquals(1, memoryLockManager.getLocks().size());
memoryLockManager.unlock("123");
executorService.shutdown();
}
@Test
public void other_thread_can_unlock_with_lock_resource() throws Exception {
final CountDownLatch countDownLatch = new CountDownLatch(1);
final ExecutorService executorService = newSingleThreadExecutor();
final Future<LockResource> futureLock = newSingleThreadExecutor().submit(() -> {
try {
assertTrue(memoryLockManager.isLocked("123"));
} catch (LockManagerException e) {
fail("#isLocked check failed");
return memoryLockManager.lock("123");
} finally {
countDownLatch.countDown();
}
});
lockThread.start();
lockThread.join();
assertEquals(1, memoryLockManager.getLocks().size());
countDownLatch.await();
try {
assertTrue(memoryLockManager.isLocked("123"));
} catch (LockManagerException e) {
fail("#isLocked check failed");
}
final LockResource lockResource = futureLock.get();
assertFalse(Thread.currentThread() == lockResource.getHolder());
assertTrue(lockResource.getHolder().isAlive());
lockResource.close();
assertFalse("Lock should have been released by main thread via the LockResource", memoryLockManager.isLocked("123"));
executorService.shutdown();
}
@Test
public void when_other_thread_contains_lock_a_lock_exception_is_thrown_on_lock_attempt() throws Exception {
memoryLockManager.lock("123");
final ExecutorService executorService = newSingleThreadExecutor();
try {
newSingleThreadExecutor().submit(() -> {
executorService.submit(() -> {
memoryLockManager.lock("123");
return true;
}).get();
......@@ -102,24 +142,24 @@ public class MemoryLockManagerTest {
Throwable cause = e.getCause();
assertTrue(cause instanceof LockException);
}
executorService.shutdown();
}
@Test
public void when_other_thread_contains_lock_it_cannot_be_unlocked_by_other_thread() throws Exception {
Thread lockThread = new Thread(() -> {
try {
memoryLockManager.lock("123");
// make sure the lock thread is alive long enough
Thread.sleep(200);
} catch (LockException | InterruptedException e) {
fail(e.toString());
}
});
lockThread.start();
// give time to the lockThread
Thread.sleep(100);
final ExecutorService executorService = newSingleThreadExecutor();
final CountDownLatch countDownLatch1 = new CountDownLatch(1);
final CountDownLatch countDownLatch2 = new CountDownLatch(1);
final Future<LockResource> futureLock = executorService.submit(() -> {
final LockResource lock = memoryLockManager.lock("123");
countDownLatch1.countDown();
countDownLatch2.await();
return lock;
});
countDownLatch1.await();
// other thread should not successfully unlock and we expect an error to be logged
try (Log4jInterceptor interceptor = Log4jInterceptor.onWarn().trap(MemoryLockManager.class).build()) {
......@@ -135,24 +175,36 @@ public class MemoryLockManagerTest {
// "123" should still be locked
assertTrue(memoryLockManager.isLocked("123"));
countDownLatch2.countDown();
try {
final LockResource lockResource = futureLock.get();
lockResource.close();
} catch (ExecutionException e){
fail(e.toString());
}
executorService.shutdown();
// "123" should be freed by lockResource.close();
assertFalse(memoryLockManager.isLocked("123"));
}
@Test
public void when_thread_containing_lock_has_ended_without_unlocking_the_lock_can_be_reclaimed_by_another_thread() throws Exception {
Thread lockThread = new Thread(() -> {
try {
memoryLockManager.lock("123");
// make sure the lock thread is alive long enough
Thread.sleep(200);
} catch (LockException | InterruptedException e) {
fail(e.toString());
}
});
lockThread.start();
final ExecutorService executorService = newSingleThreadExecutor();
final CountDownLatch countDownLatch1 = new CountDownLatch(1);
final CountDownLatch countDownLatch2 = new CountDownLatch(1);
final Future<LockResource> futureLock = executorService.submit(() -> {
final LockResource lock = memoryLockManager.lock("123");
countDownLatch1.countDown();
countDownLatch2.await();
return lock;
});
// give time for the lockThread to retrieve the lock
Thread.sleep(100);
countDownLatch1.await();
try {
memoryLockManager.lock("123");
......@@ -161,12 +213,20 @@ public class MemoryLockManagerTest {
// expected
}
assertEquals("123", memoryLockManager.getLocks().iterator().next().getLockKey());
assertEquals(lockThread.getName(), memoryLockManager.getLocks().iterator().next().getLockThread());
countDownLatch2.countDown();
try {
final LockResource lockResource = futureLock.get();
assertEquals("123", memoryLockManager.getLocks().iterator().next().getLockKey());
assertEquals(lockResource.getHolder().getName(), memoryLockManager.getLocks().iterator().next().getLockThread());
} catch (ExecutionException e){
fail(e.toString());
}
// wait for the lockThread to finish : it did not unlock but since the thread is not live any more, the lock
executorService.shutdown();
// The lockThread did not unlock but since the thread is not live any more, the lock
// should again be eligible for other threads
lockThread.join();
try (Log4jInterceptor interceptor = Log4jInterceptor.onWarn().trap(MemoryLockManager.class).build()) {
assertEquals(0, memoryLockManager.getLocks().size());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment