From 0bef9f47dafce18f2d804447780206781a03f0cb Mon Sep 17 00:00:00 2001 From: Marino Meneghel Date: Wed, 18 Aug 2021 14:33:18 +0200 Subject: [PATCH 1/4] Reduce time between "send message" operation retries When any step of the "Send message" operation fails, an exponential backoff policy was applied. In the worst case scenario (each of the possible steps of the sending process failing each possibile attempt) this "retry policy" would imply long delays and cause the send operation to become too long (and the user to lose the context of the ongoing operation). This commit reduces the number of retries for the "save draft" and "upload attachments" operations (both part of the sending process) to 1, the one for "send message" to 2 and the backoff delay to 10 seconds for all ops. MAILAND-2293 --- .../ch/protonmail/android/attachments/UploadAttachments.kt | 4 ++-- .../ch/protonmail/android/compose/send/SendMessageWorker.kt | 6 +++--- .../protonmail/android/worker/drafts/CreateDraftWorker.kt | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/ch/protonmail/android/attachments/UploadAttachments.kt b/app/src/main/java/ch/protonmail/android/attachments/UploadAttachments.kt index 9b58009f3..fcaca0cf9 100644 --- a/app/src/main/java/ch/protonmail/android/attachments/UploadAttachments.kt +++ b/app/src/main/java/ch/protonmail/android/attachments/UploadAttachments.kt @@ -55,7 +55,7 @@ internal const val KEY_INPUT_UPLOAD_ATTACHMENTS_IS_MESSAGE_SENDING = "keyUploadA internal const val KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR = "keyUploadAttachmentResultError" private const val UPLOAD_ATTACHMENTS_WORK_NAME_PREFIX = "uploadAttachmentUniqueWorkName" -private const val UPLOAD_ATTACHMENTS_MAX_RETRIES = 3 +private const val UPLOAD_ATTACHMENTS_MAX_RETRIES = 1 class UploadAttachments @WorkerInject constructor( @Assisted context: Context, @@ -214,7 +214,7 @@ class UploadAttachments @WorkerInject constructor( KEY_INPUT_UPLOAD_ATTACHMENTS_IS_MESSAGE_SENDING to isMessageSending ) ) - .setBackoffCriteria(BackoffPolicy.EXPONENTIAL, 2 * TEN_SECONDS, TimeUnit.SECONDS) + .setBackoffCriteria(BackoffPolicy.LINEAR, TEN_SECONDS, TimeUnit.SECONDS) .build() workManager.enqueueUniqueWork( diff --git a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt index a986ee0f2..0fc3950ee 100644 --- a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt +++ b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt @@ -89,7 +89,7 @@ internal const val KEY_INPUT_SEND_MESSAGE_SECURITY_OPTIONS_SERIALIZED = "keySend internal const val KEY_OUTPUT_RESULT_SEND_MESSAGE_ERROR_ENUM = "keySendMessageErrorResult" private const val INPUT_MESSAGE_DB_ID_NOT_FOUND = -1L -private const val SEND_MESSAGE_MAX_RETRIES = 3 +private const val SEND_MESSAGE_MAX_RETRIES = 2 private const val NO_CONTACTS_AUTO_SAVE = 0 private const val SEND_MESSAGE_WORK_NAME_PREFIX = "sendMessageUniqueWorkName" private const val NO_SUBJECT = "" @@ -359,11 +359,11 @@ class SendMessageWorker @WorkerInject constructor( KEY_INPUT_SEND_MESSAGE_SECURITY_OPTIONS_SERIALIZED to securityOptions.serialize() ) ) - .setBackoffCriteria(BackoffPolicy.EXPONENTIAL, 2 * TEN_SECONDS, TimeUnit.SECONDS) + .setBackoffCriteria(BackoffPolicy.LINEAR, TEN_SECONDS, TimeUnit.SECONDS) .build() workManager.enqueueUniqueWork( - "${SEND_MESSAGE_WORK_NAME_PREFIX}-${requireNotNull(message.messageId)}", + "$SEND_MESSAGE_WORK_NAME_PREFIX-${requireNotNull(message.messageId)}", ExistingWorkPolicy.REPLACE, sendMessageRequest ) diff --git a/app/src/main/java/ch/protonmail/android/worker/drafts/CreateDraftWorker.kt b/app/src/main/java/ch/protonmail/android/worker/drafts/CreateDraftWorker.kt index bf224a8a2..d9dfd371a 100644 --- a/app/src/main/java/ch/protonmail/android/worker/drafts/CreateDraftWorker.kt +++ b/app/src/main/java/ch/protonmail/android/worker/drafts/CreateDraftWorker.kt @@ -77,7 +77,7 @@ internal const val KEY_OUTPUT_RESULT_SAVE_DRAFT_ERROR_ENUM = "keySaveDraftErrorR internal const val KEY_OUTPUT_RESULT_SAVE_DRAFT_MESSAGE_ID = "keySaveDraftSuccessResultDbId" private const val INPUT_MESSAGE_DB_ID_NOT_FOUND = -1L -private const val SAVE_DRAFT_MAX_RETRIES = 3 +private const val SAVE_DRAFT_MAX_RETRIES = 1 class CreateDraftWorker @WorkerInject constructor( @Assisted context: Context, @@ -336,7 +336,7 @@ class CreateDraftWorker @WorkerInject constructor( KEY_INPUT_SAVE_DRAFT_PREV_SENDER_ADDR_ID to previousSenderAddressId ) ) - .setBackoffCriteria(BackoffPolicy.EXPONENTIAL, 2 * TEN_SECONDS, TimeUnit.SECONDS) + .setBackoffCriteria(BackoffPolicy.LINEAR, TEN_SECONDS, TimeUnit.SECONDS) .build() val uniqueWorkId = "$SAVE_DRAFT_UNIQUE_WORK_ID_PREFIX-${message.messageId}" From 2de3bf021ebe38998f31ba0edb6c4c8654a570b1 Mon Sep 17 00:00:00 2001 From: Marino Meneghel Date: Wed, 18 Aug 2021 15:38:03 +0200 Subject: [PATCH 2/4] Avoid cancellation of Send message API request and its handling This is to prevent cases of the worker being cancelled after the API request was fired and before the response was handled, which might leave the user with some inconsistent state (eg. the message might be sent but not be moved in the sent folder locally) MAILAND-2293 --- .../protonmail/android/compose/send/SendMessageWorker.kt | 8 ++++++-- .../android/attachments/UploadAttachmentsTest.kt | 8 ++++---- .../android/compose/send/SendMessageWorkerTest.kt | 6 +++--- .../ch/protonmail/android/worker/CreateDraftWorkerTest.kt | 6 +++--- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt index 0fc3950ee..ea3850b90 100644 --- a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt +++ b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt @@ -69,7 +69,9 @@ import ch.protonmail.android.usecase.compose.SaveDraft import ch.protonmail.android.usecase.compose.SaveDraftResult import ch.protonmail.android.utils.notifier.UserNotifier import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.withContext import me.proton.core.util.kotlin.deserialize import me.proton.core.util.kotlin.serialize import timber.log.Timber @@ -148,8 +150,10 @@ class SendMessageWorker @WorkerInject constructor( } return try { - val response = apiManager.sendMessage(messageId, requestBody, RetrofitTag(username)) - handleSendMessageResponse(messageId, response, savedDraftMessage) + withContext(NonCancellable) { + val response = apiManager.sendMessage(messageId, requestBody, RetrofitTag(username)) + handleSendMessageResponse(messageId, response, savedDraftMessage) + } } catch (exception: IOException) { retryOrFail(ErrorPerformingApiRequest, savedDraftMessage, exception) } catch (exception: Exception) { diff --git a/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsTest.kt b/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsTest.kt index fe51f039d..862e0d1f4 100644 --- a/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsTest.kt +++ b/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsTest.kt @@ -139,8 +139,8 @@ class UploadAttachmentsTest : CoroutinesTest { assertEquals(true, actualIsMessageSending) assertArrayEquals(attachmentIds.toTypedArray(), actualAttachmentIds) assertEquals(NetworkType.CONNECTED, constraints.requiredNetworkType) - assertEquals(BackoffPolicy.EXPONENTIAL, workSpec.backoffPolicy) - assertEquals(20000, workSpec.backoffDelayDuration) + assertEquals(BackoffPolicy.LINEAR, workSpec.backoffPolicy) + assertEquals(10000, workSpec.backoffDelayDuration) verify { workManager.getWorkInfoByIdLiveData(any()) } } } @@ -261,7 +261,7 @@ class UploadAttachmentsTest : CoroutinesTest { coEvery { attachmentsRepository.upload(attachment2, crypto) } answers { AttachmentsRepository.Result.Failure("Failed to upload attachment2") } - every { parameters.runAttemptCount } returns 2 + every { parameters.runAttemptCount } returns 0 val result = uploadAttachments.doWork() @@ -325,7 +325,7 @@ class UploadAttachmentsTest : CoroutinesTest { coEvery { attachmentsRepository.uploadPublicKey(message, crypto) } answers { AttachmentsRepository.Result.Failure("Failed to upload public key") } - every { parameters.runAttemptCount } returns 2 + every { parameters.runAttemptCount } returns 0 val result = uploadAttachments.doWork() diff --git a/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt b/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt index d26d051c9..9d2a545fc 100644 --- a/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt +++ b/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt @@ -180,8 +180,8 @@ class SendMessageWorkerTest : CoroutinesTest { assertEquals(previousSenderAddressId, actualPreviousSenderAddress) assertEquals(securityOptions, actualMessageSecurityOptions?.deserialize(MessageSecurityOptions.serializer())) assertEquals(NetworkType.CONNECTED, constraints.requiredNetworkType) - assertEquals(BackoffPolicy.EXPONENTIAL, workSpec.backoffPolicy) - assertEquals(20000, workSpec.backoffDelayDuration) + assertEquals(BackoffPolicy.LINEAR, workSpec.backoffPolicy) + assertEquals(10000, workSpec.backoffDelayDuration) verify { workManager.getWorkInfoByIdLiveData(any()) } } } @@ -685,7 +685,7 @@ class SendMessageWorkerTest : CoroutinesTest { coEvery { saveDraft(any()) } returns SaveDraftResult.Success(savedDraftMessageId) every { sendPreferencesFactory.fetch(any()) } returns mapOf() every { packageFactory.generatePackages(any(), any(), any(), any()) } throws exception - every { parameters.runAttemptCount } returns 2 + every { parameters.runAttemptCount } returns 1 mockkStatic(Timber::class) val result = worker.doWork() diff --git a/app/src/test/java/ch/protonmail/android/worker/CreateDraftWorkerTest.kt b/app/src/test/java/ch/protonmail/android/worker/CreateDraftWorkerTest.kt index 80775b20f..5989b084e 100644 --- a/app/src/test/java/ch/protonmail/android/worker/CreateDraftWorkerTest.kt +++ b/app/src/test/java/ch/protonmail/android/worker/CreateDraftWorkerTest.kt @@ -174,8 +174,8 @@ class CreateDraftWorkerTest : CoroutinesTest { assertEquals(messageActionType.messageActionTypeValue, actualMessageActionType) assertEquals(previousSenderAddressId, actualPreviousSenderAddress) assertEquals(NetworkType.CONNECTED, constraints.requiredNetworkType) - assertEquals(BackoffPolicy.EXPONENTIAL, workSpec.backoffPolicy) - assertEquals(20000, workSpec.backoffDelayDuration) + assertEquals(BackoffPolicy.LINEAR, workSpec.backoffPolicy) + assertEquals(10000, workSpec.backoffDelayDuration) verify { workManager.getWorkInfoByIdLiveData(any()) } } } @@ -954,7 +954,7 @@ class CreateDraftWorkerTest : CoroutinesTest { every { this@mockk.message.subject } returns "Subject001" } coEvery { apiManager.createDraft(any()) } throws IOException(errorMessage) - every { parameters.runAttemptCount } returns 2 + every { parameters.runAttemptCount } returns 0 val attachment = Attachment("attachment", keyPackets = "OriginalAttachmentPackets", inline = true) val parentMessage = mockk { coEvery { this@mockk.Attachments } returns listOf(attachment) From ce3d5cf46d24af74722b625335d78c7a2d5dd0a8 Mon Sep 17 00:00:00 2001 From: Marino Meneghel Date: Wed, 18 Aug 2021 15:42:34 +0200 Subject: [PATCH 3/4] Updated .gitignore to exclude intellij's deploymentTargetDropDown --- .gitignore | 4 +--- .idea/deploymentTargetDropDown.xml | 17 ----------------- 2 files changed, 1 insertion(+), 20 deletions(-) delete mode 100644 .idea/deploymentTargetDropDown.xml diff --git a/.gitignore b/.gitignore index eae4d3884..a53c79951 100644 --- a/.gitignore +++ b/.gitignore @@ -56,6 +56,7 @@ app/src/beta/google-services.json .idea/**/usage.statistics.xml .idea/**/dictionaries/** .idea/assetWizardSettings.xml +.idea/deploymentTargetDropDown.xml # Do not exclude project Dictionary !.idea/**/dictionaries/project.xml .idea/**/shelf @@ -126,6 +127,3 @@ fabric.properties # Android studio 3.1+ serialized cache file .idea/caches/build_file_checksums.ser -# # # # # # # # # -# IDEA SETTINGS # -# # # # # # # # # diff --git a/.idea/deploymentTargetDropDown.xml b/.idea/deploymentTargetDropDown.xml deleted file mode 100644 index 5c46a1db3..000000000 --- a/.idea/deploymentTargetDropDown.xml +++ /dev/null @@ -1,17 +0,0 @@ - - - - - - - - - - - - - - - - - \ No newline at end of file From 3b3e34ff5ee84bc46483b82246c5a5d7cb634bae Mon Sep 17 00:00:00 2001 From: Marino Meneghel Date: Wed, 18 Aug 2021 18:14:47 +0200 Subject: [PATCH 4/4] Send Message worker finds message by messageId when dbId fails This is to add a layer or resieliency to a potential failure case that would happen if a "replace" operation in Room causes this message's DB ID to change. MAILAND-2293 --- .../android/compose/send/SendMessageWorker.kt | 14 ++++++++---- .../compose/send/SendMessageWorkerTest.kt | 22 +++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt index ea3850b90..eb6779ee6 100644 --- a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt +++ b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt @@ -72,6 +72,7 @@ import kotlinx.coroutines.CancellationException import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.withContext +import me.proton.core.util.kotlin.EMPTY_STRING import me.proton.core.util.kotlin.deserialize import me.proton.core.util.kotlin.serialize import timber.log.Timber @@ -111,8 +112,12 @@ class SendMessageWorker @WorkerInject constructor( override suspend fun doWork(): Result { val messageDatabaseId = getInputMessageDatabaseId() - Timber.i("Send Message Worker executing with messageDatabaseId $messageDatabaseId") + val inputMessageId = getInputMessageId() + Timber.i( + "Send Message Worker executing with messageDatabaseId $messageDatabaseId - messageID $inputMessageId" + ) val message = messageDetailsRepository.findMessageByMessageDbId(messageDatabaseId) + ?: messageDetailsRepository.findMessageById(inputMessageId) if (message == null) { showSendMessageError(NO_SUBJECT) pendingActionsDao.deletePendingSendByDbId(messageDatabaseId) @@ -124,8 +129,7 @@ class SendMessageWorker @WorkerInject constructor( val previousSenderAddressId = requireNotNull(getInputPreviousSenderAddressId()) val username = requireNotNull(getInputCurrentUsername()) - val result = saveDraft(message, previousSenderAddressId) - return when (result) { + return when (val result = saveDraft(message, previousSenderAddressId)) { is SaveDraftResult.Success -> { val messageId = result.draftId Timber.i("Send Message Worker saved draft successfully for messageId $messageId") @@ -327,6 +331,9 @@ class SendMessageWorker @WorkerInject constructor( private fun getInputParentId() = inputData.getString(KEY_INPUT_SEND_MESSAGE_MSG_PARENT_ID) + private fun getInputMessageId() = + inputData.getString(KEY_INPUT_SEND_MESSAGE_MESSAGE_ID) ?: EMPTY_STRING + private fun getInputMessageDatabaseId() = inputData.getLong(KEY_INPUT_SEND_MESSAGE_MSG_DB_ID, INPUT_MESSAGE_DB_ID_NOT_FOUND) @@ -374,5 +381,4 @@ class SendMessageWorker @WorkerInject constructor( return workManager.getWorkInfoByIdLiveData(sendMessageRequest.id).asFlow() } } - } diff --git a/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt b/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt index 9d2a545fc..a62b181ff 100644 --- a/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt +++ b/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt @@ -220,12 +220,33 @@ class SendMessageWorkerTest : CoroutinesTest { coVerify { saveDraft(expectedParameters) } } + @Test + fun workerTriesFindingTheMessageByMessageIdWhenMessageIsNotFoundByDatabaseId() = runBlockingTest { + val messageDbId = 23712L + val messageId = "8322224-1341" + val message = Message(messageId = messageId) + val createdDraftId = "createdDraftId" + givenFullValidInput(messageDbId, messageId) + coEvery { messageDetailsRepository.findMessageByMessageDbId(messageDbId) } returns null + coEvery { messageDetailsRepository.findMessageById(messageId) } returns message + coEvery { messageDetailsRepository.findMessageById(createdDraftId) } returns null + coEvery { saveDraft.invoke(any()) } returns SaveDraftResult.Success(createdDraftId) + + worker.doWork() + + verify { userNotifier wasNot Called } + val paramsSlot = slot() + coVerify { saveDraft.invoke(capture(paramsSlot)) } + assertEquals(message, paramsSlot.captured.message) + } + @Test fun workerNotifiesUserAndFailsWhenMessageIsNotFoundInTheDatabase() = runBlockingTest { val messageDbId = 2373L val messageId = "8322223" givenFullValidInput(messageDbId, messageId) coEvery { messageDetailsRepository.findMessageByMessageDbId(messageDbId) } returns null + coEvery { messageDetailsRepository.findMessageById(messageId) } returns null every { context.getString(R.string.message_drafted) } returns "error message 9214" val result = worker.doWork() @@ -1027,3 +1048,4 @@ class SendMessageWorkerTest : CoroutinesTest { } } } +