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.
This commit is contained in:
akwizgran
2022-07-27 15:26:23 +01:00
parent 37ff06d192
commit 5defd500ae
8 changed files with 87 additions and 44 deletions

View File

@@ -5,6 +5,7 @@ import org.briarproject.bramble.api.mailbox.MailboxProperties;
import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
import org.briarproject.bramble.mailbox.MailboxApi.ApiException; import org.briarproject.bramble.mailbox.MailboxApi.ApiException;
import org.briarproject.bramble.mailbox.MailboxApi.MailboxFile; import org.briarproject.bramble.mailbox.MailboxApi.MailboxFile;
import org.briarproject.bramble.mailbox.MailboxApi.TolerableFailureException;
import java.io.IOException; import java.io.IOException;
import java.util.LinkedList; import java.util.LinkedList;
@@ -13,6 +14,7 @@ import java.util.Queue;
import javax.annotation.concurrent.ThreadSafe; import javax.annotation.concurrent.ThreadSafe;
import static java.util.Collections.emptyList;
import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull;
@ThreadSafe @ThreadSafe
@@ -43,8 +45,13 @@ class ContactMailboxDownloadWorker extends MailboxDownloadWorker {
LOG.info("Listing inbox"); LOG.info("Listing inbox");
MailboxFolderId folderId = MailboxFolderId folderId =
requireNonNull(mailboxProperties.getInboxId()); requireNonNull(mailboxProperties.getInboxId());
List<MailboxFile> files = List<MailboxFile> files;
mailboxApi.getFiles(mailboxProperties, folderId); try {
files = mailboxApi.getFiles(mailboxProperties, folderId);
} catch (TolerableFailureException e) {
LOG.warning("Inbox folder does not exist");
files = emptyList();
}
if (files.isEmpty()) { if (files.isEmpty()) {
onDownloadCycleFinished(); onDownloadCycleFinished();
} else { } else {

View File

@@ -91,9 +91,13 @@ interface MailboxApi {
* Used by owner and contacts to list their files to retrieve. * Used by owner and contacts to list their files to retrieve.
* <p> * <p>
* Returns 200 OK with the list of files in JSON. * 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<MailboxFile> getFiles(MailboxProperties properties, List<MailboxFile> getFiles(MailboxProperties properties,
MailboxFolderId folderId) throws IOException, ApiException; MailboxFolderId folderId)
throws IOException, ApiException, TolerableFailureException;
/** /**
* Used by owner and contacts to retrieve a file. * Used by owner and contacts to retrieve a file.
@@ -102,17 +106,22 @@ interface MailboxApi {
* in the response body. * in the response body.
* *
* @param file the empty file the response bytes will be written into. * @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, 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. * Used by owner and contacts to delete files.
* <p> * <p>
* Returns 200 OK (no exception) if deletion was successful. * Returns 200 OK (no exception) if deletion was successful.
* *
* @throws TolerableFailureException on 404 response, * @throws TolerableFailureException if response code is 404 (folder does
* because file was most likely deleted already. * not exist, client is not authorised to download from folder, or file
* does not exist)
*/ */
void deleteFile(MailboxProperties properties, MailboxFolderId folderId, void deleteFile(MailboxProperties properties, MailboxFolderId folderId,
MailboxFileId fileId) MailboxFileId fileId)

View File

@@ -216,9 +216,11 @@ class MailboxApiImpl implements MailboxApi {
@Override @Override
public List<MailboxFile> getFiles(MailboxProperties properties, public List<MailboxFile> getFiles(MailboxProperties properties,
MailboxFolderId folderId) throws IOException, ApiException { MailboxFolderId folderId)
throws IOException, ApiException, TolerableFailureException {
String path = "/files/" + folderId; String path = "/files/" + folderId;
Response response = sendGetRequest(properties, path); Response response = sendGetRequest(properties, path);
if (response.code() == 404) throw new TolerableFailureException();
if (response.code() != 200) throw new ApiException(); if (response.code() != 200) throw new ApiException();
ResponseBody body = response.body(); ResponseBody body = response.body();
@@ -252,9 +254,11 @@ class MailboxApiImpl implements MailboxApi {
@Override @Override
public void getFile(MailboxProperties properties, MailboxFolderId folderId, 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; String path = "/files/" + folderId + "/" + fileId;
Response response = sendGetRequest(properties, path); Response response = sendGetRequest(properties, path);
if (response.code() == 404) throw new TolerableFailureException();
if (response.code() != 200) throw new ApiException(); if (response.code() != 200) throw new ApiException();
ResponseBody body = response.body(); ResponseBody body = response.body();

View File

@@ -19,9 +19,7 @@ import javax.annotation.Nullable;
import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.GuardedBy;
import javax.annotation.concurrent.ThreadSafe; import javax.annotation.concurrent.ThreadSafe;
import static java.util.logging.Level.INFO;
import static java.util.logging.Logger.getLogger; import static java.util.logging.Logger.getLogger;
import static org.briarproject.bramble.util.LogUtils.logException;
@ThreadSafe @ThreadSafe
@NotNullByDefault @NotNullByDefault
@@ -161,9 +159,17 @@ abstract class MailboxDownloadWorker implements MailboxWorker,
void downloadNextFile(Queue<FolderFile> queue) { void downloadNextFile(Queue<FolderFile> queue) {
synchronized (lock) { synchronized (lock) {
if (state == State.DESTROYED) return; if (state == State.DESTROYED) return;
FolderFile file = queue.remove(); if (queue.isEmpty()) {
apiCall = mailboxApiCaller.retryWithBackoff( // Check for files again, as new files may have arrived while
new SimpleApiCall(() -> apiCallDownloadFile(file, queue))); // 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"); LOG.warning("Failed to delete temporary file");
} }
throw e; 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); mailboxFileManager.handleDownloadedFile(tempFile);
deleteFile(file, queue); deleteFile(file, queue);
@@ -204,20 +218,10 @@ abstract class MailboxDownloadWorker implements MailboxWorker,
mailboxApi.deleteFile(mailboxProperties, file.folderId, mailboxApi.deleteFile(mailboxProperties, file.folderId,
file.fileId); file.fileId);
} catch (TolerableFailureException e) { } catch (TolerableFailureException e) {
// Catch this so we can continue to the next file // File not found - continue to the next file
logException(LOG, INFO, e); LOG.warning("File does not exist");
}
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);
} }
downloadNextFile(queue);
} }
@Override @Override

View File

@@ -303,7 +303,7 @@ class OwnMailboxContactListWorker
mailboxApi.deleteContact(mailboxProperties, c); mailboxApi.deleteContact(mailboxProperties, c);
} catch (TolerableFailureException e) { } catch (TolerableFailureException e) {
// Catch this so we can continue to the next update // Catch this so we can continue to the next update
logException(LOG, INFO, e); LOG.warning("Contact does not exist");
} }
updateContactList(); updateContactList();
} }

View File

@@ -5,6 +5,7 @@ import org.briarproject.bramble.api.mailbox.MailboxProperties;
import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
import org.briarproject.bramble.mailbox.MailboxApi.ApiException; import org.briarproject.bramble.mailbox.MailboxApi.ApiException;
import org.briarproject.bramble.mailbox.MailboxApi.MailboxFile; import org.briarproject.bramble.mailbox.MailboxApi.MailboxFile;
import org.briarproject.bramble.mailbox.MailboxApi.TolerableFailureException;
import java.io.IOException; import java.io.IOException;
import java.util.ArrayList; import java.util.ArrayList;
@@ -85,9 +86,15 @@ class OwnMailboxDownloadWorker extends MailboxDownloadWorker {
if (state == State.DESTROYED) return; if (state == State.DESTROYED) return;
} }
LOG.info("Listing folder"); LOG.info("Listing folder");
List<MailboxFile> files = try {
mailboxApi.getFiles(mailboxProperties, folder); List<MailboxFile> files =
if (!files.isEmpty()) available.put(folder, new LinkedList<>(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()) { if (queue.isEmpty()) {
LOG.info("Finished listing folders"); LOG.info("Finished listing folders");
if (available.isEmpty()) onDownloadCycleFinished(); if (available.isEmpty()) onDownloadCycleFinished();

View File

@@ -630,6 +630,7 @@ public class MailboxApiTest extends BrambleTestCase {
server.enqueue(new MockResponse().setBody(invalidResponse3)); server.enqueue(new MockResponse().setBody(invalidResponse3));
server.enqueue(new MockResponse().setBody(invalidResponse4)); server.enqueue(new MockResponse().setBody(invalidResponse4));
server.enqueue(new MockResponse().setResponseCode(401)); server.enqueue(new MockResponse().setResponseCode(401));
server.enqueue(new MockResponse().setResponseCode(404));
server.enqueue(new MockResponse().setResponseCode(500)); server.enqueue(new MockResponse().setResponseCode(500));
server.start(); server.start();
String baseUrl = getBaseUrl(server); String baseUrl = getBaseUrl(server);
@@ -706,13 +707,21 @@ public class MailboxApiTest extends BrambleTestCase {
assertEquals("GET", request8.getMethod()); assertEquals("GET", request8.getMethod());
assertToken(request8, token); assertToken(request8, token);
// 500 internal server error // 404 not found
assertThrows(ApiException.class, assertThrows(TolerableFailureException.class, () ->
() -> api.getFiles(properties, contactInboxId)); api.getFiles(properties, contactInboxId));
RecordedRequest request9 = server.takeRequest(); RecordedRequest request9 = server.takeRequest();
assertEquals("/files/" + contactInboxId, request9.getPath()); assertEquals("/files/" + contactInboxId, request9.getPath());
assertEquals("GET", request9.getMethod()); assertEquals("GET", request9.getMethod());
assertToken(request9, token); 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 @Test

View File

@@ -186,7 +186,7 @@ public class MailboxIntegrationTest extends BrambleTestCase {
MailboxFileId fileName1 = files1.get(0).name; MailboxFileId fileName1 = files1.get(0).name;
// owner can't check files // owner can't check files
assertThrows(ApiException.class, () -> assertThrows(TolerableFailureException.class, () ->
api.getFiles(ownerProperties, contact.inboxId)); api.getFiles(ownerProperties, contact.inboxId));
// contact downloads file // contact downloads file
@@ -197,12 +197,13 @@ public class MailboxIntegrationTest extends BrambleTestCase {
// owner can't download file, even if knowing name // owner can't download file, even if knowing name
File file1forbidden = folder.newFile(); File file1forbidden = folder.newFile();
assertThrows(ApiException.class, () -> api.getFile(ownerProperties, assertThrows(TolerableFailureException.class, () ->
contact.inboxId, fileName1, file1forbidden)); api.getFile(ownerProperties, contact.inboxId, fileName1,
file1forbidden));
assertEquals(0, file1forbidden.length()); assertEquals(0, file1forbidden.length());
// owner can't delete file // owner can't delete file
assertThrows(ApiException.class, () -> assertThrows(TolerableFailureException.class, () ->
api.deleteFile(ownerProperties, contact.inboxId, fileName1)); api.deleteFile(ownerProperties, contact.inboxId, fileName1));
// contact deletes file // contact deletes file
@@ -232,7 +233,7 @@ public class MailboxIntegrationTest extends BrambleTestCase {
MailboxFileId file3name = files2.get(1).name; MailboxFileId file3name = files2.get(1).name;
// contact can't list files in contact's outbox // contact can't list files in contact's outbox
assertThrows(ApiException.class, () -> assertThrows(TolerableFailureException.class, () ->
api.getFiles(contactProperties, contact.outboxId)); api.getFiles(contactProperties, contact.outboxId));
// owner downloads both files from contact's outbox // 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 // contact can't download files again, even if knowing name
File file2forbidden = folder.newFile(); File file2forbidden = folder.newFile();
File file3forbidden = folder.newFile(); File file3forbidden = folder.newFile();
assertThrows(ApiException.class, () -> api.getFile(contactProperties, assertThrows(TolerableFailureException.class, () ->
contact.outboxId, file2name, file2forbidden)); api.getFile(contactProperties, contact.outboxId, file2name,
assertThrows(ApiException.class, () -> api.getFile(contactProperties, file2forbidden));
contact.outboxId, file3name, file3forbidden)); assertThrows(TolerableFailureException.class, () ->
api.getFile(contactProperties, contact.outboxId, file3name,
file3forbidden));
assertEquals(0, file1forbidden.length()); assertEquals(0, file1forbidden.length());
assertEquals(0, file2forbidden.length()); assertEquals(0, file2forbidden.length());
// contact can't delete files in outbox // contact can't delete files in outbox
assertThrows(ApiException.class, () -> assertThrows(TolerableFailureException.class, () ->
api.deleteFile(contactProperties, contact.outboxId, file2name)); api.deleteFile(contactProperties, contact.outboxId, file2name));
assertThrows(ApiException.class, () -> assertThrows(TolerableFailureException.class, () ->
api.deleteFile(contactProperties, contact.outboxId, file3name)); api.deleteFile(contactProperties, contact.outboxId, file3name));
// owner deletes files // owner deletes files