From 85d1addd04151318bd624ae8b07fb3f072f87669 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jun 2022 17:16:02 +0100 Subject: [PATCH 1/4] Continue shutdown if an exception is thrown. --- .../lifecycle/LifecycleManagerImpl.java | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) 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 b55115fb7..a1ca7dd6a 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 @@ -198,11 +198,15 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { state = STOPPING; eventBus.broadcast(new LifecycleEvent(STOPPING)); for (Service s : services) { - long start = now(); - s.stopService(); - if (LOG.isLoggable(FINE)) { - logDuration(LOG, "Stopping service " - + s.getClass().getSimpleName(), start); + 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) { @@ -212,12 +216,14 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { } e.shutdownNow(); } - long start = now(); - db.close(); - logDuration(LOG, "Closing database", start); + try { + long start = now(); + db.close(); + logDuration(LOG, "Closing database", start); + } catch (DbException e) { + logException(LOG, WARNING, e); + } shutdownLatch.countDown(); - } catch (DbException | ServiceException e) { - logException(LOG, WARNING, e); } finally { startStopSemaphore.release(); } From de3a87fff5a0a7b9240952bf8b25b7c8840cee01 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jun 2022 18:01:32 +0100 Subject: [PATCH 2/4] 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()); } } From 825dff27fc23402f6c25ce059f6f777d46f021d8 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jun 2022 18:06:08 +0100 Subject: [PATCH 3/4] Exit if BriarService finds lifecycle already running. --- .../java/org/briarproject/briar/android/BriarService.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/BriarService.java b/briar-android/src/main/java/org/briarproject/briar/android/BriarService.java index 70be9f0e9..d114d107b 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/BriarService.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/BriarService.java @@ -156,8 +156,11 @@ public class BriarService extends Service { if (result == SUCCESS) { started = true; } else if (result == ALREADY_RUNNING) { - LOG.info("Already running"); - stopSelf(); + LOG.warning("Already running"); + // The LifecycleManager has outlived the BriarService + // instance that created it. Rather than trying to recover + // from this unexpected state, try to exit cleanly + shutdownFromBackground(); } else { if (LOG.isLoggable(WARNING)) LOG.warning("Startup failed: " + result); From e481a02126854cea654bf6d12bd3dffcf3f37d03 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 9 Jun 2022 18:10:24 +0100 Subject: [PATCH 4/4] Shutdown from background if BriarService is recreated. --- .../java/org/briarproject/briar/android/BriarService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/BriarService.java b/briar-android/src/main/java/org/briarproject/briar/android/BriarService.java index d114d107b..58e9adf68 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/BriarService.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/BriarService.java @@ -157,9 +157,9 @@ public class BriarService extends Service { started = true; } else if (result == ALREADY_RUNNING) { LOG.warning("Already running"); - // The LifecycleManager has outlived the BriarService - // instance that created it. Rather than trying to recover - // from this unexpected state, try to exit cleanly + // The core has outlived the original BriarService + // instance. We don't know how to recover from this + // unexpected state, so try to exit cleanly shutdownFromBackground(); } else { if (LOG.isLoggable(WARNING))