Check for concurrent cache updates.

briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java

@@ -135,10 +135,6 @@ public class AttachmentItem implements Parcelable {
 		return toHexString(instanceId);
 	}
 
-	boolean hasSize() {
-		return width != 0 && height != 0;
-	}
-
 	@Override
 	public int describeContents() {
 		return 0;
@@ -156,6 +152,10 @@ public class AttachmentItem implements Parcelable {
 		dest.writeString(state.name());
 	}
 
+	/**
+	 * This is used to identity if two items are the same,
+	 * irrespective of their state or size.
+	 */
 	@Override
 	public boolean equals(@Nullable Object o) {
 		return o instanceof AttachmentItem &&
@@ -164,4 +164,8 @@ public class AttachmentItem implements Parcelable {
 				);
 	}
 
+	@Override
+	public int hashCode() {
+		return header.getMessageId().hashCode();
+	}
 }

briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java

@@ -49,6 +49,13 @@ public interface AttachmentRetriever {
 	 * Loads an {@link AttachmentItem}
 	 * that arrived via an {@link AttachmentReceivedEvent}
 	 * and notifies the associated {@link LiveData}.
+	 *
+	 * Note that you need to call {@link #getAttachmentItems(PrivateMessageHeader)}
+	 * first to get the LiveData.
+	 *
+	 * It is possible that no LiveData is available,
+	 * because the message of the AttachmentItem did not arrive, yet.
+	 * In this case, the load wil be deferred until the message arrives.
 	 */
 	@DatabaseExecutor
 	void loadAttachmentItem(MessageId attachmentId);

briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java

@@ -15,8 +15,8 @@ import java.io.BufferedInputStream;
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.Executor;
 import java.util.logging.Logger;
 
@@ -50,9 +50,9 @@ class AttachmentRetrieverImpl implements AttachmentRetriever {
 	private final int minWidth, maxWidth;
 	private final int minHeight, maxHeight;
 
-	private final Map<MessageId, MutableLiveData<AttachmentItem>>
+	private final ConcurrentMap<MessageId, MutableLiveData<AttachmentItem>>
 			itemsWithSize = new ConcurrentHashMap<>();
-	private final Map<MessageId, MutableLiveData<AttachmentItem>>
+	private final ConcurrentMap<MessageId, MutableLiveData<AttachmentItem>>
 			itemsWithoutSize = new ConcurrentHashMap<>();
 
 	@Inject
@@ -99,15 +99,25 @@ class AttachmentRetrieverImpl implements AttachmentRetriever {
 			if (liveData == null) {
 				AttachmentItem item = new AttachmentItem(h,
 						defaultSize, defaultSize, LOADING);
-				final MutableLiveData<AttachmentItem> finalLiveData =
-						new MutableLiveData<>(item);
-				// kick-off loading of attachment, will post to live data
-				dbExecutor.execute(
-						() -> loadAttachmentItem(h, needsSize, finalLiveData));
-				// add new LiveData to cache
-				liveData = finalLiveData;
-				if (needsSize) itemsWithSize.put(h.getMessageId(), liveData);
-				else itemsWithoutSize.put(h.getMessageId(), liveData);
+				liveData = new MutableLiveData<>(item);
+				// add new LiveData to cache, checking for concurrent updates
+				MutableLiveData<AttachmentItem> oldLiveData;
+				if (needsSize) {
+					oldLiveData = itemsWithSize.putIfAbsent(h.getMessageId(),
+							liveData);
+				} else {
+					oldLiveData = itemsWithoutSize.putIfAbsent(h.getMessageId(),
+							liveData);
+				}
+				if (oldLiveData == null) {
+					// kick-off loading of attachment, will post to live data
+					MutableLiveData<AttachmentItem> finalLiveData = liveData;
+					dbExecutor.execute(() ->
+							loadAttachmentItem(h, needsSize, finalLiveData));
+				} else {
+					// Concurrent cache update - use the existing live data
+					liveData = oldLiveData;
+				}
 			}
 			items.add(liveData);
 		}
@@ -118,12 +128,15 @@ class AttachmentRetrieverImpl implements AttachmentRetriever {
 	@DatabaseExecutor
 	public void cacheAttachmentItemWithSize(MessageId conversationMessageId,
 			AttachmentHeader h) throws DbException {
+		// If a live data is already cached we don't need to do anything
+		if (itemsWithSize.containsKey(h.getMessageId())) return;
 		try {
 			Attachment a = messagingManager.getAttachment(h);
 			AttachmentItem item = createAttachmentItem(a, true);
 			MutableLiveData<AttachmentItem> liveData =
 					new MutableLiveData<>(item);
-			itemsWithSize.put(h.getMessageId(), liveData);
+			// If a live data was concurrently cached, don't replace it
+			itemsWithSize.putIfAbsent(h.getMessageId(), liveData);
 		} catch (NoSuchMessageException e) {
 			LOG.info("Attachment not received yet");
 		}

briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java

@@ -1,36 +0,0 @@
-package org.briarproject.briar.android.attachment;
-
-import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
-import org.briarproject.bramble.api.sync.MessageId;
-import org.briarproject.briar.api.messaging.AttachmentHeader;
-
-import javax.annotation.concurrent.Immutable;
-
-@Immutable
-@NotNullByDefault
-class UnavailableItem {
-
-	private final MessageId conversationMessageId;
-	private final AttachmentHeader header;
-	private final boolean needsSize;
-
-	UnavailableItem(MessageId conversationMessageId,
-			AttachmentHeader header, boolean needsSize) {
-		this.conversationMessageId = conversationMessageId;
-		this.header = header;
-		this.needsSize = needsSize;
-	}
-
-	MessageId getConversationMessageId() {
-		return conversationMessageId;
-	}
-
-	AttachmentHeader getHeader() {
-		return header;
-	}
-
-	boolean needsSize() {
-		return needsSize;
-	}
-
-}

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java

@@ -640,16 +640,14 @@ public class ConversationActivity extends BriarActivity
 				&& adapter.isScrolledToBottom(layoutManager);
 	}
 
+	@UiThread
 	private void updateMessageAttachment(MessageId m, AttachmentItem item) {
-		runOnUiThreadUnlessDestroyed(() -> {
-			Pair<Integer, ConversationMessageItem> pair =
-					adapter.getMessageItem(m);
-			if (pair != null && pair.getSecond().updateAttachments(item)) {
-				boolean scroll = shouldScrollWhenUpdatingMessage();
-				adapter.notifyItemChanged(pair.getFirst());
-				if (scroll) scrollToBottom();
-			}
-		});
+		Pair<Integer, ConversationMessageItem> pair = adapter.getMessageItem(m);
+		if (pair != null && pair.getSecond().updateAttachments(item)) {
+			boolean scroll = shouldScrollWhenUpdatingMessage();
+			adapter.notifyItemChanged(pair.getFirst());
+			if (scroll) scrollToBottom();
+		}
 	}
 
 	@Override

briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java

@@ -59,7 +59,7 @@ class ImageAdapter extends Adapter<ImageViewHolder> {
 		// get item
 		requireNonNull(conversationItem);
 		AttachmentItem item = items.get(position);
-		// set onClick listener, if not missing or error
+		// set onClick listener
 		imageViewHolder.itemView.setOnClickListener(v ->
 				listener.onAttachmentClicked(v, conversationItem, item)
 		);

briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java

@@ -25,7 +25,6 @@ import org.briarproject.bramble.api.sync.Message;
 import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.bramble.api.sync.MessageStatus;
 import org.briarproject.bramble.api.sync.validation.IncomingMessageHook;
-import org.briarproject.bramble.api.system.Scheduler;
 import org.briarproject.bramble.api.versioning.ClientVersioningManager;
 import org.briarproject.bramble.api.versioning.ClientVersioningManager.ClientVersioningHook;
 import org.briarproject.briar.api.client.MessageTracker;
@@ -53,20 +52,14 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.logging.Logger;
 
 import javax.annotation.concurrent.Immutable;
 import javax.inject.Inject;
 
 import static java.util.Collections.emptyList;
-import static java.util.concurrent.TimeUnit.SECONDS;
-import static java.util.logging.Level.WARNING;
-import static java.util.logging.Logger.getLogger;
 import static org.briarproject.bramble.api.sync.SyncConstants.MAX_MESSAGE_BODY_LENGTH;
 import static org.briarproject.bramble.api.sync.validation.MessageState.DELIVERED;
 import static org.briarproject.bramble.util.IoUtils.copyAndClose;
-import static org.briarproject.bramble.util.LogUtils.logException;
 import static org.briarproject.briar.client.MessageTrackerConstants.MSG_KEY_READ;
 import static org.briarproject.briar.messaging.MessageTypes.ATTACHMENT;
 import static org.briarproject.briar.messaging.MessageTypes.PRIVATE_MESSAGE;
@@ -85,10 +78,6 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook,
 		ConversationClient, OpenDatabaseHook, ContactHook,
 		ClientVersioningHook {
 
-	private static final Logger LOG =
-			getLogger(MessagingManagerImpl.class.getName());
-
-	private final ScheduledExecutorService scheduler;
 	private final DatabaseComponent db;
 	private final ClientHelper clientHelper;
 	private final MetadataParser metadataParser;
@@ -97,12 +86,10 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook,
 	private final ContactGroupFactory contactGroupFactory;
 
 	@Inject
-	MessagingManagerImpl(@Scheduler ScheduledExecutorService scheduler,
-			DatabaseComponent db, ClientHelper clientHelper,
+	MessagingManagerImpl(DatabaseComponent db, ClientHelper clientHelper,
 			ClientVersioningManager clientVersioningManager,
 			MetadataParser metadataParser, MessageTracker messageTracker,
 			ContactGroupFactory contactGroupFactory) {
-		this.scheduler = scheduler;
 		this.db = db;
 		this.clientHelper = clientHelper;
 		this.metadataParser = metadataParser;
@@ -258,20 +245,9 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook,
 				meta.put(MSG_KEY_ATTACHMENT_HEADERS, headers);
 			}
 			// Mark attachments as shared and permanent now we're ready to send
-			// FIXME: Revert
-			int i = 15;
 			for (AttachmentHeader a : m.getAttachmentHeaders()) {
-				scheduler.schedule(() -> {
-					try {
-						db.transaction(false, txn1 -> {
-							db.setMessageShared(txn1, a.getMessageId());
-							db.setMessagePermanent(txn1, a.getMessageId());
-						});
-					} catch (DbException e) {
-						logException(LOG, WARNING, e);
-					}
-				}, i, SECONDS);
-				i *= 2;
+				db.setMessageShared(txn, a.getMessageId());
+				db.setMessagePermanent(txn, a.getMessageId());
 			}
 			clientHelper.addLocalMessage(txn, m.getMessage(), meta, true,
 					false);