From ec972e8a1d056799bfc07b90f9f5622e7f13aebe Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 23 Nov 2020 14:26:16 -0300 Subject: [PATCH] Handle concurrent updates of our avatar --- .../briar/avatar/AvatarManagerImpl.java | 24 ++++++----- .../briar/avatar/AvatarManagerImplTest.java | 40 ++++++++++++++++++- 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/briar-core/src/main/java/org/briarproject/briar/avatar/AvatarManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/avatar/AvatarManagerImpl.java index 5258458f4..c69c3390f 100644 --- a/briar-core/src/main/java/org/briarproject/briar/avatar/AvatarManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/avatar/AvatarManagerImpl.java @@ -175,8 +175,6 @@ class AvatarManagerImpl implements AvatarManager, OpenDatabaseHook, ContactHook, Transaction txn = db.startTransaction(true); try { groupId = getOurGroup(txn).getId(); - // TODO this might not be the latest anymore at the end of this method - // Can we run everything in one transaction? Probably not. latest = findLatest(txn, groupId); db.commitTransaction(txn); } finally { @@ -184,7 +182,6 @@ class AvatarManagerImpl implements AvatarManager, OpenDatabaseHook, ContactHook, } long version = latest == null ? 0 : latest.version + 1; // 0.0: Message Type, Version, Content-Type - // TODO do we need to add the message type explicitly? BdfList list = BdfList.of(MSG_TYPE_UPDATE, version, contentType); byte[] descriptor = clientHelper.toByteArray(list); // add BdfList and stream content to body @@ -202,16 +199,23 @@ class AvatarManagerImpl implements AvatarManager, OpenDatabaseHook, ContactHook, meta.put(MSG_KEY_VERSION, version); meta.put(MSG_KEY_CONTENT_TYPE, contentType); meta.put(MSG_KEY_DESCRIPTOR_LENGTH, descriptor.length); - // send message - db.transaction(false, txn2 -> { - if (latest != null) { - // delete previous update - db.deleteMessage(txn2, latest.messageId); - db.deleteMessageMetadata(txn2, latest.messageId); + // save/send avatar and delete old one + return db.transactionWithResult(false, txn2 -> { + // re-query latest update as it might have changed since last query + LatestUpdate newLatest = findLatest(txn2, groupId); + if (newLatest != null && newLatest.version > version) { + // latest update is newer than our own + // no need to store or delete anything, just return latest + return new AttachmentHeader(newLatest.messageId, + newLatest.contentType); + } else if (newLatest != null) { + // delete latest update if it has the same or lower version + db.deleteMessage(txn2, newLatest.messageId); + db.deleteMessageMetadata(txn2, newLatest.messageId); } clientHelper.addLocalMessage(txn2, m, meta, true, false); + return new AttachmentHeader(m.getId(), contentType); }); - return new AttachmentHeader(m.getId(), contentType); } @Nullable diff --git a/briar-core/src/test/java/org/briarproject/briar/avatar/AvatarManagerImplTest.java b/briar-core/src/test/java/org/briarproject/briar/avatar/AvatarManagerImplTest.java index 9a0436634..53168f763 100644 --- a/briar-core/src/test/java/org/briarproject/briar/avatar/AvatarManagerImplTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/avatar/AvatarManagerImplTest.java @@ -294,7 +294,7 @@ public class AvatarManagerImplTest extends BrambleMockTestCase { oneOf(clientHelper).createMessage(with(equal(localGroupId)), with(equal(now)), with(any(byte[].class))); will(returnValue(newMsg)); - oneOf(db).transaction(with(false), withDbRunnable(txn2)); + oneOf(db).transactionWithResult(with(false), withDbCallable(txn2)); oneOf(db).deleteMessage(txn2, ourMsg.getId()); oneOf(db).deleteMessageMetadata(txn2, ourMsg.getId()); oneOf(clientHelper) @@ -302,6 +302,7 @@ public class AvatarManagerImplTest extends BrambleMockTestCase { }}); expectGetOurGroup(txn); expectFindLatest(txn, localGroupId, ourMsg.getId(), metaDict); + expectFindLatest(txn2, localGroupId, ourMsg.getId(), metaDict); AttachmentHeader header = avatarManager.addAvatar(contentType, inputStream); @@ -309,6 +310,43 @@ public class AvatarManagerImplTest extends BrambleMockTestCase { assertEquals(contentType, header.getContentType()); } + @Test + public void testAddAvatarConcurrently() throws Exception { + byte[] avatarBytes = getRandomBytes(42); + InputStream inputStream = new ByteArrayInputStream(avatarBytes); + Transaction txn = new Transaction(null, true); + Transaction txn2 = new Transaction(null, false); + long latestVersion = metaDict.getLong(MSG_KEY_VERSION); + Message newMsg = getMessage(localGroupId); + BdfDictionary newMeta = BdfDictionary.of( + new BdfEntry(MSG_KEY_VERSION, latestVersion + 2), + new BdfEntry(MSG_KEY_CONTENT_TYPE, contentType), + new BdfEntry(MSG_KEY_DESCRIPTOR_LENGTH, 0) + ); + context.checking(new DbExpectations() {{ + oneOf(db).startTransaction(true); + will(returnValue(txn)); + oneOf(db).commitTransaction(txn); + oneOf(db).endTransaction(txn); + oneOf(clientHelper).toByteArray(with(any(BdfList.class))); + oneOf(clock).currentTimeMillis(); + oneOf(clientHelper).createMessage(with(equal(localGroupId)), + with(any(long.class)), with(any(byte[].class))); + will(returnValue(newMsg)); + oneOf(db).transactionWithResult(with(false), withDbCallable(txn2)); + // no deletion or storing happening + }}); + expectGetOurGroup(txn); + expectFindLatest(txn, localGroupId, ourMsg.getId(), metaDict); + // second query for latest update returns higher version + expectFindLatest(txn2, localGroupId, ourMsg.getId(), newMeta); + + AttachmentHeader header = + avatarManager.addAvatar(contentType, inputStream); + assertEquals(ourMsg.getId(), header.getMessageId()); + assertEquals(contentType, header.getContentType()); + } + private void expectGetContactId(Transaction txn, GroupId groupId, ContactId contactId) throws DbException, FormatException { BdfDictionary d = BdfDictionary