From 5defd500ae48d49197871bd1696bf6544dba31a9 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 27 Jul 2022 15:26:23 +0100 Subject: [PATCH] Tolerate 404 responses due to missing folders. This prevents our own mailbox's download worker from getting stuck trying to list a folder that has been removed. Instead, the worker will move on to the next folder. --- .../mailbox/ContactMailboxDownloadWorker.java | 11 ++++- .../bramble/mailbox/MailboxApi.java | 17 ++++++-- .../bramble/mailbox/MailboxApiImpl.java | 8 +++- .../mailbox/MailboxDownloadWorker.java | 40 ++++++++++--------- .../mailbox/OwnMailboxContactListWorker.java | 2 +- .../mailbox/OwnMailboxDownloadWorker.java | 13 ++++-- .../bramble/mailbox/MailboxApiTest.java | 15 +++++-- .../mailbox/MailboxIntegrationTest.java | 25 +++++++----- 8 files changed, 87 insertions(+), 44 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 3922ebccf..2b1bad8fd 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 @@ -5,6 +5,7 @@ import org.briarproject.bramble.api.mailbox.MailboxProperties; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.mailbox.MailboxApi.ApiException; import org.briarproject.bramble.mailbox.MailboxApi.MailboxFile; +import org.briarproject.bramble.mailbox.MailboxApi.TolerableFailureException; import java.io.IOException; import java.util.LinkedList; @@ -13,6 +14,7 @@ import java.util.Queue; import javax.annotation.concurrent.ThreadSafe; +import static java.util.Collections.emptyList; import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; @ThreadSafe @@ -43,8 +45,13 @@ class ContactMailboxDownloadWorker extends MailboxDownloadWorker { LOG.info("Listing inbox"); MailboxFolderId folderId = requireNonNull(mailboxProperties.getInboxId()); - List files = - mailboxApi.getFiles(mailboxProperties, folderId); + List files; + try { + files = mailboxApi.getFiles(mailboxProperties, folderId); + } catch (TolerableFailureException e) { + LOG.warning("Inbox folder does not exist"); + files = emptyList(); + } if (files.isEmpty()) { onDownloadCycleFinished(); } else { diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java index 1e384009b..2939ab968 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApi.java @@ -91,9 +91,13 @@ interface MailboxApi { * Used by owner and contacts to list their files to retrieve. *

* Returns 200 OK with the list of files in JSON. + * + * @throws TolerableFailureException if response code is 404 (folder does + * not exist or client is not authorised to download from it) */ List getFiles(MailboxProperties properties, - MailboxFolderId folderId) throws IOException, ApiException; + MailboxFolderId folderId) + throws IOException, ApiException, TolerableFailureException; /** * Used by owner and contacts to retrieve a file. @@ -102,17 +106,22 @@ interface MailboxApi { * in the response body. * * @param file the empty file the response bytes will be written into. + * @throws TolerableFailureException if response code is 404 (folder does + * not exist, client is not authorised to download from folder, or file + * does not exist) */ void getFile(MailboxProperties properties, MailboxFolderId folderId, - MailboxFileId fileId, File file) throws IOException, ApiException; + MailboxFileId fileId, File file) + throws IOException, ApiException, TolerableFailureException; /** * Used by owner and contacts to delete files. *

* Returns 200 OK (no exception) if deletion was successful. * - * @throws TolerableFailureException on 404 response, - * because file was most likely deleted already. + * @throws TolerableFailureException if response code is 404 (folder does + * not exist, client is not authorised to download from folder, or file + * does not exist) */ void deleteFile(MailboxProperties properties, MailboxFolderId folderId, MailboxFileId fileId) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java index 06e1306c6..7bf057904 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxApiImpl.java @@ -216,9 +216,11 @@ class MailboxApiImpl implements MailboxApi { @Override public List getFiles(MailboxProperties properties, - MailboxFolderId folderId) throws IOException, ApiException { + MailboxFolderId folderId) + throws IOException, ApiException, TolerableFailureException { String path = "/files/" + folderId; Response response = sendGetRequest(properties, path); + if (response.code() == 404) throw new TolerableFailureException(); if (response.code() != 200) throw new ApiException(); ResponseBody body = response.body(); @@ -252,9 +254,11 @@ class MailboxApiImpl implements MailboxApi { @Override public void getFile(MailboxProperties properties, MailboxFolderId folderId, - MailboxFileId fileId, File file) throws IOException, ApiException { + MailboxFileId fileId, File file) + throws IOException, ApiException, TolerableFailureException { String path = "/files/" + folderId + "/" + fileId; Response response = sendGetRequest(properties, path); + if (response.code() == 404) throw new TolerableFailureException(); if (response.code() != 200) throw new ApiException(); ResponseBody body = response.body(); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxDownloadWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxDownloadWorker.java index 6489fae07..faf39752f 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxDownloadWorker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxDownloadWorker.java @@ -19,9 +19,7 @@ import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.ThreadSafe; -import static java.util.logging.Level.INFO; import static java.util.logging.Logger.getLogger; -import static org.briarproject.bramble.util.LogUtils.logException; @ThreadSafe @NotNullByDefault @@ -161,9 +159,17 @@ abstract class MailboxDownloadWorker implements MailboxWorker, void downloadNextFile(Queue queue) { synchronized (lock) { if (state == State.DESTROYED) return; - FolderFile file = queue.remove(); - apiCall = mailboxApiCaller.retryWithBackoff( - new SimpleApiCall(() -> apiCallDownloadFile(file, queue))); + if (queue.isEmpty()) { + // Check for files again, as new files may have arrived while + // we were downloading + apiCall = mailboxApiCaller.retryWithBackoff( + createApiCallForDownloadCycle()); + } else { + FolderFile file = queue.remove(); + apiCall = mailboxApiCaller.retryWithBackoff( + new SimpleApiCall(() -> + apiCallDownloadFile(file, queue))); + } } } @@ -182,6 +188,14 @@ abstract class MailboxDownloadWorker implements MailboxWorker, LOG.warning("Failed to delete temporary file"); } throw e; + } catch (TolerableFailureException e) { + // File not found - continue to the next file + LOG.warning("File does not exist"); + if (!tempFile.delete()) { + LOG.warning("Failed to delete temporary file"); + } + downloadNextFile(queue); + return; } mailboxFileManager.handleDownloadedFile(tempFile); deleteFile(file, queue); @@ -204,20 +218,10 @@ abstract class MailboxDownloadWorker implements MailboxWorker, mailboxApi.deleteFile(mailboxProperties, file.folderId, file.fileId); } catch (TolerableFailureException e) { - // Catch this so we can continue to the next file - logException(LOG, INFO, e); - } - if (queue.isEmpty()) { - // Check for files again, as new files may have arrived while we - // were downloading - synchronized (lock) { - if (state == State.DESTROYED) return; - apiCall = mailboxApiCaller.retryWithBackoff( - createApiCallForDownloadCycle()); - } - } else { - downloadNextFile(queue); + // File not found - continue to the next file + LOG.warning("File does not exist"); } + downloadNextFile(queue); } @Override diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorker.java index 40fe296d6..5fb674f72 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxContactListWorker.java @@ -303,7 +303,7 @@ class OwnMailboxContactListWorker mailboxApi.deleteContact(mailboxProperties, c); } catch (TolerableFailureException e) { // Catch this so we can continue to the next update - logException(LOG, INFO, e); + LOG.warning("Contact does not exist"); } updateContactList(); } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxDownloadWorker.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxDownloadWorker.java index 48517f9fa..44afd8ce1 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxDownloadWorker.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/OwnMailboxDownloadWorker.java @@ -5,6 +5,7 @@ import org.briarproject.bramble.api.mailbox.MailboxProperties; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.mailbox.MailboxApi.ApiException; import org.briarproject.bramble.mailbox.MailboxApi.MailboxFile; +import org.briarproject.bramble.mailbox.MailboxApi.TolerableFailureException; import java.io.IOException; import java.util.ArrayList; @@ -85,9 +86,15 @@ class OwnMailboxDownloadWorker extends MailboxDownloadWorker { if (state == State.DESTROYED) return; } LOG.info("Listing folder"); - List files = - mailboxApi.getFiles(mailboxProperties, folder); - if (!files.isEmpty()) available.put(folder, new LinkedList<>(files)); + try { + List files = + mailboxApi.getFiles(mailboxProperties, folder); + if (!files.isEmpty()) { + available.put(folder, new LinkedList<>(files)); + } + } catch (TolerableFailureException e) { + LOG.warning("Folder does not exist"); + } if (queue.isEmpty()) { LOG.info("Finished listing folders"); if (available.isEmpty()) onDownloadCycleFinished(); diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java index df921573b..3b46320c8 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxApiTest.java @@ -630,6 +630,7 @@ public class MailboxApiTest extends BrambleTestCase { server.enqueue(new MockResponse().setBody(invalidResponse3)); server.enqueue(new MockResponse().setBody(invalidResponse4)); server.enqueue(new MockResponse().setResponseCode(401)); + server.enqueue(new MockResponse().setResponseCode(404)); server.enqueue(new MockResponse().setResponseCode(500)); server.start(); String baseUrl = getBaseUrl(server); @@ -706,13 +707,21 @@ public class MailboxApiTest extends BrambleTestCase { assertEquals("GET", request8.getMethod()); assertToken(request8, token); - // 500 internal server error - assertThrows(ApiException.class, - () -> api.getFiles(properties, contactInboxId)); + // 404 not found + assertThrows(TolerableFailureException.class, () -> + api.getFiles(properties, contactInboxId)); RecordedRequest request9 = server.takeRequest(); assertEquals("/files/" + contactInboxId, request9.getPath()); assertEquals("GET", request9.getMethod()); assertToken(request9, token); + + // 500 internal server error + assertThrows(ApiException.class, + () -> api.getFiles(properties, contactInboxId)); + RecordedRequest request10 = server.takeRequest(); + assertEquals("/files/" + contactInboxId, request10.getPath()); + assertEquals("GET", request10.getMethod()); + assertToken(request10, token); } @Test diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxIntegrationTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxIntegrationTest.java index 21c85131b..f1b88946d 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxIntegrationTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxIntegrationTest.java @@ -186,7 +186,7 @@ public class MailboxIntegrationTest extends BrambleTestCase { MailboxFileId fileName1 = files1.get(0).name; // owner can't check files - assertThrows(ApiException.class, () -> + assertThrows(TolerableFailureException.class, () -> api.getFiles(ownerProperties, contact.inboxId)); // contact downloads file @@ -197,12 +197,13 @@ public class MailboxIntegrationTest extends BrambleTestCase { // owner can't download file, even if knowing name File file1forbidden = folder.newFile(); - assertThrows(ApiException.class, () -> api.getFile(ownerProperties, - contact.inboxId, fileName1, file1forbidden)); + assertThrows(TolerableFailureException.class, () -> + api.getFile(ownerProperties, contact.inboxId, fileName1, + file1forbidden)); assertEquals(0, file1forbidden.length()); // owner can't delete file - assertThrows(ApiException.class, () -> + assertThrows(TolerableFailureException.class, () -> api.deleteFile(ownerProperties, contact.inboxId, fileName1)); // contact deletes file @@ -232,7 +233,7 @@ public class MailboxIntegrationTest extends BrambleTestCase { MailboxFileId file3name = files2.get(1).name; // contact can't list files in contact's outbox - assertThrows(ApiException.class, () -> + assertThrows(TolerableFailureException.class, () -> api.getFiles(contactProperties, contact.outboxId)); // owner downloads both files from contact's outbox @@ -252,17 +253,19 @@ public class MailboxIntegrationTest extends BrambleTestCase { // contact can't download files again, even if knowing name File file2forbidden = folder.newFile(); File file3forbidden = folder.newFile(); - assertThrows(ApiException.class, () -> api.getFile(contactProperties, - contact.outboxId, file2name, file2forbidden)); - assertThrows(ApiException.class, () -> api.getFile(contactProperties, - contact.outboxId, file3name, file3forbidden)); + assertThrows(TolerableFailureException.class, () -> + api.getFile(contactProperties, contact.outboxId, file2name, + file2forbidden)); + assertThrows(TolerableFailureException.class, () -> + api.getFile(contactProperties, contact.outboxId, file3name, + file3forbidden)); assertEquals(0, file1forbidden.length()); assertEquals(0, file2forbidden.length()); // contact can't delete files in outbox - assertThrows(ApiException.class, () -> + assertThrows(TolerableFailureException.class, () -> api.deleteFile(contactProperties, contact.outboxId, file2name)); - assertThrows(ApiException.class, () -> + assertThrows(TolerableFailureException.class, () -> api.deleteFile(contactProperties, contact.outboxId, file3name)); // owner deletes files