Handle concurrent updates of our avatar

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
+		// save/send avatar and delete old one
-		db.transaction(false, txn2 -> {
+		return db.transactionWithResult(false, txn2 -> {
-			if (latest != null) {
+			// re-query latest update as it might have changed since last query
-				// delete previous update
+			LatestUpdate newLatest = findLatest(txn2, groupId);
-				db.deleteMessage(txn2, latest.messageId);
+			if (newLatest != null && newLatest.version > version) {
-				db.deleteMessageMetadata(txn2, latest.messageId);
+				// 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

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