From 444e7365c972d79f54553b841d4a212a52f44a4e Mon Sep 17 00:00:00 2001 From: Konrad Pozniak Date: Tue, 3 May 2022 19:12:35 +0200 Subject: [PATCH] fix race condition where multiple uploaded media can get same internal id (#2479) * fix race condition where multiple uploaded media can get same internal id * atomically update media stateflow * atomically update media stateflow --- .../components/compose/ComposeActivity.kt | 24 +++-- .../components/compose/ComposeViewModel.kt | 99 +++++++++++-------- 2 files changed, 72 insertions(+), 51 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeActivity.kt b/app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeActivity.kt index 48e14def5..321626147 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeActivity.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeActivity.kt @@ -51,6 +51,7 @@ import androidx.core.view.ContentInfoCompat import androidx.core.view.OnReceiveContentListener import androidx.core.view.isGone import androidx.core.view.isVisible +import androidx.lifecycle.asLiveData import androidx.lifecycle.lifecycleScope import androidx.preference.PreferenceManager import androidx.recyclerview.widget.LinearLayoutManager @@ -344,14 +345,17 @@ class ComposeActivity : viewModel.statusVisibility.observe { visibility -> setStatusVisibility(visibility) } - viewModel.media.observe { media -> - mediaAdapter.submitList(media) - if (media.size != mediaCount) { - mediaCount = media.size - binding.composeMediaPreviewBar.visible(media.isNotEmpty()) - updateSensitiveMediaToggle(viewModel.markMediaAsSensitive.value != false, viewModel.showContentWarning.value != false) + lifecycleScope.launch { + viewModel.media.collect { media -> + mediaAdapter.submitList(media) + if (media.size != mediaCount) { + mediaCount = media.size + binding.composeMediaPreviewBar.visible(media.isNotEmpty()) + updateSensitiveMediaToggle(viewModel.markMediaAsSensitive.value != false, viewModel.showContentWarning.value != false) + } } } + viewModel.poll.observe { poll -> binding.pollPreview.visible(poll != null) poll?.let(binding.pollPreview::setPoll) @@ -364,7 +368,7 @@ class ComposeActivity : } updateScheduleButton() } - combineOptionalLiveData(viewModel.media, viewModel.poll) { media, poll -> + combineOptionalLiveData(viewModel.media.asLiveData(), viewModel.poll) { media, poll -> val active = poll == null && media!!.size != 4 && (media.isEmpty() || media.first().type == QueuedMedia.Type.IMAGE) @@ -781,11 +785,11 @@ class ComposeActivity : spoilerText = binding.composeContentWarningField.text.toString() } val characterCount = calculateTextLength() - if ((characterCount <= 0 || contentText.isBlank()) && viewModel.media.value!!.isEmpty()) { + if ((characterCount <= 0 || contentText.isBlank()) && viewModel.media.value.isEmpty()) { binding.composeEditField.error = getString(R.string.error_empty) enableButtons(true) } else if (characterCount <= maximumTootCharacters) { - if (viewModel.media.value!!.isNotEmpty()) { + if (viewModel.media.value.isNotEmpty()) { finishingUploadDialog = ProgressDialog.show( this, getString(R.string.dialog_title_finishing_media_upload), getString(R.string.dialog_message_uploading_media), true, true @@ -983,7 +987,7 @@ class ComposeActivity : } data class QueuedMedia( - val localId: Long, + val localId: Int, val uri: Uri, val type: Type, val mediaSize: Long, diff --git a/app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeViewModel.kt index 81500ee49..7b1805327 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeViewModel.kt @@ -21,6 +21,7 @@ import androidx.core.net.toUri import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import androidx.lifecycle.ViewModel +import androidx.lifecycle.asLiveData import androidx.lifecycle.viewModelScope import com.keylesspalace.tusky.components.compose.ComposeActivity.QueuedMedia import com.keylesspalace.tusky.components.drafts.DraftHelper @@ -36,15 +37,17 @@ import com.keylesspalace.tusky.network.MastodonApi import com.keylesspalace.tusky.service.ServiceClient import com.keylesspalace.tusky.service.StatusToSend import com.keylesspalace.tusky.util.combineLiveData -import com.keylesspalace.tusky.util.filter -import com.keylesspalace.tusky.util.map import com.keylesspalace.tusky.util.randomAlphanumericString import com.keylesspalace.tusky.util.toLiveData -import com.keylesspalace.tusky.util.withoutFirstWhich import io.reactivex.rxjava3.core.Observable import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.catch +import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.update +import kotlinx.coroutines.flow.updateAndGet import kotlinx.coroutines.launch import kotlinx.coroutines.rx3.rxSingle import kotlinx.coroutines.withContext @@ -84,10 +87,10 @@ class ComposeViewModel @Inject constructor( val poll: MutableLiveData = mutableLiveData(null) val scheduledAt: MutableLiveData = mutableLiveData(null) - val media = mutableLiveData>(listOf()) + val media: MutableStateFlow> = MutableStateFlow(emptyList()) val uploadError = MutableLiveData() - private val mediaToJob = mutableMapOf() + private val mediaToJob = mutableMapOf() private val isEditingScheduledToot get() = !scheduledTootId.isNullOrEmpty() @@ -103,7 +106,7 @@ class ComposeViewModel @Inject constructor( suspend fun pickMedia(mediaUri: Uri, description: String? = null): Result = withContext(Dispatchers.IO) { try { val (type, uri, size) = mediaUploader.prepareMedia(mediaUri) - val mediaItems = media.value!! + val mediaItems = media.value if (type != QueuedMedia.Type.IMAGE && mediaItems.isNotEmpty() && mediaItems[0].type == QueuedMedia.Type.IMAGE @@ -118,29 +121,31 @@ class ComposeViewModel @Inject constructor( } } - private fun addMediaToQueue( + private suspend fun addMediaToQueue( type: QueuedMedia.Type, uri: Uri, mediaSize: Long, description: String? = null ): QueuedMedia { - val mediaItem = QueuedMedia( - localId = System.currentTimeMillis(), - uri = uri, - type = type, - mediaSize = mediaSize, - description = description - ) - media.postValue(media.value!! + mediaItem) + val mediaItem = media.updateAndGet { mediaValue -> + val mediaItem = QueuedMedia( + localId = (mediaValue.maxOfOrNull { it.localId } ?: 0) + 1, + uri = uri, + type = type, + mediaSize = mediaSize, + description = description + ) + mediaValue + mediaItem + }.last() mediaToJob[mediaItem.localId] = viewModelScope.launch { mediaUploader .uploadMedia(mediaItem) .catch { error -> - media.postValue(media.value?.filter { it.localId != mediaItem.localId } ?: emptyList()) + media.update { mediaValue -> mediaValue.filter { it.localId != mediaItem.localId } } uploadError.postValue(error) } .collect { event -> - val item = media.value?.find { it.localId == mediaItem.localId } + val item = media.value.find { it.localId == mediaItem.localId } ?: return@collect val newMediaItem = when (event) { is UploadEvent.ProgressEvent -> @@ -148,16 +153,14 @@ class ComposeViewModel @Inject constructor( is UploadEvent.FinishedEvent -> item.copy(id = event.mediaId, uploadPercent = -1) } - synchronized(media) { - val mediaValue = media.value!! - val index = mediaValue.indexOfFirst { it.localId == newMediaItem.localId } - media.postValue( - if (index == -1) { - mediaValue + newMediaItem + media.update { mediaValue -> + mediaValue.map { mediaItem -> + if (mediaItem.localId == newMediaItem.localId) { + newMediaItem } else { - mediaValue.toMutableList().also { it[index] = newMediaItem } + mediaItem } - ) + } } } } @@ -165,13 +168,23 @@ class ComposeViewModel @Inject constructor( } private fun addUploadedMedia(id: String, type: QueuedMedia.Type, uri: Uri, description: String?) { - val mediaItem = QueuedMedia(System.currentTimeMillis(), uri, type, 0, -1, id, description) - media.value = media.value!! + mediaItem + media.update { mediaValue -> + val mediaItem = QueuedMedia( + localId = (mediaValue.maxOfOrNull { it.localId } ?: 0) + 1, + uri = uri, + type = type, + mediaSize = 0, + uploadPercent = -1, + id = id, + description = description + ) + mediaValue + mediaItem + } } fun removeMediaFromQueue(item: QueuedMedia) { mediaToJob[item.localId]?.cancel() - media.value = media.value!!.withoutFirstWhich { it.localId == item.localId } + media.update { mediaValue -> mediaValue.filter { it.localId == item.localId } } } fun toggleMarkSensitive() { @@ -211,7 +224,7 @@ class ComposeViewModel @Inject constructor( viewModelScope.launch { val mediaUris: MutableList = mutableListOf() val mediaDescriptions: MutableList = mutableListOf() - media.value?.forEach { item -> + media.value.forEach { item -> mediaUris.add(item.uri.toString()) mediaDescriptions.add(item.description) } @@ -248,14 +261,14 @@ class ComposeViewModel @Inject constructor( Observable.just(Unit) }.toLiveData() - val sendObservable = media + val sendFlow = media .filter { items -> items.all { it.uploadPercent == -1 } } .map { val mediaIds: MutableList = mutableListOf() val mediaUris: MutableList = mutableListOf() val mediaDescriptions: MutableList = mutableListOf() val mediaProcessed: MutableList = mutableListOf() - for (item in media.value!!) { + for (item in media.value) { mediaIds.add(item.id!!) mediaUris.add(item.uri) mediaDescriptions.add(item.description ?: "") @@ -285,17 +298,21 @@ class ComposeViewModel @Inject constructor( serviceClient.sendToot(tootToSend) } - return combineLiveData(deletionObservable, sendObservable) { _, _ -> } + return combineLiveData(deletionObservable, sendFlow.asLiveData()) { _, _ -> } } - suspend fun updateDescription(localId: Long, description: String): Boolean { - val newList = media.value!!.toMutableList() - val index = newList.indexOfFirst { it.localId == localId } - if (index != -1) { - newList[index] = newList[index].copy(description = description) + suspend fun updateDescription(localId: Int, description: String): Boolean { + val newMediaList = media.updateAndGet { mediaValue -> + mediaValue.map { mediaItem -> + if (mediaItem.localId == localId) { + mediaItem.copy(description = description) + } else { + mediaItem + } + } } - media.value = newList - val updatedItem = newList.find { it.localId == localId } + + val updatedItem = newMediaList.find { it.localId == localId } if (updatedItem?.id != null) { return api.updateMedia(updatedItem.id, description) .fold({ @@ -387,8 +404,8 @@ class ComposeViewModel @Inject constructor( val draftAttachments = composeOptions?.draftAttachments if (draftAttachments != null) { // when coming from DraftActivity - draftAttachments.forEach { attachment -> - viewModelScope.launch { + viewModelScope.launch { + draftAttachments.forEach { attachment -> pickMedia(attachment.uri, attachment.description) } }