diff --git a/src/main/java/org/kiwiproject/dropwizard/util/startup/StartupLocker.java b/src/main/java/org/kiwiproject/dropwizard/util/startup/StartupLocker.java index c052f83a..0bc49006 100644 --- a/src/main/java/org/kiwiproject/dropwizard/util/startup/StartupLocker.java +++ b/src/main/java/org/kiwiproject/dropwizard/util/startup/StartupLocker.java @@ -1,14 +1,10 @@ package org.kiwiproject.dropwizard.util.startup; -import static java.util.Objects.isNull; -import static java.util.Objects.nonNull; -import static org.kiwiproject.base.KiwiPreconditions.requireNotNull; +import static org.kiwiproject.base.KiwiStrings.format; -import com.google.common.annotations.VisibleForTesting; import io.dropwizard.setup.Environment; import io.dropwizard.util.Duration; import lombok.AccessLevel; -import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -30,20 +26,12 @@ * Utilities to acquire and release a lock from ZooKeeper during startup of the service */ @Slf4j +@Getter(AccessLevel.PACKAGE) // For testing public class StartupLocker { - @Getter(AccessLevel.PACKAGE) - @VisibleForTesting private final ZooKeeperAvailabilityChecker zkAvailabilityChecker; - - @Getter(AccessLevel.PACKAGE) - @VisibleForTesting private final CuratorFrameworkHelper curatorFrameworkHelper; - - @Getter(AccessLevel.PACKAGE) - @VisibleForTesting private final CuratorLockHelper curatorLockHelper; - private final SystemExecutioner executioner; @Builder @@ -51,17 +39,29 @@ private StartupLocker(SystemExecutioner executioner, ZooKeeperAvailabilityChecker zkAvailabilityChecker, CuratorFrameworkHelper curatorFrameworkHelper, CuratorLockHelper curatorLockHelper) { - this.executioner = requireNotNull(executioner, "SystemExecutioner is required"); - this.zkAvailabilityChecker = Optional.ofNullable(zkAvailabilityChecker).orElse(new ZooKeeperAvailabilityChecker()); - this.curatorFrameworkHelper = Optional.ofNullable(curatorFrameworkHelper).orElse(new CuratorFrameworkHelper()); - this.curatorLockHelper = Optional.ofNullable(curatorLockHelper).orElse(new CuratorLockHelper()); + this.executioner = Optional.ofNullable(executioner).orElseGet(SystemExecutioner::new); + this.zkAvailabilityChecker = Optional.ofNullable(zkAvailabilityChecker).orElseGet(ZooKeeperAvailabilityChecker::new); + this.curatorFrameworkHelper = Optional.ofNullable(curatorFrameworkHelper).orElseGet(CuratorFrameworkHelper::new); + this.curatorLockHelper = Optional.ofNullable(curatorLockHelper).orElseGet(CuratorLockHelper::new); } - @AllArgsConstructor + /** + * Model object containing information on the state of the acquired lock and necessary objects needed to release the lock. + */ + @Getter + @Builder public static class StartupLockInfo { - final CuratorFramework client; - final InterProcessLock lock; - final String lockPath; + + public enum LockState { + NOT_ATTEMPTED, ACQUIRED, ACQUIRE_FAIL + } + + CuratorFramework client; + InterProcessLock lock; + String lockPath; + LockState lockState; + String infoMessage; + Exception exception; } /** @@ -71,52 +71,67 @@ public static class StartupLockInfo { * @param lockTimeout the amount of time to wait for the lock to be acquired * @param curatorConfig the Curator configuration * @param environment the Dropwizard environment - * @return information about the acquired lock or null if lock could not be acquired + * @return information about the acquired lock or {@link Optional#empty()} if lock could not be acquired */ - public StartupLockInfo acquireStartupLock(String lockPath, Duration lockTimeout, PortAssignment assignment, CuratorConfig curatorConfig, Environment environment) { + public StartupLockInfo acquireStartupLock(String lockPath, + Duration lockTimeout, + PortAssignment assignment, + CuratorConfig curatorConfig, + Environment environment) { + if (assignment == PortAssignment.STATIC) { - return null; + return StartupLockInfo.builder() + .lockState(StartupLockInfo.LockState.NOT_ATTEMPTED) + .infoMessage("Using static port assignment. Lock not needed.") + .build(); } if (zkAvailabilityChecker.anyZooKeepersAvailable(curatorConfig)) { var curatorFramework = curatorFrameworkHelper.startCuratorFramework(curatorConfig); var lock = curatorLockHelper.createInterProcessMutex(curatorFramework, lockPath); - if (tryAcquireStartupLock(lock, lockPath, lockTimeout)) { + try { + tryAcquireStartupLock(lock, lockPath, lockTimeout); environment.lifecycle().addLifeCycleListener(new StartupWithLockJettyLifeCycleListener(curatorFramework, lock, lockPath, executioner)); - return new StartupLockInfo(curatorFramework, lock, lockPath); - } else { + return StartupLockInfo.builder() + .client(curatorFramework) + .lock(lock) + .lockPath(lockPath) + .lockState(StartupLockInfo.LockState.ACQUIRED) + .infoMessage("Lock acquired") + .build(); + } catch (LockAcquisitionException e) { LOG.warn("Lock on path [{}] not obtained. Closing Curator.", lockPath); curatorFrameworkHelper.closeQuietly(curatorFramework); + + return StartupLockInfo.builder() + .lockState(StartupLockInfo.LockState.ACQUIRE_FAIL) + .infoMessage("Failed to obtain startup lock") + .exception(e) + .build(); } - } else { - LOG.warn("No ZooKeepers are available from connect string [{}]", curatorConfig.getZkConnectString()); } - LOG.warn("Startup using dynamic ports will continue without ZooKeeper lock (which may result in port conflicts)"); - return null; + return StartupLockInfo.builder() + .lockState(StartupLockInfo.LockState.NOT_ATTEMPTED) + .infoMessage(format("No ZooKeepers are available from connect string [{}]", curatorConfig.getZkConnectString())) + .build(); } - private boolean tryAcquireStartupLock(InterProcessMutex lock, String lockPath, Duration lockTimeout) { - try { - LOG.debug("Start lock acquisition for path [{}]. Timeout set to [{}]", lockPath, lockTimeout); - curatorLockHelper.acquire(lock, lockTimeout.getQuantity(), lockTimeout.getUnit()); - LOG.debug("Acquired lock on path [{}]", lockPath); - return true; - } catch (LockAcquisitionException e) { - LOG.warn("Failed to obtain startup lock. Allow startup to continue and maybe we will be lucky with port assignment!", e); - return false; - } + private void tryAcquireStartupLock(InterProcessMutex lock, String lockPath, Duration lockTimeout) { + LOG.debug("Start lock acquisition for path [{}]. Timeout set to [{}]", lockPath, lockTimeout); + curatorLockHelper.acquire(lock, lockTimeout.getQuantity(), lockTimeout.getUnit()); + LOG.debug("Acquired lock on path [{}]", lockPath); } /** * Adds a {@link StartupJettyLifeCycleListener} in case the lock was never acquired. * - * @param lockInfo the lock info indicating if the lock was acquired - * @param environment the Dropwizard environment used to add the listener + * @param lockInfo the lock info indicating if the lock was acquired + * @param environment the Dropwizard environment used to add the listener */ public void addFallbackJettyStartupLifeCycleListener(StartupLockInfo lockInfo, Environment environment) { - if (isNull(lockInfo)) { + if (lockInfo.getLockState() != StartupLockInfo.LockState.ACQUIRED) { environment.lifecycle().addLifeCycleListener(new StartupJettyLifeCycleListener(executioner)); } } @@ -127,7 +142,7 @@ public void addFallbackJettyStartupLifeCycleListener(StartupLockInfo lockInfo, E * @param lockInfo the lock info indicating if the lock was acquired */ public void releaseStartupLockIfPresent(StartupLockInfo lockInfo) { - if (nonNull(lockInfo)) { + if (lockInfo.getLockState() == StartupLockInfo.LockState.ACQUIRED) { LOG.warn("Due to exception caught running app [{}], early releasing lock [{}] on path [{}]", this, lockInfo.lock, lockInfo.lockPath); curatorLockHelper.releaseLockQuietlyIfHeld(lockInfo.lock); curatorFrameworkHelper.closeIfStarted(lockInfo.client); diff --git a/src/main/java/org/kiwiproject/dropwizard/util/startup/listener/StartupJettyLifeCycleListener.java b/src/main/java/org/kiwiproject/dropwizard/util/startup/listener/StartupJettyLifeCycleListener.java index 16e7db11..944b08e8 100644 --- a/src/main/java/org/kiwiproject/dropwizard/util/startup/listener/StartupJettyLifeCycleListener.java +++ b/src/main/java/org/kiwiproject/dropwizard/util/startup/listener/StartupJettyLifeCycleListener.java @@ -1,5 +1,7 @@ package org.kiwiproject.dropwizard.util.startup.listener; +import static org.kiwiproject.base.KiwiPreconditions.requireNotNull; + import lombok.extern.slf4j.Slf4j; import org.eclipse.jetty.util.component.AbstractLifeCycle; import org.eclipse.jetty.util.component.LifeCycle; @@ -15,7 +17,7 @@ public class StartupJettyLifeCycleListener extends AbstractLifeCycle.AbstractLif private final SystemExecutioner executioner; public StartupJettyLifeCycleListener(SystemExecutioner executioner) { - this.executioner = executioner; + this.executioner = requireNotNull(executioner); } @Override diff --git a/src/main/java/org/kiwiproject/dropwizard/util/startup/listener/StartupWithLockJettyLifeCycleListener.java b/src/main/java/org/kiwiproject/dropwizard/util/startup/listener/StartupWithLockJettyLifeCycleListener.java index 936e83a7..119447ac 100644 --- a/src/main/java/org/kiwiproject/dropwizard/util/startup/listener/StartupWithLockJettyLifeCycleListener.java +++ b/src/main/java/org/kiwiproject/dropwizard/util/startup/listener/StartupWithLockJettyLifeCycleListener.java @@ -1,5 +1,8 @@ package org.kiwiproject.dropwizard.util.startup.listener; +import static org.kiwiproject.base.KiwiPreconditions.requireNotBlank; +import static org.kiwiproject.base.KiwiPreconditions.requireNotNull; + import lombok.extern.slf4j.Slf4j; import org.apache.curator.framework.CuratorFramework; import org.apache.curator.framework.recipes.locks.InterProcessLock; @@ -24,10 +27,10 @@ public class StartupWithLockJettyLifeCycleListener extends AbstractLifeCycle.Abs private final SystemExecutioner executioner; public StartupWithLockJettyLifeCycleListener(CuratorFramework curatorFramework, InterProcessLock lock, String lockPath, SystemExecutioner executioner) { - this.curatorFramework = curatorFramework; - this.lock = lock; - this.lockPath = lockPath; - this.executioner = executioner; + this.curatorFramework = requireNotNull(curatorFramework); + this.lock = requireNotNull(lock); + this.lockPath = requireNotBlank(lockPath); + this.executioner = requireNotNull(executioner); } @Override diff --git a/src/test/java/org/kiwiproject/dropwizard/util/startup/StartupLockerTest.java b/src/test/java/org/kiwiproject/dropwizard/util/startup/StartupLockerTest.java index cb685455..bc06b371 100644 --- a/src/test/java/org/kiwiproject/dropwizard/util/startup/StartupLockerTest.java +++ b/src/test/java/org/kiwiproject/dropwizard/util/startup/StartupLockerTest.java @@ -1,7 +1,6 @@ package org.kiwiproject.dropwizard.util.startup; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.kiwiproject.dropwizard.util.startup.PortAssigner.PortAssignment.DYNAMIC; import static org.kiwiproject.dropwizard.util.startup.PortAssigner.PortAssignment.STATIC; import static org.mockito.ArgumentMatchers.any; @@ -25,6 +24,8 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.kiwiproject.curator.CuratorFrameworkHelper; import org.kiwiproject.curator.CuratorLockHelper; import org.kiwiproject.curator.config.CuratorConfig; @@ -49,37 +50,25 @@ class StartupLockerTest { class Builder { @Test - void shouldRequireSystemExecutioner() { - var builder = StartupLocker.builder(); - - assertThatThrownBy(builder::build) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("SystemExecutioner is required"); + void shouldDefaultSystemExecutioner() { + var builder = StartupLocker.builder().build(); + assertThat(builder.getExecutioner()).isNotNull(); } @Test void shouldDefaultZooKeeperAvailabilityChecker() { - var builder = StartupLocker.builder() - .executioner(new SystemExecutioner()) - .build(); - + var builder = StartupLocker.builder().build(); assertThat(builder.getZkAvailabilityChecker()).isNotNull(); } @Test void shouldDefaultCuratorFrameworkHelper() { - var builder = StartupLocker.builder() - .executioner(new SystemExecutioner()) - .build(); - + var builder = StartupLocker.builder().build(); assertThat(builder.getCuratorFrameworkHelper()).isNotNull(); } @Test void shouldDefaultCuratorLockHelper() { - var builder = StartupLocker.builder() - .executioner(new SystemExecutioner()) - .build(); - + var builder = StartupLocker.builder().build(); assertThat(builder.getCuratorLockHelper()).isNotNull(); } @@ -87,6 +76,7 @@ void shouldDefaultCuratorLockHelper() { @Nested class AcquireStartupLock { + // NOTE: This is not static because there isn't a way to reset the lifecycle listeners for each test final DropwizardClientExtension CLIENT_EXTENSION = new DropwizardClientExtension(); private StartupLocker.StartupLockerBuilder startupLockerBuilder; @@ -107,7 +97,12 @@ void shouldNotAttemptToAcquireLock_WhenUsingStaticPorts() { var lockInfo = locker.acquireStartupLock(LOCK_PATH, Duration.milliseconds(100), STATIC, new CuratorConfig(), mock(Environment.class)); - assertThat(lockInfo).isNull(); + assertThat(lockInfo.getLockState()).isEqualTo(StartupLocker.StartupLockInfo.LockState.NOT_ATTEMPTED); + assertThat(lockInfo.getInfoMessage()).isEqualTo("Using static port assignment. Lock not needed."); + assertThat(lockInfo.getLock()).isNull(); + assertThat(lockInfo.getLockPath()).isBlank(); + assertThat(lockInfo.getException()).isNull(); + assertThat(lockInfo.getClient()).isNull(); } @Test @@ -117,22 +112,34 @@ void shouldNotAttemptToAcquireLock_WhenUnableToConnectToZooKeeper() { var lockInfo = locker.acquireStartupLock(LOCK_PATH, Duration.milliseconds(100), DYNAMIC, new CuratorConfig(), mock(Environment.class)); - assertThat(lockInfo).isNull(); + assertThat(lockInfo.getLockState()).isEqualTo(StartupLocker.StartupLockInfo.LockState.NOT_ATTEMPTED); + assertThat(lockInfo.getInfoMessage()).startsWith("No ZooKeepers are available from connect string "); + assertThat(lockInfo.getLock()).isNull(); + assertThat(lockInfo.getLockPath()).isBlank(); + assertThat(lockInfo.getException()).isNull(); + assertThat(lockInfo.getClient()).isNull(); } @Test - void shouldAttemptToAcquireLock_AndReturnNull_WhenLockIsNotAcquired() { + void shouldAttemptToAcquireLock_AndReturnFailStatus_WhenLockIsNotAcquired() { var curatorLockHelper = mock(CuratorLockHelper.class); var locker = startupLockerBuilder.curatorLockHelper(curatorLockHelper).build(); var lock = mock(InterProcessMutex.class); when(curatorLockHelper.createInterProcessMutex(any(CuratorFramework.class), eq(LOCK_PATH))).thenReturn(lock); - doThrow(new LockAcquisitionException("Oops")).when(curatorLockHelper).acquire(any(InterProcessMutex.class), eq(100L), eq(TimeUnit.MILLISECONDS)); + + var exception = new LockAcquisitionException("Oops"); + doThrow(exception).when(curatorLockHelper).acquire(any(InterProcessMutex.class), eq(100L), eq(TimeUnit.MILLISECONDS)); var curatorConfig = CuratorConfig.copyOfWithZkConnectString(new CuratorConfig(), ZK_TEST_SERVER.getConnectString()); var lockInfo = locker.acquireStartupLock(LOCK_PATH, Duration.milliseconds(100), DYNAMIC, curatorConfig, mock(Environment.class)); - assertThat(lockInfo).isNull(); + assertThat(lockInfo.getLockState()).isEqualTo(StartupLocker.StartupLockInfo.LockState.ACQUIRE_FAIL); + assertThat(lockInfo.getInfoMessage()).isEqualTo("Failed to obtain startup lock"); + assertThat(lockInfo.getLock()).isNull(); + assertThat(lockInfo.getLockPath()).isBlank(); + assertThat(lockInfo.getException()).isSameAs(exception); + assertThat(lockInfo.getClient()).isNull(); } @Test @@ -142,8 +149,12 @@ void shouldAttemptAndSucceedInAcquiringLock() { var lockInfo = locker.acquireStartupLock(LOCK_PATH, Duration.milliseconds(100), DYNAMIC, curatorConfig, CLIENT_EXTENSION.getEnvironment()); - assertThat(lockInfo).isNotNull(); - assertThat(lockInfo.lockPath).isEqualTo(LOCK_PATH); + assertThat(lockInfo.getLockState()).isEqualTo(StartupLocker.StartupLockInfo.LockState.ACQUIRED); + assertThat(lockInfo.getInfoMessage()).isEqualTo("Lock acquired"); + assertThat(lockInfo.getLock()).isNotNull(); + assertThat(lockInfo.getLockPath()).isEqualTo(LOCK_PATH); + assertThat(lockInfo.getException()).isNull(); + assertThat(lockInfo.getClient()).isNotNull(); var listeners = DropwizardAppTests.lifeCycleListenersOf(CLIENT_EXTENSION.getEnvironment().lifecycle()); assertThat(listeners).hasAtLeastOneElementOfType(StartupWithLockJettyLifeCycleListener.class); @@ -153,26 +164,38 @@ void shouldAttemptAndSucceedInAcquiringLock() { @Nested class AddFallbackJettyStartupLifeCycleListener { + // NOTE: This is not static because there isn't a way to reset the lifecycle listeners for each test final DropwizardClientExtension CLIENT_EXTENSION = new DropwizardClientExtension(); @Test - void shouldNotAddListener_WhenLockInfoIsNotNull() { + void shouldNotAddListener_WhenLockInfoIsAcquired() { var locker = StartupLocker.builder().executioner(mock(SystemExecutioner.class)).build(); var curatorClient = mock(CuratorFramework.class); var lock = mock(InterProcessLock.class); - var lockInfo = new StartupLocker.StartupLockInfo(curatorClient, lock, LOCK_PATH); + var lockInfo = StartupLocker.StartupLockInfo.builder() + .client(curatorClient) + .lock(lock) + .lockPath(LOCK_PATH) + .lockState(StartupLocker.StartupLockInfo.LockState.ACQUIRED) + .build(); + locker.addFallbackJettyStartupLifeCycleListener(lockInfo, CLIENT_EXTENSION.getEnvironment()); var listeners = DropwizardAppTests.lifeCycleListenersOf(CLIENT_EXTENSION.getEnvironment().lifecycle()); assertThat(listeners).doesNotHaveAnyElementsOfTypes(StartupJettyLifeCycleListener.class); } - @Test - void shouldAddListener_WhenLockInfoIsNull() { + @ParameterizedTest + @ValueSource(strings = { "NOT_ATTEMPTED", "ACQUIRE_FAIL" }) + void shouldAddListener_WhenLockInfoNotAcquired(String state) { var locker = StartupLocker.builder().executioner(mock(SystemExecutioner.class)).build(); - locker.addFallbackJettyStartupLifeCycleListener(null, CLIENT_EXTENSION.getEnvironment()); + var info = StartupLocker.StartupLockInfo.builder() + .lockState(StartupLocker.StartupLockInfo.LockState.valueOf(state)) + .build(); + + locker.addFallbackJettyStartupLifeCycleListener(info, CLIENT_EXTENSION.getEnvironment()); var listeners = DropwizardAppTests.lifeCycleListenersOf(CLIENT_EXTENSION.getEnvironment().lifecycle()); assertThat(listeners).hasAtLeastOneElementOfType(StartupJettyLifeCycleListener.class); @@ -198,10 +221,15 @@ void setUp() { } @Test - void shouldCleanupLock_WhenInfoIsNotNull() { + void shouldCleanupLock_WhenLockIsAcquired() { var curatorClient = mock(CuratorFramework.class); var lock = mock(InterProcessLock.class); - var lockInfo = new StartupLocker.StartupLockInfo(curatorClient, lock, LOCK_PATH); + var lockInfo = StartupLocker.StartupLockInfo.builder() + .client(curatorClient) + .lock(lock) + .lockPath(LOCK_PATH) + .lockState(StartupLocker.StartupLockInfo.LockState.ACQUIRED) + .build(); locker.releaseStartupLockIfPresent(lockInfo); @@ -209,9 +237,14 @@ void shouldCleanupLock_WhenInfoIsNotNull() { verify(curatorFrameworkHelper).closeIfStarted(curatorClient); } - @Test - void shouldNotDoAnything_WhenInfoIsNull() { - locker.releaseStartupLockIfPresent(null); + @ParameterizedTest + @ValueSource(strings = { "NOT_ATTEMPTED", "ACQUIRE_FAIL" }) + void shouldNotDoAnything_WhenLockIsNotAcquired(String state) { + var info = StartupLocker.StartupLockInfo.builder() + .lockState(StartupLocker.StartupLockInfo.LockState.valueOf(state)) + .build(); + + locker.releaseStartupLockIfPresent(info); verifyNoInteractions(curatorLockHelper, curatorFrameworkHelper); }