From fd61f073738318790783f076642f288ee571e287 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Wed, 19 Jan 2022 16:48:20 +0100 Subject: [PATCH 1/5] Do not automatically retry 429 with a too long delay --- .../android/sdk/api/failure/Extensions.kt | 5 +++++ .../android/sdk/internal/network/Request.kt | 21 +++++++++++-------- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt index 13a26c89c1..8e7a0a80eb 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt @@ -35,6 +35,11 @@ fun Throwable.isTokenError() = error.code == MatrixError.M_MISSING_TOKEN || error.code == MatrixError.ORG_MATRIX_EXPIRED_ACCOUNT) +fun Throwable.isLimitExceededError() = + this is Failure.ServerError && + httpCode == 429 && + error.code == MatrixError.M_LIMIT_EXCEEDED + fun Throwable.shouldBeRetried(): Boolean { return this is Failure.NetworkConnection || this is IOException || diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt index 927d9f7dd2..8cd608a945 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt @@ -19,8 +19,8 @@ package org.matrix.android.sdk.internal.network import kotlinx.coroutines.CancellationException import kotlinx.coroutines.delay import org.matrix.android.sdk.api.failure.Failure -import org.matrix.android.sdk.api.failure.MatrixError import org.matrix.android.sdk.api.failure.getRetryDelay +import org.matrix.android.sdk.api.failure.isLimitExceededError import org.matrix.android.sdk.api.failure.shouldBeRetried import org.matrix.android.sdk.internal.network.ssl.CertUtil import retrofit2.HttpException @@ -74,23 +74,26 @@ internal suspend inline fun executeRequest(globalErrorReceiver: GlobalErr currentRetryCount++ - if (exception is Failure.ServerError && - exception.httpCode == 429 && - exception.error.code == MatrixError.M_LIMIT_EXCEEDED && - currentRetryCount < maxRetriesCount) { + if (exception.isLimitExceededError() && currentRetryCount < maxRetriesCount) { // 429, we can retry - delay(exception.getRetryDelay(1_000)) + val retryDelay = exception.getRetryDelay(1_000) + if (retryDelay <= maxDelayBeforeRetry) { + delay(retryDelay) + } else { + // delay is too high to be retried, propagate the exception + throw exception + } } else if (canRetry && currentRetryCount < maxRetriesCount && exception.shouldBeRetried()) { delay(currentDelay) currentDelay = currentDelay.times(2L).coerceAtMost(maxDelayBeforeRetry) // Try again (loop) } else { throw when (exception) { - is IOException -> Failure.NetworkConnection(exception) + is IOException -> Failure.NetworkConnection(exception) is Failure.ServerError, is Failure.OtherServerError, - is CancellationException -> exception - else -> Failure.Unknown(exception) + is CancellationException -> exception + else -> Failure.Unknown(exception) } } } From 83c961e2552b8d7ee5af5ae48fe261c0a5496e81 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Wed, 19 Jan 2022 16:53:53 +0100 Subject: [PATCH 2/5] Use Throwable.isLimitExceededError extension --- .../android/sdk/api/failure/Extensions.kt | 6 ++-- .../queue/EventSenderProcessorCoroutine.kt | 10 +++---- .../send/queue/EventSenderProcessorThread.kt | 12 ++++---- .../vector/app/core/error/ErrorFormatter.kt | 29 ++++++++++--------- 4 files changed, 29 insertions(+), 28 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt index 8e7a0a80eb..aabe6e0d06 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt @@ -32,8 +32,8 @@ fun Throwable.is401() = fun Throwable.isTokenError() = this is Failure.ServerError && (error.code == MatrixError.M_UNKNOWN_TOKEN || - error.code == MatrixError.M_MISSING_TOKEN || - error.code == MatrixError.ORG_MATRIX_EXPIRED_ACCOUNT) + error.code == MatrixError.M_MISSING_TOKEN || + error.code == MatrixError.ORG_MATRIX_EXPIRED_ACCOUNT) fun Throwable.isLimitExceededError() = this is Failure.ServerError && @@ -43,7 +43,7 @@ fun Throwable.isLimitExceededError() = fun Throwable.shouldBeRetried(): Boolean { return this is Failure.NetworkConnection || this is IOException || - (this is Failure.ServerError && error.code == MatrixError.M_LIMIT_EXCEEDED) + this.isLimitExceededError() } /** diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorCoroutine.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorCoroutine.kt index 3be01762e7..eb69161614 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorCoroutine.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorCoroutine.kt @@ -23,8 +23,8 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.matrix.android.sdk.api.auth.data.SessionParams import org.matrix.android.sdk.api.failure.Failure -import org.matrix.android.sdk.api.failure.MatrixError import org.matrix.android.sdk.api.failure.getRetryDelay +import org.matrix.android.sdk.api.failure.isLimitExceededError import org.matrix.android.sdk.api.session.Session import org.matrix.android.sdk.api.session.crypto.CryptoService import org.matrix.android.sdk.api.session.events.model.Event @@ -145,17 +145,17 @@ internal class EventSenderProcessorCoroutine @Inject constructor( task.execute() } catch (exception: Throwable) { when { - exception is IOException || exception is Failure.NetworkConnection -> { + exception is IOException || exception is Failure.NetworkConnection -> { canReachServer.set(false) task.markAsFailedOrRetry(exception, 0) } - (exception is Failure.ServerError && exception.error.code == MatrixError.M_LIMIT_EXCEEDED) -> { + (exception.isLimitExceededError()) -> { task.markAsFailedOrRetry(exception, exception.getRetryDelay(3_000)) } - exception is CancellationException -> { + exception is CancellationException -> { Timber.v("## $task has been cancelled, try next task") } - else -> { + else -> { Timber.v("## un-retryable error for $task, try next task") // this task is in error, check next one? task.onTaskFailed() diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorThread.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorThread.kt index f32890f3fb..1ee3139194 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorThread.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorThread.kt @@ -23,7 +23,7 @@ import org.matrix.android.sdk.api.auth.data.SessionParams import org.matrix.android.sdk.api.auth.data.sessionId import org.matrix.android.sdk.api.extensions.tryOrNull import org.matrix.android.sdk.api.failure.Failure -import org.matrix.android.sdk.api.failure.MatrixError +import org.matrix.android.sdk.api.failure.isLimitExceededError import org.matrix.android.sdk.api.failure.isTokenError import org.matrix.android.sdk.api.session.Session import org.matrix.android.sdk.api.session.crypto.CryptoService @@ -171,7 +171,7 @@ internal class EventSenderProcessorThread @Inject constructor( break@retryLoop } catch (exception: Throwable) { when { - exception is IOException || exception is Failure.NetworkConnection -> { + exception is IOException || exception is Failure.NetworkConnection -> { canReachServer = false if (task.retryCount.getAndIncrement() >= 3) task.onTaskFailed() while (!canReachServer) { @@ -180,7 +180,7 @@ internal class EventSenderProcessorThread @Inject constructor( waitForNetwork() } } - (exception is Failure.ServerError && exception.error.code == MatrixError.M_LIMIT_EXCEEDED) -> { + (exception.isLimitExceededError()) -> { if (task.retryCount.getAndIncrement() >= 3) task.onTaskFailed() Timber.v("## SendThread retryLoop retryable error for $task reason: ${exception.localizedMessage}") // wait a bit @@ -188,17 +188,17 @@ internal class EventSenderProcessorThread @Inject constructor( sleep(3_000) continue@retryLoop } - exception.isTokenError() -> { + exception.isTokenError() -> { Timber.v("## SendThread retryLoop retryable TOKEN error, interrupt") // we can exit the loop task.onTaskFailed() throw InterruptedException() } - exception is CancellationException -> { + exception is CancellationException -> { Timber.v("## SendThread task has been cancelled") break@retryLoop } - else -> { + else -> { Timber.v("## SendThread retryLoop Un-Retryable error, try next task") // this task is in error, check next one? task.onTaskFailed() diff --git a/vector/src/main/java/im/vector/app/core/error/ErrorFormatter.kt b/vector/src/main/java/im/vector/app/core/error/ErrorFormatter.kt index 8640fa6f05..2eb36d758e 100644 --- a/vector/src/main/java/im/vector/app/core/error/ErrorFormatter.kt +++ b/vector/src/main/java/im/vector/app/core/error/ErrorFormatter.kt @@ -24,6 +24,7 @@ import org.matrix.android.sdk.api.failure.Failure import org.matrix.android.sdk.api.failure.MatrixError import org.matrix.android.sdk.api.failure.MatrixIdFailure import org.matrix.android.sdk.api.failure.isInvalidPassword +import org.matrix.android.sdk.api.failure.isLimitExceededError import org.matrix.android.sdk.api.session.identity.IdentityServiceError import java.net.HttpURLConnection import java.net.SocketTimeoutException @@ -58,53 +59,53 @@ class DefaultErrorFormatter @Inject constructor( } is Failure.ServerError -> { when { - throwable.error.code == MatrixError.M_CONSENT_NOT_GIVEN -> { + throwable.error.code == MatrixError.M_CONSENT_NOT_GIVEN -> { // Special case for terms and conditions stringProvider.getString(R.string.error_terms_not_accepted) } - throwable.isInvalidPassword() -> { + throwable.isInvalidPassword() -> { stringProvider.getString(R.string.auth_invalid_login_param) } - throwable.error.code == MatrixError.M_USER_IN_USE -> { + throwable.error.code == MatrixError.M_USER_IN_USE -> { stringProvider.getString(R.string.login_signup_error_user_in_use) } - throwable.error.code == MatrixError.M_BAD_JSON -> { + throwable.error.code == MatrixError.M_BAD_JSON -> { stringProvider.getString(R.string.login_error_bad_json) } - throwable.error.code == MatrixError.M_NOT_JSON -> { + throwable.error.code == MatrixError.M_NOT_JSON -> { stringProvider.getString(R.string.login_error_not_json) } - throwable.error.code == MatrixError.M_THREEPID_DENIED -> { + throwable.error.code == MatrixError.M_THREEPID_DENIED -> { stringProvider.getString(R.string.login_error_threepid_denied) } - throwable.error.code == MatrixError.M_LIMIT_EXCEEDED -> { + throwable.isLimitExceededError() -> { limitExceededError(throwable.error) } - throwable.error.code == MatrixError.M_TOO_LARGE -> { + throwable.error.code == MatrixError.M_TOO_LARGE -> { stringProvider.getString(R.string.error_file_too_big_simple) } - throwable.error.code == MatrixError.M_THREEPID_NOT_FOUND -> { + throwable.error.code == MatrixError.M_THREEPID_NOT_FOUND -> { stringProvider.getString(R.string.login_reset_password_error_not_found) } - throwable.error.code == MatrixError.M_USER_DEACTIVATED -> { + throwable.error.code == MatrixError.M_USER_DEACTIVATED -> { stringProvider.getString(R.string.auth_invalid_login_deactivated_account) } throwable.error.code == MatrixError.M_THREEPID_IN_USE && - throwable.error.message == "Email is already in use" -> { + throwable.error.message == "Email is already in use" -> { stringProvider.getString(R.string.account_email_already_used_error) } throwable.error.code == MatrixError.M_THREEPID_IN_USE && - throwable.error.message == "MSISDN is already in use" -> { + throwable.error.message == "MSISDN is already in use" -> { stringProvider.getString(R.string.account_phone_number_already_used_error) } - throwable.error.code == MatrixError.M_THREEPID_AUTH_FAILED -> { + throwable.error.code == MatrixError.M_THREEPID_AUTH_FAILED -> { stringProvider.getString(R.string.error_threepid_auth_failed) } throwable.error.code == MatrixError.M_UNKNOWN && throwable.error.message == "Not allowed to join this room" -> { stringProvider.getString(R.string.room_error_access_unauthorized) } - else -> { + else -> { throwable.error.message.takeIf { it.isNotEmpty() } ?: throwable.error.code.takeIf { it.isNotEmpty() } } From 9ebcb3ed5250ec1044e021e007a4689b66d77a83 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Wed, 19 Jan 2022 17:04:00 +0100 Subject: [PATCH 3/5] Add changelog --- changelog.d/4995.removal | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4995.removal diff --git a/changelog.d/4995.removal b/changelog.d/4995.removal new file mode 100644 index 0000000000..9eacff87cd --- /dev/null +++ b/changelog.d/4995.removal @@ -0,0 +1 @@ +429 are not automatically retried anymore in case of too long retry delay \ No newline at end of file From 879d5eb5f6199a678faf11489310a4aa4dd73e77 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Thu, 20 Jan 2022 10:49:02 +0100 Subject: [PATCH 4/5] Update kdoc --- .../java/org/matrix/android/sdk/internal/network/Request.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt index 8cd608a945..485816aba4 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt @@ -33,7 +33,8 @@ import java.io.IOException * * @param globalErrorReceiver will be use to notify error such as invalid token error. See [GlobalError] * @param canRetry if set to true, the request will be executed again in case of error, after a delay - * @param maxDelayBeforeRetry the max delay to wait before a retry + * @param maxDelayBeforeRetry the max delay to wait before a retry. Note that in the case of a 429, if the provided delay exceeds this value, the error will + * be propagated as it does not make sense to retry it with a shorter delay. * @param maxRetriesCount the max number of retries * @param requestBlock a suspend lambda to perform the network request */ From b8fa6f9ec82643f56890cc35ddb2bd2b005d5ec1 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Thu, 20 Jan 2022 10:50:07 +0100 Subject: [PATCH 5/5] Add missing import in kdoc --- .../main/java/org/matrix/android/sdk/internal/network/Request.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt index 485816aba4..695e7525af 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt @@ -19,6 +19,7 @@ package org.matrix.android.sdk.internal.network import kotlinx.coroutines.CancellationException import kotlinx.coroutines.delay import org.matrix.android.sdk.api.failure.Failure +import org.matrix.android.sdk.api.failure.GlobalError import org.matrix.android.sdk.api.failure.getRetryDelay import org.matrix.android.sdk.api.failure.isLimitExceededError import org.matrix.android.sdk.api.failure.shouldBeRetried