Skip to content

Do not auto-retry on 429 in case of too long retry delay #4995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 20, 2022
+50 −38
Merged

Do not auto-retry on 429 in case of too long retry delay #4995

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd61f07
Do not automatically retry 429 with a too long delay
Jan 19, 2022
83c961e
Use Throwable.isLimitExceededError extension
Jan 19, 2022
9ebcb3e
Add changelog
Jan 19, 2022
879d5eb
Update kdoc
Jan 20, 2022
b8fa6f9
Add missing import in kdoc
Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/4995.removal
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
429 are not automatically retried anymore in case of too long retry delay
11 changes: 8 additions & 3 deletions matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt
View file
Original file line number Diff line number Diff line change
@@ -32,13 +32,18 @@ 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 &&
httpCode == 429 &&
error.code == MatrixError.M_LIMIT_EXCEEDED

fun Throwable.shouldBeRetried(): Boolean {
return this is Failure.NetworkConnection ||
this is IOException ||
(this is Failure.ServerError && error.code == MatrixError.M_LIMIT_EXCEEDED)
this.isLimitExceededError()
}

/**
21 changes: 12 additions & 9 deletions matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/Request.kt
View file
Original file line number Diff line number Diff line change
@@ -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 <DATA> 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe update the documentation of maxDelayBeforeRetry to inform that it also has an effect on 429 management.

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)
}
}
}
10 changes: 5 additions & 5 deletions .../org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorCoroutine.kt
View file
Original file line number Diff line number Diff line change
@@ -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()
12 changes: 6 additions & 6 deletions ...ava/org/matrix/android/sdk/internal/session/room/send/queue/EventSenderProcessorThread.kt
View file
Original file line number Diff line number Diff line change
@@ -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,25 +180,25 @@ 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
// Todo if its a quota exception can we get timout?
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()
29 changes: 15 additions & 14 deletions vector/src/main/java/im/vector/app/core/error/ErrorFormatter.kt
View file
Original file line number Diff line number Diff line change
@@ -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() }
}