From de3a87fff5a0a7b9240952bf8b25b7c8840cee01 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jun 2022 18:01:32 +0100 Subject: [PATCH] Return early when starting/stopping if not in expected state. --- .../api/lifecycle/LifecycleManager.java | 10 ++- .../lifecycle/LifecycleManagerImpl.java | 90 +++++++++---------- .../lifecycle/LifecycleManagerImplTest.java | 78 +++++++++++++--- 3 files changed, 115 insertions(+), 63 deletions(-) diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/lifecycle/LifecycleManager.java b/bramble-api/src/main/java/org/briarproject/bramble/api/lifecycle/LifecycleManager.java index 12bc36248..1f5a2c156 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/lifecycle/LifecycleManager.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/lifecycle/LifecycleManager.java @@ -37,8 +37,14 @@ public interface LifecycleManager { */ enum LifecycleState { - STARTING, MIGRATING_DATABASE, COMPACTING_DATABASE, STARTING_SERVICES, - RUNNING, STOPPING; + CREATED, + STARTING, + MIGRATING_DATABASE, + COMPACTING_DATABASE, + STARTING_SERVICES, + RUNNING, + STOPPING, + STOPPED; public boolean isAfter(LifecycleState state) { return ordinal() > state.ordinal(); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java index a1ca7dd6a..bd2a35b99 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java @@ -18,7 +18,7 @@ import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Semaphore; +import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; import javax.annotation.concurrent.ThreadSafe; @@ -29,10 +29,12 @@ import static java.util.logging.Level.INFO; import static java.util.logging.Level.WARNING; import static java.util.logging.Logger.getLogger; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.COMPACTING_DATABASE; +import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.CREATED; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.MIGRATING_DATABASE; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.RUNNING; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.STARTING; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.STARTING_SERVICES; +import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.STOPPED; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.STOPPING; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.StartResult.ALREADY_RUNNING; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.StartResult.CLOCK_ERROR; @@ -60,12 +62,11 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { private final List services; private final List openDatabaseHooks; private final List executors; - private final Semaphore startStopSemaphore = new Semaphore(1); private final CountDownLatch dbLatch = new CountDownLatch(1); private final CountDownLatch startupLatch = new CountDownLatch(1); private final CountDownLatch shutdownLatch = new CountDownLatch(1); - - private volatile LifecycleState state = STARTING; + private final AtomicReference state = + new AtomicReference<>(CREATED); @Inject LifecycleManagerImpl(DatabaseComponent db, EventBus eventBus, @@ -102,8 +103,8 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { @Override public StartResult startServices(SecretKey dbKey) { - if (!startStopSemaphore.tryAcquire()) { - LOG.info("Already starting or stopping"); + if (!state.compareAndSet(CREATED, STARTING)) { + LOG.warning("Already running"); return ALREADY_RUNNING; } long now = clock.currentTimeMillis(); @@ -135,7 +136,7 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { }); LOG.info("Starting services"); - state = STARTING_SERVICES; + state.set(STARTING_SERVICES); dbLatch.countDown(); eventBus.broadcast(new LifecycleEvent(STARTING_SERVICES)); @@ -148,7 +149,7 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { } } - state = RUNNING; + state.set(RUNNING); startupLatch.countDown(); eventBus.broadcast(new LifecycleEvent(RUNNING)); return SUCCESS; @@ -164,69 +165,58 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { } catch (ServiceException e) { logException(LOG, WARNING, e); return SERVICE_ERROR; - } finally { - startStopSemaphore.release(); } } @Override public void onDatabaseMigration() { - state = MIGRATING_DATABASE; + state.set(MIGRATING_DATABASE); eventBus.broadcast(new LifecycleEvent(MIGRATING_DATABASE)); } @Override public void onDatabaseCompaction() { - state = COMPACTING_DATABASE; + state.set(COMPACTING_DATABASE); eventBus.broadcast(new LifecycleEvent(COMPACTING_DATABASE)); } @Override public void stopServices() { - try { - startStopSemaphore.acquire(); - } catch (InterruptedException e) { - LOG.warning("Interrupted while waiting to stop services"); + if (!state.compareAndSet(RUNNING, STOPPING)) { + LOG.warning("Not running"); return; } - try { - if (state == STOPPING) { - LOG.info("Already stopped"); - return; - } - LOG.info("Stopping services"); - state = STOPPING; - eventBus.broadcast(new LifecycleEvent(STOPPING)); - for (Service s : services) { - try { - long start = now(); - s.stopService(); - if (LOG.isLoggable(FINE)) { - logDuration(LOG, "Stopping service " - + s.getClass().getSimpleName(), start); - } - } catch (ServiceException e) { - logException(LOG, WARNING, e); - } - } - for (ExecutorService e : executors) { - if (LOG.isLoggable(FINE)) { - LOG.fine("Stopping executor " - + e.getClass().getSimpleName()); - } - e.shutdownNow(); - } + LOG.info("Stopping services"); + eventBus.broadcast(new LifecycleEvent(STOPPING)); + for (Service s : services) { try { long start = now(); - db.close(); - logDuration(LOG, "Closing database", start); - } catch (DbException e) { + s.stopService(); + if (LOG.isLoggable(FINE)) { + logDuration(LOG, "Stopping service " + + s.getClass().getSimpleName(), start); + } + } catch (ServiceException e) { logException(LOG, WARNING, e); } - shutdownLatch.countDown(); - } finally { - startStopSemaphore.release(); } + for (ExecutorService e : executors) { + if (LOG.isLoggable(FINE)) { + LOG.fine("Stopping executor " + + e.getClass().getSimpleName()); + } + e.shutdownNow(); + } + try { + long start = now(); + db.close(); + logDuration(LOG, "Closing database", start); + } catch (DbException e) { + logException(LOG, WARNING, e); + } + state.set(STOPPED); + shutdownLatch.countDown(); + eventBus.broadcast(new LifecycleEvent(STOPPED)); } @Override @@ -246,6 +236,6 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { @Override public LifecycleState getLifecycleState() { - return state; + return state.get(); } } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/lifecycle/LifecycleManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/lifecycle/LifecycleManagerImplTest.java index e6c7ac708..49a8e3610 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/lifecycle/LifecycleManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/lifecycle/LifecycleManagerImplTest.java @@ -5,6 +5,7 @@ import org.briarproject.bramble.api.db.DatabaseComponent; import org.briarproject.bramble.api.db.Transaction; import org.briarproject.bramble.api.event.EventBus; import org.briarproject.bramble.api.lifecycle.LifecycleManager.OpenDatabaseHook; +import org.briarproject.bramble.api.lifecycle.Service; import org.briarproject.bramble.api.lifecycle.event.LifecycleEvent; import org.briarproject.bramble.api.system.Clock; import org.briarproject.bramble.test.BrambleMockTestCase; @@ -13,12 +14,10 @@ import org.jmock.Expectations; import org.junit.Before; import org.junit.Test; -import java.util.concurrent.atomic.AtomicBoolean; - -import static junit.framework.TestCase.assertTrue; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.RUNNING; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.STARTING; -import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.STOPPING; +import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.STOPPED; +import static org.briarproject.bramble.api.lifecycle.LifecycleManager.StartResult.ALREADY_RUNNING; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.StartResult.CLOCK_ERROR; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.StartResult.SUCCESS; import static org.briarproject.bramble.api.system.Clock.MAX_REASONABLE_TIME_MS; @@ -31,6 +30,8 @@ public class LifecycleManagerImplTest extends BrambleMockTestCase { private final DatabaseComponent db = context.mock(DatabaseComponent.class); private final EventBus eventBus = context.mock(EventBus.class); private final Clock clock = context.mock(Clock.class); + private final OpenDatabaseHook hook = context.mock(OpenDatabaseHook.class); + private final Service service = context.mock(Service.class); private final SecretKey dbKey = getSecretKey(); @@ -45,8 +46,6 @@ public class LifecycleManagerImplTest extends BrambleMockTestCase { public void testOpenDatabaseHooksAreCalledAtStartup() throws Exception { long now = System.currentTimeMillis(); Transaction txn = new Transaction(null, false); - AtomicBoolean called = new AtomicBoolean(false); - OpenDatabaseHook hook = transaction -> called.set(true); context.checking(new DbExpectations() {{ oneOf(clock).currentTimeMillis(); @@ -55,6 +54,7 @@ public class LifecycleManagerImplTest extends BrambleMockTestCase { will(returnValue(false)); oneOf(db).transaction(with(false), withDbRunnable(txn)); oneOf(db).removeTemporaryMessages(txn); + oneOf(hook).onDatabaseOpened(txn); allowing(eventBus).broadcast(with(any(LifecycleEvent.class))); }}); @@ -62,7 +62,38 @@ public class LifecycleManagerImplTest extends BrambleMockTestCase { assertEquals(SUCCESS, lifecycleManager.startServices(dbKey)); assertEquals(RUNNING, lifecycleManager.getLifecycleState()); - assertTrue(called.get()); + } + + @Test + public void testServicesAreStartedAndStopped() throws Exception { + long now = System.currentTimeMillis(); + Transaction txn = new Transaction(null, false); + + context.checking(new DbExpectations() {{ + oneOf(clock).currentTimeMillis(); + will(returnValue(now)); + oneOf(db).open(dbKey, lifecycleManager); + will(returnValue(false)); + oneOf(db).transaction(with(false), withDbRunnable(txn)); + oneOf(db).removeTemporaryMessages(txn); + oneOf(service).startService(); + allowing(eventBus).broadcast(with(any(LifecycleEvent.class))); + }}); + + lifecycleManager.registerService(service); + + assertEquals(SUCCESS, lifecycleManager.startServices(dbKey)); + assertEquals(RUNNING, lifecycleManager.getLifecycleState()); + context.assertIsSatisfied(); + + context.checking(new Expectations() {{ + oneOf(db).close(); + oneOf(service).stopService(); + allowing(eventBus).broadcast(with(any(LifecycleEvent.class))); + }}); + + lifecycleManager.stopServices(); + assertEquals(STOPPED, lifecycleManager.getLifecycleState()); } @Test @@ -89,6 +120,31 @@ public class LifecycleManagerImplTest extends BrambleMockTestCase { assertEquals(STARTING, lifecycleManager.getLifecycleState()); } + @Test + public void testSecondCallToStartServicesReturnsEarly() throws Exception { + long now = System.currentTimeMillis(); + Transaction txn = new Transaction(null, false); + + context.checking(new DbExpectations() {{ + oneOf(clock).currentTimeMillis(); + will(returnValue(now)); + oneOf(db).open(dbKey, lifecycleManager); + will(returnValue(false)); + oneOf(db).transaction(with(false), withDbRunnable(txn)); + oneOf(db).removeTemporaryMessages(txn); + allowing(eventBus).broadcast(with(any(LifecycleEvent.class))); + }}); + + assertEquals(SUCCESS, lifecycleManager.startServices(dbKey)); + assertEquals(RUNNING, lifecycleManager.getLifecycleState()); + context.assertIsSatisfied(); + + // Calling startServices() again should not try to open the DB or + // start the services again + assertEquals(ALREADY_RUNNING, lifecycleManager.startServices(dbKey)); + assertEquals(RUNNING, lifecycleManager.getLifecycleState()); + } + @Test public void testSecondCallToStopServicesReturnsEarly() throws Exception { long now = System.currentTimeMillis(); @@ -101,7 +157,7 @@ public class LifecycleManagerImplTest extends BrambleMockTestCase { will(returnValue(false)); oneOf(db).transaction(with(false), withDbRunnable(txn)); oneOf(db).removeTemporaryMessages(txn); - exactly(2).of(eventBus).broadcast(with(any(LifecycleEvent.class))); + allowing(eventBus).broadcast(with(any(LifecycleEvent.class))); }}); assertEquals(SUCCESS, lifecycleManager.startServices(dbKey)); @@ -109,17 +165,17 @@ public class LifecycleManagerImplTest extends BrambleMockTestCase { context.assertIsSatisfied(); context.checking(new Expectations() {{ - oneOf(eventBus).broadcast(with(any(LifecycleEvent.class))); oneOf(db).close(); + allowing(eventBus).broadcast(with(any(LifecycleEvent.class))); }}); lifecycleManager.stopServices(); - assertEquals(STOPPING, lifecycleManager.getLifecycleState()); + assertEquals(STOPPED, lifecycleManager.getLifecycleState()); context.assertIsSatisfied(); // Calling stopServices() again should not broadcast another event or // try to close the DB again lifecycleManager.stopServices(); - assertEquals(STOPPING, lifecycleManager.getLifecycleState()); + assertEquals(STOPPED, lifecycleManager.getLifecycleState()); } }