From 2b4a1cf54b3b25a233f2fca9679a7e5b3f038439 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 27 May 2022 16:33:55 +0100 Subject: [PATCH 1/7] Refactor SimpleApiCall to support lambdas. --- .../ContactMailboxConnectivityChecker.java | 17 ++++--------- .../bramble/mailbox/SimpleApiCall.java | 24 +++++++++++++++---- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxConnectivityChecker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxConnectivityChecker.java index 8759aa595..8922948c6 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxConnectivityChecker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxConnectivityChecker.java @@ -5,8 +5,6 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.system.Clock; import org.briarproject.bramble.mailbox.MailboxApi.ApiException; -import java.io.IOException; - import javax.annotation.concurrent.ThreadSafe; @ThreadSafe @@ -24,16 +22,11 @@ class ContactMailboxConnectivityChecker extends ConnectivityCheckerImpl { @Override ApiCall createConnectivityCheckTask(MailboxProperties properties) { if (properties.isOwner()) throw new IllegalArgumentException(); - return new SimpleApiCall() { - @Override - void tryToCallApi() throws IOException, ApiException { - if (!mailboxApi.checkStatus(properties)) { - throw new ApiException(); - } - // Call the observers and cache the result - onConnectivityCheckSucceeded(clock.currentTimeMillis()); - } - }; + return new SimpleApiCall(() -> { + if (!mailboxApi.checkStatus(properties)) throw new ApiException(); + // Call the observers and cache the result + onConnectivityCheckSucceeded(clock.currentTimeMillis()); + }); } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/SimpleApiCall.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/SimpleApiCall.java index 7c3e6f4b0..9a34a6244 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/SimpleApiCall.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/SimpleApiCall.java @@ -16,17 +16,20 @@ import static org.briarproject.bramble.util.LogUtils.logException; * Convenience class for making simple API calls that don't return values. */ @NotNullByDefault -public abstract class SimpleApiCall implements ApiCall { +class SimpleApiCall implements ApiCall { private static final Logger LOG = getLogger(SimpleApiCall.class.getName()); - abstract void tryToCallApi() - throws IOException, ApiException, TolerableFailureException; + private final Attempt attempt; + + SimpleApiCall(Attempt attempt) { + this.attempt = attempt; + } @Override public boolean callApi() { try { - tryToCallApi(); + attempt.tryToCallApi(); return false; // Succeeded, don't retry } catch (IOException | ApiException e) { logException(LOG, WARNING, e); @@ -36,4 +39,17 @@ public abstract class SimpleApiCall implements ApiCall { return false; // Failed tolerably, don't retry } } + + interface Attempt { + + /** + * Makes a single attempt to call an API endpoint. If this method + * throws an {@link IOException} or an {@link ApiException}, the call + * will be retried. If it throws a {@link TolerableFailureException} + * or returns without throwing an exception, the call will not be + * retried. + */ + void tryToCallApi() + throws IOException, ApiException, TolerableFailureException; + } } From 9aacd9d3d8b047d6bd43ea1af3d692aa0c795a00 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 27 May 2022 16:34:26 +0100 Subject: [PATCH 2/7] Allow observers to be removed. --- .../bramble/mailbox/ConnectivityChecker.java | 11 ++++++++ .../mailbox/ConnectivityCheckerImpl.java | 15 +++++++++-- .../mailbox/TorReachabilityMonitor.java | 6 +++++ .../mailbox/TorReachabilityMonitorImpl.java | 8 ++++++ .../mailbox/ConnectivityCheckerImplTest.java | 26 +++++++++++++++++++ 5 files changed, 64 insertions(+), 2 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ConnectivityChecker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ConnectivityChecker.java index 921557027..d0a6ab57d 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ConnectivityChecker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ConnectivityChecker.java @@ -22,10 +22,21 @@ interface ConnectivityChecker { * the check succeeds. If a check is already running then the observer is * called when the check succeeds. If a connectivity check has recently * succeeded then the observer is called immediately. + *

+ * Observers are removed after being called, or when the checker is + * {@link #destroy() destroyed}. */ void checkConnectivity(MailboxProperties properties, ConnectivityObserver o); + /** + * Removes an observer that was added via + * {@link #checkConnectivity(MailboxProperties, ConnectivityObserver)}. If + * there are no remaining observers and a connectivity check is running + * then the check will be cancelled. + */ + void removeObserver(ConnectivityObserver o); + interface ConnectivityObserver { void onConnectivityCheckSucceeded(); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ConnectivityCheckerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ConnectivityCheckerImpl.java index 1468c5db1..aa3e97930 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ConnectivityCheckerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ConnectivityCheckerImpl.java @@ -80,8 +80,7 @@ abstract class ConnectivityCheckerImpl implements ConnectivityChecker { > CONNECTIVITY_CHECK_FRESHNESS_MS) { // The last connectivity check is stale, start a new one connectivityObservers.add(o); - ApiCall task = - createConnectivityCheckTask(properties); + ApiCall task = createConnectivityCheckTask(properties); connectivityCheck = mailboxApiCaller.retryWithBackoff(task); } else { // The last connectivity check is fresh @@ -108,4 +107,16 @@ abstract class ConnectivityCheckerImpl implements ConnectivityChecker { o.onConnectivityCheckSucceeded(); } } + + @Override + public void removeObserver(ConnectivityObserver o) { + synchronized (lock) { + if (destroyed) return; + connectivityObservers.remove(o); + if (connectivityObservers.isEmpty() && connectivityCheck != null) { + connectivityCheck.cancel(); + connectivityCheck = null; + } + } + } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/TorReachabilityMonitor.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/TorReachabilityMonitor.java index c6b86d88a..5dd10b365 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/TorReachabilityMonitor.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/TorReachabilityMonitor.java @@ -38,6 +38,12 @@ interface TorReachabilityMonitor { */ void addOneShotObserver(TorReachabilityObserver o); + /** + * Removes an observer that was added via + * {@link #addOneShotObserver(TorReachabilityObserver)}. + */ + void removeObserver(TorReachabilityObserver o); + interface TorReachabilityObserver { void onTorReachable(); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/TorReachabilityMonitorImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/TorReachabilityMonitorImpl.java index fa16682b3..8620aee01 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/TorReachabilityMonitorImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/TorReachabilityMonitorImpl.java @@ -87,6 +87,14 @@ class TorReachabilityMonitorImpl if (callNow) o.onTorReachable(); } + @Override + public void removeObserver(TorReachabilityObserver o) { + synchronized (lock) { + if (destroyed) return; + observers.remove(o); + } + } + @Override public void eventOccurred(Event e) { if (e instanceof TransportActiveEvent) { diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ConnectivityCheckerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ConnectivityCheckerImplTest.java index 09fb0df5b..a476ec552 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ConnectivityCheckerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ConnectivityCheckerImplTest.java @@ -187,6 +187,32 @@ public class ConnectivityCheckerImplTest extends BrambleMockTestCase { checker.onConnectivityCheckSucceeded(now); } + @Test + public void testCheckIsCancelledWhenObserverIsRemoved() { + ConnectivityCheckerImpl checker = createChecker(); + + // When checkConnectivity() is called a check should be started + context.checking(new Expectations() {{ + oneOf(clock).currentTimeMillis(); + will(returnValue(now)); + oneOf(mailboxApiCaller).retryWithBackoff(apiCall); + will(returnValue(task)); + }}); + + checker.checkConnectivity(properties, observer1); + + // When the observer is removed the check should be cancelled + context.checking(new Expectations() {{ + oneOf(task).cancel(); + }}); + + checker.removeObserver(observer1); + + // If the check runs anyway (cancellation came too late) the observer + // should not be called + checker.onConnectivityCheckSucceeded(now); + } + private ConnectivityCheckerImpl createChecker() { return new ConnectivityCheckerImpl(clock, mailboxApiCaller) { From 79f41064e4850fefe34e1f1a71c2e8e53fb2e4e9 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Thu, 2 Jun 2022 14:25:05 +0100 Subject: [PATCH 3/7] Add download worker for a contact's mailbox. --- .../mailbox/ContactMailboxDownloadWorker.java | 260 ++++++++++++++++++ .../bramble/mailbox/MailboxApiCaller.java | 2 + .../bramble/mailbox/MailboxWorker.java | 23 ++ 3 files changed, 285 insertions(+) create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java create mode 100644 bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorker.java diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java new file mode 100644 index 000000000..818403215 --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java @@ -0,0 +1,260 @@ +package org.briarproject.bramble.mailbox; + +import org.briarproject.bramble.api.Cancellable; +import org.briarproject.bramble.api.mailbox.MailboxProperties; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.mailbox.ConnectivityChecker.ConnectivityObserver; +import org.briarproject.bramble.mailbox.MailboxApi.ApiException; +import org.briarproject.bramble.mailbox.MailboxApi.MailboxFile; +import org.briarproject.bramble.mailbox.MailboxApi.TolerableFailureException; +import org.briarproject.bramble.mailbox.TorReachabilityMonitor.TorReachabilityObserver; + +import java.io.File; +import java.io.IOException; +import java.util.ArrayList; +import java.util.LinkedList; +import java.util.List; +import java.util.Queue; +import java.util.logging.Logger; + +import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; +import javax.annotation.concurrent.ThreadSafe; + +import static java.util.Collections.sort; +import static java.util.logging.Level.INFO; +import static java.util.logging.Logger.getLogger; +import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; +import static org.briarproject.bramble.util.LogUtils.logException; + +@ThreadSafe +@NotNullByDefault +class ContactMailboxDownloadWorker implements MailboxWorker, + ConnectivityObserver, TorReachabilityObserver { + + private enum State { + CREATED, + CONNECTIVITY_CHECK, + DOWNLOAD_CYCLE_1, + WAITING_FOR_TOR, + DOWNLOAD_CYCLE_2, + FINISHED, + DESTROYED + } + + private static final Logger LOG = + getLogger(ContactMailboxDownloadWorker.class.getName()); + + private final ConnectivityChecker connectivityChecker; + private final TorReachabilityMonitor torReachabilityMonitor; + private final MailboxApiCaller mailboxApiCaller; + private final MailboxApi mailboxApi; + private final MailboxFileManager mailboxFileManager; + private final MailboxProperties mailboxProperties; + private final Object lock = new Object(); + + @GuardedBy("lock") + private State state = State.CREATED; + + @GuardedBy("lock") + @Nullable + private Cancellable apiCall = null; + + @GuardedBy("lock") + @Nullable + private ConnectivityObserver connectivityObserver = null; + + @GuardedBy("lock") + @Nullable + private TorReachabilityObserver reachabilityObserver = null; + + ContactMailboxDownloadWorker( + ConnectivityChecker connectivityChecker, + TorReachabilityMonitor torReachabilityMonitor, + MailboxApiCaller mailboxApiCaller, + MailboxApi mailboxApi, + MailboxFileManager mailboxFileManager, + MailboxProperties mailboxProperties) { + if (mailboxProperties.isOwner()) throw new IllegalArgumentException(); + this.connectivityChecker = connectivityChecker; + this.torReachabilityMonitor = torReachabilityMonitor; + this.mailboxApiCaller = mailboxApiCaller; + this.mailboxApi = mailboxApi; + this.mailboxFileManager = mailboxFileManager; + this.mailboxProperties = mailboxProperties; + } + + @Override + public void start() { + LOG.info("Started"); + synchronized (lock) { + // Don't allow the worker to be reused + if (state != State.CREATED) throw new IllegalStateException(); + state = State.CONNECTIVITY_CHECK; + } + connectivityChecker.checkConnectivity(mailboxProperties, this); + // Avoid leaking observer in case destroy() is called concurrently + boolean remove = false; + synchronized (lock) { + if (state == State.DESTROYED) remove = true; + else connectivityObserver = this; + } + if (remove) connectivityChecker.removeObserver(this); + } + + @Override + public void destroy() { + LOG.info("Destroyed"); + Cancellable apiCall; + ConnectivityObserver connectivityObserver; + TorReachabilityObserver reachabilityObserver; + synchronized (lock) { + state = State.DESTROYED; + apiCall = this.apiCall; + this.apiCall = null; + connectivityObserver = this.connectivityObserver; + this.connectivityObserver = null; + reachabilityObserver = this.reachabilityObserver; + this.reachabilityObserver = null; + } + if (apiCall != null) apiCall.cancel(); + if (connectivityObserver != null) { + connectivityChecker.removeObserver(connectivityObserver); + } + if (reachabilityObserver != null) { + torReachabilityMonitor.removeObserver(reachabilityObserver); + } + } + + @Override + public void onConnectivityCheckSucceeded() { + LOG.info("Connectivity check succeeded"); + synchronized (lock) { + if (state != State.CONNECTIVITY_CHECK) return; + state = State.DOWNLOAD_CYCLE_1; + // No need to remove the observer in destroy() + connectivityObserver = null; + // Start first download cycle + apiCall = mailboxApiCaller.retryWithBackoff( + new SimpleApiCall(this::apiCallListInbox)); + } + } + + private void apiCallListInbox() throws IOException, ApiException { + synchronized (lock) { + if (state == State.DESTROYED) return; + } + LOG.info("Listing inbox"); + List files = mailboxApi.getFiles(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId())); + if (files.isEmpty()) { + onDownloadCycleFinished(); + } else { + files = new ArrayList<>(files); + //noinspection UseCompareMethod,Java8ListSort + sort(files, (a, b) -> Long.valueOf(a.time).compareTo(b.time)); + downloadNextFile(new LinkedList<>(files)); + } + } + + private void onDownloadCycleFinished() { + boolean addObserver = false; + synchronized (lock) { + if (state == State.DOWNLOAD_CYCLE_1) { + LOG.info("First download cycle finished"); + state = State.WAITING_FOR_TOR; + addObserver = true; + } else if (state == State.DOWNLOAD_CYCLE_2) { + LOG.info("Second download cycle finished"); + state = State.FINISHED; + } + } + if (addObserver) { + torReachabilityMonitor.addOneShotObserver(this); + // Avoid leaking observer in case destroy() is called concurrently + boolean remove = false; + synchronized (lock) { + if (state == State.DESTROYED) remove = true; + else reachabilityObserver = this; + } + if (remove) torReachabilityMonitor.removeObserver(this); + } + } + + private void downloadNextFile(Queue queue) { + synchronized (lock) { + if (state == State.DESTROYED) return; + MailboxFile file = queue.remove(); + apiCall = mailboxApiCaller.retryWithBackoff( + new SimpleApiCall(() -> apiCallDownloadFile(file, queue))); + } + } + + private void apiCallDownloadFile(MailboxFile file, + Queue queue) throws IOException, ApiException { + synchronized (lock) { + if (state == State.DESTROYED) return; + } + LOG.info("Downloading file"); + File tempFile = mailboxFileManager.createTempFileForDownload(); + try { + mailboxApi.getFile(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId()), + file.name, tempFile); + } catch (IOException | ApiException e) { + if (!tempFile.delete()) { + LOG.warning("Failed to delete temporary file"); + } + throw e; + } + mailboxFileManager.handleDownloadedFile(tempFile); + deleteFile(file, queue); + } + + private void deleteFile(MailboxFile file, Queue queue) { + synchronized (lock) { + if (state == State.DESTROYED) return; + apiCall = mailboxApiCaller.retryWithBackoff( + new SimpleApiCall(() -> apiCallDeleteFile(file, queue))); + } + } + + private void apiCallDeleteFile(MailboxFile file, Queue queue) + throws IOException, ApiException { + synchronized (lock) { + if (state == State.DESTROYED) return; + } + try { + mailboxApi.deleteFile(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId()), file.name); + } catch (TolerableFailureException e) { + // Catch this so we can continue to the next file + logException(LOG, INFO, e); + } + if (queue.isEmpty()) { + // List the inbox again to check for files that may have arrived + // while we were downloading + synchronized (lock) { + if (state == State.DESTROYED) return; + apiCall = mailboxApiCaller.retryWithBackoff( + new SimpleApiCall(this::apiCallListInbox)); + } + } else { + downloadNextFile(queue); + } + } + + @Override + public void onTorReachable() { + LOG.info("Our Tor hidden service is reachable"); + synchronized (lock) { + if (state != State.WAITING_FOR_TOR) return; + state = State.DOWNLOAD_CYCLE_2; + // No need to remove the observer in destroy() + reachabilityObserver = null; + // Start second download cycle + apiCall = mailboxApiCaller.retryWithBackoff( + new SimpleApiCall(this::apiCallListInbox)); + } + } +} diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiCaller.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiCaller.java index a3888e01c..3da4616ed 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiCaller.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiCaller.java @@ -24,6 +24,8 @@ interface MailboxApiCaller { * Asynchronously calls the given API call on the {@link IoExecutor}, * automatically retrying at increasing intervals until the API call * returns false or retries are cancelled. + *

+ * This method is safe to call while holding a lock. * * @return A {@link Cancellable} that can be used to cancel any future * retries. diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorker.java new file mode 100644 index 000000000..75b0f7ce7 --- /dev/null +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxWorker.java @@ -0,0 +1,23 @@ +package org.briarproject.bramble.mailbox; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import javax.annotation.concurrent.ThreadSafe; + +/** + * A worker that downloads files from a contact's mailbox. + */ +@ThreadSafe +@NotNullByDefault +interface MailboxWorker { + + /** + * Asynchronously starts the worker. + */ + void start(); + + /** + * Destroys the worker and cancels any pending tasks or retries. + */ + void destroy(); +} From 97d11cc60229f8243ddf4cb082cf38196386c4ff Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 3 Jun 2022 17:35:05 +0100 Subject: [PATCH 4/7] Add tests for download worker. --- .../ContactMailboxDownloadWorkerTest.java | 210 ++++++++++++++++++ 1 file changed, 210 insertions(+) create mode 100644 bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java new file mode 100644 index 000000000..d2d08ed77 --- /dev/null +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java @@ -0,0 +1,210 @@ +package org.briarproject.bramble.mailbox; + +import org.briarproject.bramble.api.mailbox.MailboxFileId; +import org.briarproject.bramble.api.mailbox.MailboxProperties; +import org.briarproject.bramble.mailbox.MailboxApi.MailboxFile; +import org.briarproject.bramble.mailbox.MailboxApi.TolerableFailureException; +import org.briarproject.bramble.test.BrambleMockTestCase; +import org.briarproject.bramble.test.CaptureArgumentAction; +import org.jmock.Expectations; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; + +import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; +import static org.briarproject.bramble.api.mailbox.MailboxConstants.CLIENT_SUPPORTS; +import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; +import static org.briarproject.bramble.test.TestUtils.deleteTestDirectory; +import static org.briarproject.bramble.test.TestUtils.getMailboxProperties; +import static org.briarproject.bramble.test.TestUtils.getRandomId; +import static org.briarproject.bramble.test.TestUtils.getTestDirectory; +import static org.junit.Assert.assertFalse; + +public class ContactMailboxDownloadWorkerTest extends BrambleMockTestCase { + + private final ConnectivityChecker connectivityChecker = + context.mock(ConnectivityChecker.class); + private final TorReachabilityMonitor torReachabilityMonitor = + context.mock(TorReachabilityMonitor.class); + private final MailboxApiCaller mailboxApiCaller = + context.mock(MailboxApiCaller.class); + private final MailboxApi mailboxApi = context.mock(MailboxApi.class); + private final MailboxFileManager mailboxFileManager = + context.mock(MailboxFileManager.class); + + private final MailboxProperties mailboxProperties = + getMailboxProperties(false, CLIENT_SUPPORTS); + private final long now = System.currentTimeMillis(); + private final MailboxFile file1 = + new MailboxFile(new MailboxFileId(getRandomId()), now - 1); + private final MailboxFile file2 = + new MailboxFile(new MailboxFileId(getRandomId()), now); + private final List filesInWrongOrder = asList(file2, file1); + + private File testDir, tempFile; + private ContactMailboxDownloadWorker worker; + + @Before + public void setUp() { + testDir = getTestDirectory(); + tempFile = new File(testDir, "temp"); + worker = new ContactMailboxDownloadWorker(connectivityChecker, + torReachabilityMonitor, mailboxApiCaller, mailboxApi, + mailboxFileManager, mailboxProperties); + } + + @After + public void tearDown() { + deleteTestDirectory(testDir); + } + + @Test + public void testChecksConnectivityWhenStartedAndRemovesObserverWhenDestroyed() { + // When the worker is started it should start a connectivity check + context.checking(new Expectations() {{ + oneOf(connectivityChecker).checkConnectivity( + with(mailboxProperties), with(worker)); + }}); + + worker.start(); + + // When the worker is destroyed it should remove the connectivity + // observer + context.checking(new Expectations() {{ + oneOf(connectivityChecker).removeObserver(worker); + }}); + + worker.destroy(); + } + + @Test + public void testDownloadsFilesWhenConnectivityCheckSucceeds() + throws Exception { + // When the worker is started it should start a connectivity check + context.checking(new Expectations() {{ + oneOf(connectivityChecker).checkConnectivity( + with(mailboxProperties), with(worker)); + }}); + + worker.start(); + + // When the connectivity check succeeds, a list-inbox task should be + // started for the first download cycle + AtomicReference listTask = new AtomicReference<>(null); + context.checking(new Expectations() {{ + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new CaptureArgumentAction<>(listTask, ApiCall.class, 0)); + }}); + + worker.onConnectivityCheckSucceeded(); + + // When the list-inbox tasks runs and finds some files to download, + // it should sort them in timestamp order and start a download task + // for the first file + AtomicReference downloadTask = new AtomicReference<>(null); + context.checking(new Expectations() {{ + oneOf(mailboxApi).getFiles(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId())); + will(returnValue(filesInWrongOrder)); + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new CaptureArgumentAction<>(downloadTask, ApiCall.class, 0)); + }}); + + assertFalse(listTask.get().callApi()); + + // When the first download task runs it should download the file to the + // location provided by the file manager and start a delete task + AtomicReference deleteTask = new AtomicReference<>(null); + context.checking(new Expectations() {{ + oneOf(mailboxFileManager).createTempFileForDownload(); + will(returnValue(tempFile)); + oneOf(mailboxApi).getFile(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId()), + file1.name, tempFile); + oneOf(mailboxFileManager).handleDownloadedFile(tempFile); + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new CaptureArgumentAction<>(deleteTask, ApiCall.class, 0)); + }}); + + assertFalse(downloadTask.get().callApi()); + + // When the first delete task runs it should delete the file, ignore + // the tolerable failure, and start a download task for the next file + context.checking(new Expectations() {{ + oneOf(mailboxApi).deleteFile(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId()), file1.name); + will(throwException(new TolerableFailureException())); + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new CaptureArgumentAction<>(downloadTask, ApiCall.class, 0)); + }}); + + assertFalse(deleteTask.get().callApi()); + + // When the second download task runs it should download the file to + // the location provided by the file manager and start a delete task + context.checking(new Expectations() {{ + oneOf(mailboxFileManager).createTempFileForDownload(); + will(returnValue(tempFile)); + oneOf(mailboxApi).getFile(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId()), + file2.name, tempFile); + oneOf(mailboxFileManager).handleDownloadedFile(tempFile); + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new CaptureArgumentAction<>(deleteTask, ApiCall.class, 0)); + }}); + + assertFalse(downloadTask.get().callApi()); + + // When the second delete task runs it should delete the file and + // start a list-inbox task to check for files that may have arrived + // since the first download cycle started + context.checking(new Expectations() {{ + oneOf(mailboxApi).deleteFile(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId()), file2.name); + will(throwException(new TolerableFailureException())); + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new CaptureArgumentAction<>(listTask, ApiCall.class, 0)); + }}); + + assertFalse(deleteTask.get().callApi()); + + // When the list-inbox tasks runs and finds no more files to download, + // it should add a Tor reachability observer + context.checking(new Expectations() {{ + oneOf(mailboxApi).getFiles(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId())); + will(returnValue(emptyList())); + oneOf(torReachabilityMonitor).addOneShotObserver(worker); + }}); + + assertFalse(listTask.get().callApi()); + + // When the reachability observer is called, a list-inbox task should + // be started for the second download cycle + context.checking(new Expectations() {{ + oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); + will(new CaptureArgumentAction<>(listTask, ApiCall.class, 0)); + }}); + + worker.onTorReachable(); + + // When the list-inbox tasks runs and finds no more files to download, + // it should finish the second download cycle + context.checking(new Expectations() {{ + oneOf(mailboxApi).getFiles(mailboxProperties, + requireNonNull(mailboxProperties.getInboxId())); + will(returnValue(emptyList())); + }}); + + assertFalse(listTask.get().callApi()); + + // When the worker is destroyed it should not remove the connectivity + // and reachability observers, which were removed when they were called + worker.destroy(); + } +} From 5d363496bd93309b3f125f9a25eb20aaba38fd3e Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 8 Jun 2022 12:03:11 +0100 Subject: [PATCH 5/7] Download files in the order the mailbox returns them. --- .../mailbox/ContactMailboxDownloadWorker.java | 12 ++---------- .../mailbox/ContactMailboxDownloadWorkerTest.java | 7 +++---- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java index 818403215..971d6e7cd 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java @@ -11,7 +11,6 @@ import org.briarproject.bramble.mailbox.TorReachabilityMonitor.TorReachabilityOb import java.io.File; import java.io.IOException; -import java.util.ArrayList; import java.util.LinkedList; import java.util.List; import java.util.Queue; @@ -21,7 +20,6 @@ import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; -import static java.util.Collections.sort; import static java.util.logging.Level.INFO; import static java.util.logging.Logger.getLogger; import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; @@ -147,14 +145,8 @@ class ContactMailboxDownloadWorker implements MailboxWorker, LOG.info("Listing inbox"); List files = mailboxApi.getFiles(mailboxProperties, requireNonNull(mailboxProperties.getInboxId())); - if (files.isEmpty()) { - onDownloadCycleFinished(); - } else { - files = new ArrayList<>(files); - //noinspection UseCompareMethod,Java8ListSort - sort(files, (a, b) -> Long.valueOf(a.time).compareTo(b.time)); - downloadNextFile(new LinkedList<>(files)); - } + if (files.isEmpty()) onDownloadCycleFinished(); + else downloadNextFile(new LinkedList<>(files)); } private void onDownloadCycleFinished() { diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java index d2d08ed77..cd6b4f210 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java @@ -44,7 +44,7 @@ public class ContactMailboxDownloadWorkerTest extends BrambleMockTestCase { new MailboxFile(new MailboxFileId(getRandomId()), now - 1); private final MailboxFile file2 = new MailboxFile(new MailboxFileId(getRandomId()), now); - private final List filesInWrongOrder = asList(file2, file1); + private final List files = asList(file1, file2); private File testDir, tempFile; private ContactMailboxDownloadWorker worker; @@ -104,13 +104,12 @@ public class ContactMailboxDownloadWorkerTest extends BrambleMockTestCase { worker.onConnectivityCheckSucceeded(); // When the list-inbox tasks runs and finds some files to download, - // it should sort them in timestamp order and start a download task - // for the first file + // it should start a download task for the first file AtomicReference downloadTask = new AtomicReference<>(null); context.checking(new Expectations() {{ oneOf(mailboxApi).getFiles(mailboxProperties, requireNonNull(mailboxProperties.getInboxId())); - will(returnValue(filesInWrongOrder)); + will(returnValue(files)); oneOf(mailboxApiCaller).retryWithBackoff(with(any(ApiCall.class))); will(new CaptureArgumentAction<>(downloadTask, ApiCall.class, 0)); }}); From 6288577daabcb462bf0ff39e39104717df47433e Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 8 Jun 2022 12:13:07 +0100 Subject: [PATCH 6/7] Add javadoc explaining worker's lifecycle. --- .../bramble/mailbox/ContactMailboxDownloadWorker.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java index 971d6e7cd..fdf682a8b 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java @@ -30,6 +30,17 @@ import static org.briarproject.bramble.util.LogUtils.logException; class ContactMailboxDownloadWorker implements MailboxWorker, ConnectivityObserver, TorReachabilityObserver { + /** + * When the worker is started it waits for a connectivity check, then + * starts its first download cycle: checking the inbox, downloading and + * deleting any files, and checking again until the inbox is empty. + *

+ * The worker then waits for our Tor hidden service to be reachable before + * starting its second download cycle. This ensures that if a contact + * tried and failed to connect to our hidden service before it was + * reachable, and therefore uploaded a file to the mailbox instead, we'll + * find the file in the second download cycle. + */ private enum State { CREATED, CONNECTIVITY_CHECK, From 01e72eff40f8324ccef825769182fa8ce439019b Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 8 Jun 2022 13:53:27 +0100 Subject: [PATCH 7/7] Always remove observers in destroy(). --- .../mailbox/ContactMailboxDownloadWorker.java | 46 +++++-------------- .../ContactMailboxDownloadWorkerTest.java | 12 +++-- 2 files changed, 21 insertions(+), 37 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java index fdf682a8b..70e4266f6 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorker.java @@ -69,14 +69,6 @@ class ContactMailboxDownloadWorker implements MailboxWorker, @Nullable private Cancellable apiCall = null; - @GuardedBy("lock") - @Nullable - private ConnectivityObserver connectivityObserver = null; - - @GuardedBy("lock") - @Nullable - private TorReachabilityObserver reachabilityObserver = null; - ContactMailboxDownloadWorker( ConnectivityChecker connectivityChecker, TorReachabilityMonitor torReachabilityMonitor, @@ -101,38 +93,28 @@ class ContactMailboxDownloadWorker implements MailboxWorker, if (state != State.CREATED) throw new IllegalStateException(); state = State.CONNECTIVITY_CHECK; } - connectivityChecker.checkConnectivity(mailboxProperties, this); // Avoid leaking observer in case destroy() is called concurrently - boolean remove = false; + // before observer is added + connectivityChecker.checkConnectivity(mailboxProperties, this); + boolean destroyed; synchronized (lock) { - if (state == State.DESTROYED) remove = true; - else connectivityObserver = this; + destroyed = state == State.DESTROYED; } - if (remove) connectivityChecker.removeObserver(this); + if (destroyed) connectivityChecker.removeObserver(this); } @Override public void destroy() { LOG.info("Destroyed"); Cancellable apiCall; - ConnectivityObserver connectivityObserver; - TorReachabilityObserver reachabilityObserver; synchronized (lock) { state = State.DESTROYED; apiCall = this.apiCall; this.apiCall = null; - connectivityObserver = this.connectivityObserver; - this.connectivityObserver = null; - reachabilityObserver = this.reachabilityObserver; - this.reachabilityObserver = null; } if (apiCall != null) apiCall.cancel(); - if (connectivityObserver != null) { - connectivityChecker.removeObserver(connectivityObserver); - } - if (reachabilityObserver != null) { - torReachabilityMonitor.removeObserver(reachabilityObserver); - } + connectivityChecker.removeObserver(this); + torReachabilityMonitor.removeObserver(this); } @Override @@ -141,8 +123,6 @@ class ContactMailboxDownloadWorker implements MailboxWorker, synchronized (lock) { if (state != State.CONNECTIVITY_CHECK) return; state = State.DOWNLOAD_CYCLE_1; - // No need to remove the observer in destroy() - connectivityObserver = null; // Start first download cycle apiCall = mailboxApiCaller.retryWithBackoff( new SimpleApiCall(this::apiCallListInbox)); @@ -173,14 +153,14 @@ class ContactMailboxDownloadWorker implements MailboxWorker, } } if (addObserver) { - torReachabilityMonitor.addOneShotObserver(this); // Avoid leaking observer in case destroy() is called concurrently - boolean remove = false; + // before observer is added + torReachabilityMonitor.addOneShotObserver(this); + boolean destroyed; synchronized (lock) { - if (state == State.DESTROYED) remove = true; - else reachabilityObserver = this; + destroyed = state == State.DESTROYED; } - if (remove) torReachabilityMonitor.removeObserver(this); + if (destroyed) torReachabilityMonitor.removeObserver(this); } } @@ -253,8 +233,6 @@ class ContactMailboxDownloadWorker implements MailboxWorker, synchronized (lock) { if (state != State.WAITING_FOR_TOR) return; state = State.DOWNLOAD_CYCLE_2; - // No need to remove the observer in destroy() - reachabilityObserver = null; // Start second download cycle apiCall = mailboxApiCaller.retryWithBackoff( new SimpleApiCall(this::apiCallListInbox)); diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java index cd6b4f210..273c4f016 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/ContactMailboxDownloadWorkerTest.java @@ -74,9 +74,10 @@ public class ContactMailboxDownloadWorkerTest extends BrambleMockTestCase { worker.start(); // When the worker is destroyed it should remove the connectivity - // observer + // and reachability observers context.checking(new Expectations() {{ oneOf(connectivityChecker).removeObserver(worker); + oneOf(torReachabilityMonitor).removeObserver(worker); }}); worker.destroy(); @@ -202,8 +203,13 @@ public class ContactMailboxDownloadWorkerTest extends BrambleMockTestCase { assertFalse(listTask.get().callApi()); - // When the worker is destroyed it should not remove the connectivity - // and reachability observers, which were removed when they were called + // When the worker is destroyed it should remove the connectivity + // and reachability observers + context.checking(new Expectations() {{ + oneOf(connectivityChecker).removeObserver(worker); + oneOf(torReachabilityMonitor).removeObserver(worker); + }}); + worker.destroy(); } }