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(); } }