From 8982964fbff4dba7c001aa85d6c8cf71c17e6e8a Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 15 Jul 2022 18:00:13 +0100 Subject: [PATCH 1/2] Add mailbox client for our own mailbox. --- .../bramble/mailbox/ContactMailboxClient.java | 2 - .../bramble/mailbox/MailboxWorkerFactory.java | 4 + .../mailbox/MailboxWorkerFactoryImpl.java | 20 +- .../bramble/mailbox/OwnMailboxClient.java | 139 ++++++++++++ .../mailbox/ContactMailboxClientTest.java | 4 +- .../bramble/mailbox/OwnMailboxClientTest.java | 208 ++++++++++++++++++ 6 files changed, 370 insertions(+), 7 deletions(-) create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxClient.java create mode 100644 bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxClientTest.java diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxClient.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxClient.java index c27fe977a..8b419bdef 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxClient.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxClient.java @@ -10,7 +10,6 @@ import java.util.logging.Logger; import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; -import javax.inject.Inject; import static java.util.logging.Logger.getLogger; @@ -30,7 +29,6 @@ class ContactMailboxClient implements MailboxClient { @Nullable private MailboxWorker uploadWorker = null, downloadWorker = null; - @Inject ContactMailboxClient(MailboxWorkerFactory workerFactory, ConnectivityChecker connectivityChecker, TorReachabilityMonitor reachabilityMonitor) { diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorkerFactory.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorkerFactory.java index 2f6f827e8..dc36443d0 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorkerFactory.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorkerFactory.java @@ -24,4 +24,8 @@ interface MailboxWorkerFactory { ConnectivityChecker connectivityChecker, TorReachabilityMonitor reachabilityMonitor, MailboxProperties properties); + + MailboxWorker createContactListWorkerForOwnMailbox( + ConnectivityChecker connectivityChecker, + MailboxProperties properties); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorkerFactoryImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorkerFactoryImpl.java index 950cfd749..34a3cab74 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorkerFactoryImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorkerFactoryImpl.java @@ -6,6 +6,7 @@ import org.briarproject.bramble.api.event.EventBus; 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.mailbox.MailboxUpdateManager; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.system.Clock; import org.briarproject.bramble.api.system.TaskScheduler; @@ -27,6 +28,7 @@ class MailboxWorkerFactoryImpl implements MailboxWorkerFactory { private final MailboxApiCaller mailboxApiCaller; private final MailboxApi mailboxApi; private final MailboxFileManager mailboxFileManager; + private final MailboxUpdateManager mailboxUpdateManager; @Inject MailboxWorkerFactoryImpl(@IoExecutor Executor ioExecutor, @@ -36,7 +38,8 @@ class MailboxWorkerFactoryImpl implements MailboxWorkerFactory { EventBus eventBus, MailboxApiCaller mailboxApiCaller, MailboxApi mailboxApi, - MailboxFileManager mailboxFileManager) { + MailboxFileManager mailboxFileManager, + MailboxUpdateManager mailboxUpdateManager) { this.ioExecutor = ioExecutor; this.db = db; this.clock = clock; @@ -45,6 +48,7 @@ class MailboxWorkerFactoryImpl implements MailboxWorkerFactory { this.mailboxApiCaller = mailboxApiCaller; this.mailboxApi = mailboxApi; this.mailboxFileManager = mailboxFileManager; + this.mailboxUpdateManager = mailboxUpdateManager; } @Override @@ -75,7 +79,17 @@ class MailboxWorkerFactoryImpl implements MailboxWorkerFactory { ConnectivityChecker connectivityChecker, TorReachabilityMonitor reachabilityMonitor, MailboxProperties properties) { - // TODO - throw new UnsupportedOperationException(); + return new OwnMailboxDownloadWorker(connectivityChecker, + reachabilityMonitor, mailboxApiCaller, mailboxApi, + mailboxFileManager, properties); + } + + @Override + public MailboxWorker createContactListWorkerForOwnMailbox( + ConnectivityChecker connectivityChecker, + MailboxProperties properties) { + return new OwnMailboxContactListWorker(ioExecutor, db, eventBus, + connectivityChecker, mailboxApiCaller, mailboxApi, + mailboxUpdateManager, 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 new file mode 100644 index 000000000..19ea707c3 --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxClient.java @@ -0,0 +1,139 @@ +package org.briarproject.bramble.mailbox; + +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.nullsafety.NotNullByDefault; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.logging.Logger; + +import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; +import javax.annotation.concurrent.ThreadSafe; + +import static java.util.logging.Logger.getLogger; + +@ThreadSafe +@NotNullByDefault +class OwnMailboxClient implements MailboxClient { + + private static final Logger LOG = + getLogger(OwnMailboxClient.class.getName()); + + private final MailboxWorkerFactory workerFactory; + private final ConnectivityChecker connectivityChecker; + private final TorReachabilityMonitor reachabilityMonitor; + private final MailboxWorker contactListWorker; + private final Object lock = new Object(); + + @GuardedBy("lock") + private final Map uploadWorkers = new HashMap<>(); + + @GuardedBy("lock") + @Nullable + private MailboxWorker downloadWorker = null; + + @GuardedBy("lock") + private final Set assignedForDownload = new HashSet<>(); + + OwnMailboxClient(MailboxWorkerFactory workerFactory, + ConnectivityChecker connectivityChecker, + TorReachabilityMonitor reachabilityMonitor, + MailboxProperties properties) { + this.workerFactory = workerFactory; + this.connectivityChecker = connectivityChecker; + this.reachabilityMonitor = reachabilityMonitor; + contactListWorker = workerFactory.createContactListWorkerForOwnMailbox( + connectivityChecker, properties); + } + + @Override + public void start() { + LOG.info("Started"); + contactListWorker.start(); + } + + @Override + public void destroy() { + LOG.info("Destroyed"); + List uploadWorkers; + MailboxWorker downloadWorker; + synchronized (lock) { + uploadWorkers = new ArrayList<>(this.uploadWorkers.values()); + this.uploadWorkers.clear(); + downloadWorker = this.downloadWorker; + this.downloadWorker = null; + } + for (MailboxWorker worker : uploadWorkers) worker.destroy(); + if (downloadWorker != null) downloadWorker.destroy(); + contactListWorker.destroy(); + } + + @Override + public void assignContactForUpload(ContactId contactId, + MailboxProperties properties, MailboxFolderId folderId) { + LOG.info("Contact assigned for upload"); + if (!properties.isOwner()) throw new IllegalArgumentException(); + MailboxWorker uploadWorker = workerFactory.createUploadWorker( + connectivityChecker, properties, folderId, contactId); + synchronized (lock) { + MailboxWorker old = uploadWorkers.put(contactId, uploadWorker); + if (old != null) throw new IllegalStateException(); + } + uploadWorker.start(); + } + + @Override + public void deassignContactForUpload(ContactId contactId) { + LOG.info("Contact deassigned for upload"); + MailboxWorker uploadWorker; + synchronized (lock) { + uploadWorker = uploadWorkers.remove(contactId); + } + if (uploadWorker != null) uploadWorker.destroy(); + } + + @Override + public void assignContactForDownload(ContactId contactId, + MailboxProperties properties, MailboxFolderId folderId) { + LOG.info("Contact assigned for download"); + if (!properties.isOwner()) throw new IllegalArgumentException(); + MailboxWorker toStart = null; + synchronized (lock) { + if (!assignedForDownload.add(contactId)) { + throw new IllegalStateException(); + } + // Create a download worker if we don't already have one + if (downloadWorker == null) { + toStart = workerFactory.createDownloadWorkerForOwnMailbox( + connectivityChecker, reachabilityMonitor, properties); + downloadWorker = toStart; + } + } + if (toStart != null) toStart.start(); + } + + @Override + public void deassignContactForDownload(ContactId contactId) { + LOG.info("Contact deassigned for download"); + MailboxWorker toDestroy = null; + synchronized (lock) { + if (!assignedForDownload.remove(contactId)) { + throw new IllegalStateException(); + } + // If there are no more contacts assigned for download, destroy + // the download worker + if (assignedForDownload.isEmpty()) { + toDestroy = downloadWorker; + downloadWorker = null; + } + } + if (toDestroy != null) toDestroy.destroy(); + } +} diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxClientTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxClientTest.java index 0ec96d0a5..2c38b3938 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxClientTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxClientTest.java @@ -75,7 +75,7 @@ public class ContactMailboxClientTest extends BrambleMockTestCase { } @Test - public void assignContactForDownloadAndDestroyClient() { + public void testAssignContactForDownloadAndDestroyClient() { client.start(); // When the contact is assigned, the worker should be created and @@ -89,7 +89,7 @@ public class ContactMailboxClientTest extends BrambleMockTestCase { } @Test - public void assignAndDeassignContactForDownload() { + public void testAssignAndDeassignContactForDownload() { client.start(); // When the contact is assigned, the worker should be created and 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 new file mode 100644 index 000000000..b3dd9e744 --- /dev/null +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/OwnMailboxClientTest.java @@ -0,0 +1,208 @@ +package org.briarproject.bramble.mailbox; + +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.test.BrambleMockTestCase; +import org.jmock.Expectations; +import org.junit.Test; + +import static org.briarproject.bramble.api.mailbox.MailboxConstants.CLIENT_SUPPORTS; +import static org.briarproject.bramble.test.TestUtils.getContactId; +import static org.briarproject.bramble.test.TestUtils.getMailboxProperties; +import static org.briarproject.bramble.test.TestUtils.getRandomId; + +public class OwnMailboxClientTest extends BrambleMockTestCase { + + private final MailboxWorkerFactory workerFactory = + context.mock(MailboxWorkerFactory.class); + private final ConnectivityChecker connectivityChecker = + context.mock(ConnectivityChecker.class); + private final TorReachabilityMonitor reachabilityMonitor = + context.mock(TorReachabilityMonitor.class); + private final MailboxWorker contactListWorker = + context.mock(MailboxWorker.class, "contactListWorker"); + private final MailboxWorker uploadWorker1 = + context.mock(MailboxWorker.class, "uploadWorker1"); + private final MailboxWorker uploadWorker2 = + context.mock(MailboxWorker.class, "uploadWorker2"); + private final MailboxWorker downloadWorker = + context.mock(MailboxWorker.class, "downloadWorker"); + + private final MailboxProperties properties = + getMailboxProperties(true, CLIENT_SUPPORTS); + private final MailboxFolderId folderId = new MailboxFolderId(getRandomId()); + private final ContactId contactId1 = getContactId(); + private final ContactId contactId2 = getContactId(); + + private final OwnMailboxClient client; + + public OwnMailboxClientTest() { + expectCreateContactListWorker(); + client = new OwnMailboxClient(workerFactory, connectivityChecker, + reachabilityMonitor, properties); + context.assertIsSatisfied(); + } + + @Test + public void testStartAndDestroyWithNoContactsAssigned() { + expectStartWorker(contactListWorker); + client.start(); + + expectDestroyWorker(contactListWorker); + client.destroy(); + } + + @Test + public void testAssignContactForUploadAndDestroyClient() { + expectStartWorker(contactListWorker); + client.start(); + + // When the contact is assigned, the worker should be created and + // started + expectCreateUploadWorker(contactId1, uploadWorker1); + expectStartWorker(uploadWorker1); + client.assignContactForUpload(contactId1, properties, folderId); + + // When the client is destroyed, the worker should be destroyed + expectDestroyWorker(uploadWorker1); + expectDestroyWorker(contactListWorker); + client.destroy(); + } + + @Test + public void testAssignAndDeassignContactForUpload() { + expectStartWorker(contactListWorker); + client.start(); + + // When the contact is assigned, the worker should be created and + // started + expectCreateUploadWorker(contactId1, uploadWorker1); + expectStartWorker(uploadWorker1); + client.assignContactForUpload(contactId1, properties, folderId); + + // When the contact is deassigned, the worker should be destroyed + expectDestroyWorker(uploadWorker1); + client.deassignContactForUpload(contactId1); + context.assertIsSatisfied(); + + expectDestroyWorker(contactListWorker); + client.destroy(); + } + + @Test + public void testAssignAndDeassignTwoContactsForUpload() { + expectStartWorker(contactListWorker); + client.start(); + + // When the first contact is assigned, the first worker should be + // created and started + expectCreateUploadWorker(contactId1, uploadWorker1); + expectStartWorker(uploadWorker1); + client.assignContactForUpload(contactId1, properties, folderId); + + // When the second contact is assigned, the second worker should be + // created and started + expectCreateUploadWorker(contactId2, uploadWorker2); + expectStartWorker(uploadWorker2); + client.assignContactForUpload(contactId2, properties, folderId); + + // When the second contact is deassigned, the second worker should be + // destroyed + expectDestroyWorker(uploadWorker2); + client.deassignContactForUpload(contactId2); + context.assertIsSatisfied(); + + // When the first contact is deassigned, the first worker should be + // destroyed + expectDestroyWorker(uploadWorker1); + client.deassignContactForUpload(contactId1); + context.assertIsSatisfied(); + + expectDestroyWorker(contactListWorker); + client.destroy(); + } + + @Test + public void testAssignContactForDownloadAndDestroyClient() { + expectStartWorker(contactListWorker); + client.start(); + + // When the contact is assigned, the worker should be created and + // started + expectCreateDownloadWorker(); + expectStartWorker(downloadWorker); + client.assignContactForDownload(contactId1, properties, folderId); + + // When the client is destroyed, the worker should be destroyed + expectDestroyWorker(downloadWorker); + expectDestroyWorker(contactListWorker); + client.destroy(); + } + + @Test + public void testAssignAndDeassignTwoContactsForDownload() { + expectStartWorker(contactListWorker); + client.start(); + + // When the first contact is assigned, the worker should be created and + // started + expectCreateDownloadWorker(); + expectStartWorker(downloadWorker); + client.assignContactForDownload(contactId1, properties, folderId); + + // When the second contact is assigned, nothing should happen to the + // worker + client.assignContactForDownload(contactId2, properties, folderId); + + // When the first contact is deassigned, nothing should happen to the + // worker + client.deassignContactForDownload(contactId1); + + // When the second contact is deassigned, the worker should be + // destroyed + expectDestroyWorker(downloadWorker); + client.deassignContactForDownload(contactId2); + context.assertIsSatisfied(); + + expectDestroyWorker(contactListWorker); + client.destroy(); + } + + private void expectCreateContactListWorker() { + context.checking(new Expectations() {{ + oneOf(workerFactory).createContactListWorkerForOwnMailbox( + connectivityChecker, properties); + will(returnValue(contactListWorker)); + }}); + } + + private void expectCreateUploadWorker(ContactId contactId, + MailboxWorker worker) { + context.checking(new Expectations() {{ + oneOf(workerFactory).createUploadWorker(connectivityChecker, + properties, folderId, contactId); + will(returnValue(worker)); + }}); + } + + private void expectCreateDownloadWorker() { + context.checking(new Expectations() {{ + oneOf(workerFactory).createDownloadWorkerForOwnMailbox( + connectivityChecker, reachabilityMonitor, properties); + will(returnValue(downloadWorker)); + }}); + } + + private void expectStartWorker(MailboxWorker worker) { + context.checking(new Expectations() {{ + oneOf(worker).start(); + }}); + } + + private void expectDestroyWorker(MailboxWorker worker) { + context.checking(new Expectations() {{ + oneOf(worker).destroy(); + }}); + } +} From 85aa21ebf6f667d16052cc7ca0ea3de4b6e6044d Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 18 Jul 2022 11:25:27 +0100 Subject: [PATCH 2/2] Address review feedback. --- .../bramble/mailbox/MailboxClient.java | 9 ++++++++ .../bramble/mailbox/OwnMailboxClient.java | 21 ++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClient.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClient.java index 8dc4ab4e1..24a621030 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClient.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClient.java @@ -24,6 +24,10 @@ interface MailboxClient { /** * Assigns a contact to the client for upload. + * + * @param properties Properties for communicating with the mailbox + * managed by this client. + * @param folderId The ID of the folder to which files will be uploaded. */ void assignContactForUpload(ContactId c, MailboxProperties properties, MailboxFolderId folderId); @@ -35,6 +39,11 @@ interface MailboxClient { /** * Assigns a contact to the client for download. + * + * @param properties Properties for communicating with the mailbox + * managed by this client. + * @param folderId The ID of the folder from which files will be + * downloaded. */ void assignContactForDownload(ContactId c, MailboxProperties properties, MailboxFolderId folderId); 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 19ea707c3..c73d38151 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 @@ -32,13 +32,24 @@ class OwnMailboxClient implements MailboxClient { private final MailboxWorker contactListWorker; private final Object lock = new Object(); + /** + * Upload workers: one worker per contact assigned for upload. + */ @GuardedBy("lock") private final Map uploadWorkers = new HashMap<>(); + /** + * Download worker: shared between all contacts assigned for download. + * Null if no contacts are assigned for download. + */ @GuardedBy("lock") @Nullable private MailboxWorker downloadWorker = null; + /** + * IDs of contacts assigned for download, so that we know when to + * create/destroy the download worker. + */ @GuardedBy("lock") private final Set assignedForDownload = new HashSet<>(); @@ -46,6 +57,7 @@ class OwnMailboxClient implements MailboxClient { ConnectivityChecker connectivityChecker, TorReachabilityMonitor reachabilityMonitor, MailboxProperties properties) { + if (!properties.isOwner()) throw new IllegalArgumentException(); this.workerFactory = workerFactory; this.connectivityChecker = connectivityChecker; this.reachabilityMonitor = reachabilityMonitor; @@ -70,6 +82,7 @@ class OwnMailboxClient implements MailboxClient { downloadWorker = this.downloadWorker; this.downloadWorker = null; } + // Destroy the workers (with apologies to Mr Marx and Mr Engels) for (MailboxWorker worker : uploadWorkers) worker.destroy(); if (downloadWorker != null) downloadWorker.destroy(); contactListWorker.destroy(); @@ -104,12 +117,14 @@ class OwnMailboxClient implements MailboxClient { MailboxProperties properties, MailboxFolderId folderId) { LOG.info("Contact assigned for download"); if (!properties.isOwner()) throw new IllegalArgumentException(); + // Create a download worker if we don't already have one. The worker + // will use the API to discover which folders have files to download, + // so it doesn't need to track the set of assigned contacts MailboxWorker toStart = null; synchronized (lock) { if (!assignedForDownload.add(contactId)) { throw new IllegalStateException(); } - // Create a download worker if we don't already have one if (downloadWorker == null) { toStart = workerFactory.createDownloadWorkerForOwnMailbox( connectivityChecker, reachabilityMonitor, properties); @@ -122,13 +137,13 @@ class OwnMailboxClient implements MailboxClient { @Override public void deassignContactForDownload(ContactId contactId) { LOG.info("Contact deassigned for download"); + // If there are no more contacts assigned for download, destroy the + // download worker MailboxWorker toDestroy = null; synchronized (lock) { if (!assignedForDownload.remove(contactId)) { throw new IllegalStateException(); } - // If there are no more contacts assigned for download, destroy - // the download worker if (assignedForDownload.isEmpty()) { toDestroy = downloadWorker; downloadWorker = null;