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 c7572bae authored by Ate Douma's avatar Ate Douma

REPO-1887 [Backport 11.2] LockManager unit tests improvements

(cherry picked from commit 77715d79)

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

(cherry picked from commit f46028aa)
parent 282cb7a1
......@@ -7,3 +7,4 @@
.settings
.DS_Store
target/
/engine/workspaces/
......@@ -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());
......
......@@ -16,6 +16,7 @@
package org.onehippo.repository.lock;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;
import org.junit.Test;
import org.onehippo.cms7.services.lock.LockException;
......@@ -31,6 +32,7 @@ public class LockManagerUtilsTest extends AbstractLockManagerTest {
private final String key;
private final long lockTime;
private AtomicBoolean started = new AtomicBoolean(false);
public TimedLockRunnable(final String key, final long lockTime) {
this.key = key;
......@@ -40,26 +42,39 @@ public class LockManagerUtilsTest extends AbstractLockManagerTest {
@Override
public void run() {
try (LockResource lock = lockManager.lock(key)){
started.set(true);
Thread.sleep(lockTime);
} catch (LockException | InterruptedException ignore) {
// bad pattern, but OK for this test-case
}
}
void waitForLock(final long waitInterval) {
while (!started.get()) {
try {
Thread.sleep(waitInterval);
} catch (InterruptedException ignore) {
}
}
}
}
@Test
public void waitForLockTest() throws Exception {
long time = System.currentTimeMillis();
Thread lockThread = new Thread(new TimedLockRunnable("123", 500));
TimedLockRunnable runnable = new TimedLockRunnable("123", 500);
Thread lockThread = new Thread(runnable);
lockThread.start();
// give lockThread time to start
Thread.sleep(15);
runnable.waitForLock(10);
try {
lockManager.lock("123");
// unexpected, but release the lock to prevent (other) warning
lockManager.unlock("123");
fail("Lock should not be available yet");
} catch (LockException ignore) {
// expected
}
try (LockResource lock = LockManagerUtils.waitForLock(lockManager, "123", 100)) {
try (LockResource ignore = LockManagerUtils.waitForLock(lockManager, "123", 100)) {
assertTrue(System.currentTimeMillis() > time + 500);
}
lockThread.join();
......@@ -67,16 +82,18 @@ public class LockManagerUtilsTest extends AbstractLockManagerTest {
@Test
public void waitForLockTimeoutTest() throws Exception {
Thread lockThread = new Thread(new TimedLockRunnable("123", 500));
TimedLockRunnable runnable = new TimedLockRunnable("123", 500);
Thread lockThread = new Thread(runnable);
lockThread.start();
// give lockThread time to start
Thread.sleep(15);
runnable.waitForLock(10);
try {
lockManager.lock("123");
// unexpected, but release the lock to prevent (other) warning
lockManager.unlock("123");
fail("Lock should not be available yet");
} catch (LockException ignore) {
}
try (LockResource lock = LockManagerUtils.waitForLock(lockManager, "123", 100, 400)) {
try (LockResource ignore = LockManagerUtils.waitForLock(lockManager, "123", 100, 400)) {
fail("Lock should not be available yet");
} catch (TimeoutException ignore) {
// expected
......
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