From 148f61a6b579e703db3cc61e5452b641be64c908 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 10 Aug 2022 14:34:09 +0100 Subject: [PATCH] Check our own mailbox periodically while we're online. --- .../mailbox/MailboxClientFactoryImpl.java | 12 +- .../bramble/mailbox/OwnMailboxClient.java | 66 ++++++++++- .../bramble/mailbox/OwnMailboxClientTest.java | 103 +++++++++++++++++- 3 files changed, 178 insertions(+), 3 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClientFactoryImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClientFactoryImpl.java index 3e8ab1930..4e6e11580 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClientFactoryImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClientFactoryImpl.java @@ -1,7 +1,11 @@ package org.briarproject.bramble.mailbox; +import org.briarproject.bramble.api.lifecycle.IoExecutor; import org.briarproject.bramble.api.mailbox.MailboxProperties; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.system.TaskScheduler; + +import java.util.concurrent.Executor; import javax.inject.Inject; import javax.inject.Provider; @@ -10,15 +14,21 @@ import javax.inject.Provider; class MailboxClientFactoryImpl implements MailboxClientFactory { private final MailboxWorkerFactory workerFactory; + private final TaskScheduler taskScheduler; + private final Executor ioExecutor; private final Provider contactCheckerProvider; private final Provider ownCheckerProvider; @Inject MailboxClientFactoryImpl(MailboxWorkerFactory workerFactory, + TaskScheduler taskScheduler, + @IoExecutor Executor ioExecutor, Provider contactCheckerProvider, Provider ownCheckerProvider) { this.workerFactory = workerFactory; + this.taskScheduler = taskScheduler; + this.ioExecutor = ioExecutor; this.contactCheckerProvider = contactCheckerProvider; this.ownCheckerProvider = ownCheckerProvider; } @@ -37,6 +47,6 @@ class MailboxClientFactoryImpl implements MailboxClientFactory { MailboxProperties properties) { ConnectivityChecker connectivityChecker = ownCheckerProvider.get(); return new OwnMailboxClient(workerFactory, connectivityChecker, - reachabilityMonitor, properties); + reachabilityMonitor, taskScheduler, ioExecutor, properties); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxClient.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxClient.java index f85adeb1f..24b7b2760 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxClient.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxClient.java @@ -1,9 +1,13 @@ package org.briarproject.bramble.mailbox; +import org.briarproject.bramble.api.Cancellable; import org.briarproject.bramble.api.contact.ContactId; +import org.briarproject.bramble.api.lifecycle.IoExecutor; import org.briarproject.bramble.api.mailbox.MailboxFolderId; import org.briarproject.bramble.api.mailbox.MailboxProperties; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.system.TaskScheduler; +import org.briarproject.bramble.mailbox.ConnectivityChecker.ConnectivityObserver; import java.util.ArrayList; import java.util.HashMap; @@ -11,17 +15,27 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.Executor; import java.util.logging.Logger; import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; +import static java.util.concurrent.TimeUnit.HOURS; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.logging.Logger.getLogger; @ThreadSafe @NotNullByDefault -class OwnMailboxClient implements MailboxClient { +class OwnMailboxClient implements MailboxClient, ConnectivityObserver { + + /** + * How often to check our own mailbox's connectivity. + *

+ * Package access for testing. + */ + static final long CONNECTIVITY_CHECK_INTERVAL_MS = HOURS.toMillis(1); private static final Logger LOG = getLogger(OwnMailboxClient.class.getName()); @@ -29,6 +43,9 @@ class OwnMailboxClient implements MailboxClient { private final MailboxWorkerFactory workerFactory; private final ConnectivityChecker connectivityChecker; private final TorReachabilityMonitor reachabilityMonitor; + private final TaskScheduler taskScheduler; + private final Executor ioExecutor; + private final MailboxProperties properties; private final MailboxWorker contactListWorker; private final Object lock = new Object(); @@ -53,14 +70,30 @@ class OwnMailboxClient implements MailboxClient { @GuardedBy("lock") private final Set assignedForDownload = new HashSet<>(); + /** + * Scheduled task for periodically checking whether the mailbox is + * reachable. + */ + @GuardedBy("lock") + @Nullable + private Cancellable connectivityTask = null; + + @GuardedBy("lock") + private boolean destroyed = false; + OwnMailboxClient(MailboxWorkerFactory workerFactory, ConnectivityChecker connectivityChecker, TorReachabilityMonitor reachabilityMonitor, + TaskScheduler taskScheduler, + @IoExecutor Executor ioExecutor, MailboxProperties properties) { if (!properties.isOwner()) throw new IllegalArgumentException(); this.workerFactory = workerFactory; this.connectivityChecker = connectivityChecker; this.reachabilityMonitor = reachabilityMonitor; + this.taskScheduler = taskScheduler; + this.ioExecutor = ioExecutor; + this.properties = properties; contactListWorker = workerFactory.createContactListWorkerForOwnMailbox( connectivityChecker, properties); } @@ -68,6 +101,7 @@ class OwnMailboxClient implements MailboxClient { @Override public void start() { LOG.info("Started"); + checkConnectivity(); contactListWorker.start(); } @@ -76,15 +110,21 @@ class OwnMailboxClient implements MailboxClient { LOG.info("Destroyed"); List uploadWorkers; MailboxWorker downloadWorker; + Cancellable connectivityTask; synchronized (lock) { uploadWorkers = new ArrayList<>(this.uploadWorkers.values()); this.uploadWorkers.clear(); downloadWorker = this.downloadWorker; this.downloadWorker = null; + connectivityTask = this.connectivityTask; + this.connectivityTask = null; + destroyed = true; } // Destroy the workers (with apologies to Mr Marx and Mr Engels) for (MailboxWorker worker : uploadWorkers) worker.destroy(); if (downloadWorker != null) downloadWorker.destroy(); + // If a connectivity check is scheduled, cancel it + if (connectivityTask != null) connectivityTask.cancel(); contactListWorker.destroy(); // The connectivity checker belongs to the client, so it should be // destroyed. The Tor reachability monitor is shared between clients, @@ -155,4 +195,28 @@ class OwnMailboxClient implements MailboxClient { } if (toDestroy != null) toDestroy.destroy(); } + + private void checkConnectivity() { + synchronized (lock) { + if (destroyed) return; + connectivityTask = null; + } + connectivityChecker.checkConnectivity(properties, this); + // Avoid leaking observer in case destroy() is called concurrently + // before observer is added + boolean removeObserver; + synchronized (lock) { + removeObserver = destroyed; + } + if (removeObserver) connectivityChecker.removeObserver(this); + } + + @Override + public void onConnectivityCheckSucceeded() { + synchronized (lock) { + if (destroyed) return; + connectivityTask = taskScheduler.schedule(this::checkConnectivity, + ioExecutor, CONNECTIVITY_CHECK_INTERVAL_MS, MILLISECONDS); + } + } } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxClientTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxClientTest.java index 37705c32d..3b62dbb1d 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxClientTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxClientTest.java @@ -1,13 +1,22 @@ package org.briarproject.bramble.mailbox; +import org.briarproject.bramble.api.Cancellable; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.mailbox.MailboxFolderId; import org.briarproject.bramble.api.mailbox.MailboxProperties; +import org.briarproject.bramble.api.system.TaskScheduler; import org.briarproject.bramble.test.BrambleMockTestCase; +import org.briarproject.bramble.test.CaptureArgumentAction; import org.jmock.Expectations; +import org.jmock.lib.action.DoAllAction; import org.junit.Test; +import java.util.concurrent.Executor; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + import static org.briarproject.bramble.api.mailbox.MailboxConstants.CLIENT_SUPPORTS; +import static org.briarproject.bramble.mailbox.OwnMailboxClient.CONNECTIVITY_CHECK_INTERVAL_MS; import static org.briarproject.bramble.test.TestUtils.getContactId; import static org.briarproject.bramble.test.TestUtils.getMailboxProperties; import static org.briarproject.bramble.test.TestUtils.getRandomId; @@ -20,6 +29,9 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { context.mock(ConnectivityChecker.class); private final TorReachabilityMonitor reachabilityMonitor = context.mock(TorReachabilityMonitor.class); + private final TaskScheduler taskScheduler = + context.mock(TaskScheduler.class); + private final Executor ioExecutor = context.mock(Executor.class); private final MailboxWorker contactListWorker = context.mock(MailboxWorker.class, "contactListWorker"); private final MailboxWorker uploadWorker1 = @@ -28,6 +40,8 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { context.mock(MailboxWorker.class, "uploadWorker2"); private final MailboxWorker downloadWorker = context.mock(MailboxWorker.class, "downloadWorker"); + private final Cancellable connectivityCheck = + context.mock(Cancellable.class); private final MailboxProperties properties = getMailboxProperties(true, CLIENT_SUPPORTS); @@ -40,12 +54,13 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { public OwnMailboxClientTest() { expectCreateContactListWorker(); client = new OwnMailboxClient(workerFactory, connectivityChecker, - reachabilityMonitor, properties); + reachabilityMonitor, taskScheduler, ioExecutor, properties); context.assertIsSatisfied(); } @Test public void testStartAndDestroyWithNoContactsAssigned() { + expectStartConnectivityCheck(); expectStartWorker(contactListWorker); client.start(); @@ -56,6 +71,7 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { @Test public void testAssignContactForUploadAndDestroyClient() { + expectStartConnectivityCheck(); expectStartWorker(contactListWorker); client.start(); @@ -74,6 +90,7 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { @Test public void testAssignAndDeassignContactForUpload() { + expectStartConnectivityCheck(); expectStartWorker(contactListWorker); client.start(); @@ -95,6 +112,7 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { @Test public void testAssignAndDeassignTwoContactsForUpload() { + expectStartConnectivityCheck(); expectStartWorker(contactListWorker); client.start(); @@ -129,6 +147,7 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { @Test public void testAssignContactForDownloadAndDestroyClient() { + expectStartConnectivityCheck(); expectStartWorker(contactListWorker); client.start(); @@ -147,6 +166,7 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { @Test public void testAssignAndDeassignTwoContactsForDownload() { + expectStartConnectivityCheck(); expectStartWorker(contactListWorker); client.start(); @@ -175,6 +195,68 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { client.destroy(); } + @Test + public void testCancelsConnectivityCheckWhenClientIsDestroyed() { + expectStartConnectivityCheck(); + expectStartWorker(contactListWorker); + client.start(); + + // When the first connectivity check succeeds, the worker should + // schedule a second check + AtomicReference task = new AtomicReference<>(); + expectScheduleConnectivityCheck(task); + client.onConnectivityCheckSucceeded(); + + // When the task runs, the worker should check the mailbox's + // connectivity + expectStartConnectivityCheck(); + task.get().run(); + + // When the second connectivity check succeeds, the worker should + // schedule a third check + expectScheduleConnectivityCheck(task); + client.onConnectivityCheckSucceeded(); + + // When the client is destroyed, the scheduled check should be cancelled + expectDestroyWorker(contactListWorker); + expectDestroyConnectivityChecker(); + context.checking(new Expectations() {{ + oneOf(connectivityCheck).cancel(); + }}); + client.destroy(); + + // If the task runs anyway (cancellation came too late), it should + // return when it finds that the client has been destroyed + task.get().run(); + } + + @Test + public void testIgnoresConnectivityResultWhenClientIsDestroyed() { + expectStartConnectivityCheck(); + expectStartWorker(contactListWorker); + client.start(); + + // When the first connectivity check succeeds, the worker should + // schedule a second check + AtomicReference task = new AtomicReference<>(); + expectScheduleConnectivityCheck(task); + client.onConnectivityCheckSucceeded(); + + // When the task runs, the worker should check the mailbox's + // connectivity + expectStartConnectivityCheck(); + task.get().run(); + + // Before the connectivity check succeeds, the client is destroyed + expectDestroyWorker(contactListWorker); + expectDestroyConnectivityChecker(); + client.destroy(); + + // If the connectivity check succeeds despite the connectivity checker + // having been destroyed, the client should not schedule another check + client.onConnectivityCheckSucceeded(); + } + private void expectCreateContactListWorker() { context.checking(new Expectations() {{ oneOf(workerFactory).createContactListWorkerForOwnMailbox( @@ -206,6 +288,25 @@ public class OwnMailboxClientTest extends BrambleMockTestCase { }}); } + private void expectStartConnectivityCheck() { + context.checking(new Expectations() {{ + oneOf(connectivityChecker).checkConnectivity(properties, client); + }}); + } + + private void expectScheduleConnectivityCheck( + AtomicReference task) { + context.checking(new Expectations() {{ + oneOf(taskScheduler).schedule(with(any(Runnable.class)), + with(ioExecutor), with(CONNECTIVITY_CHECK_INTERVAL_MS), + with(TimeUnit.MILLISECONDS)); + will(new DoAllAction( + new CaptureArgumentAction<>(task, Runnable.class, 0), + returnValue(connectivityCheck) + )); + }}); + } + private void expectDestroyWorker(MailboxWorker worker) { context.checking(new Expectations() {{ oneOf(worker).destroy();