Add lambda in executeRequest params to define a custom retry policy #4950
Conversation
Florian14
force-pushed
the
feature/fre/request_with_custom_retry_policy
branch
from
a9361cc
to
7c63264
Compare
Matrix SDKIntegration Tests Results:
|
| currentDelay = currentDelay.times(2L).coerceAtMost(maxDelayBeforeRetry) | ||
| // Try again (loop) |
There was a problem hiding this comment.
Maybe restore this comment, it is still valid.
| } else if (canRetry && currentRetryCount < maxRetriesCount && exception.shouldBeRetried()) { | ||
| delay(currentDelay) | ||
| if (canRetryOnFailure(exception) && currentRetryCount < maxRetriesCount) { | ||
| delay(exception.getRetryDelay(currentDelay)) |
There was a problem hiding this comment.
This is not strictly equivalent with the previous version, when we had a 429, the default delay was 1s. But I think it is still fine.
There was a problem hiding this comment.
Yes indeed, if we want to keep previous behaviour for 429, we should also return a fixed delay in the lambda (or null to use the default delay behaviour which is multiplied by 2 after each failure).
With my current implementation, receiving 4 successive 429 errors will lead to a total delay of 15 seconds (1 + 2 + 4 +8) instead of 4 seconds with the fixed delay. We should probably have to think about it. Was the previous behaviour originally expected for this special case ?
Note that the delay is passed as a default value in this special case, the delay passed in the 429 error body should be used, if any (it is not a mandatory field).
WDYT @bmarty?
There was a problem hiding this comment.
I see, the point is that if you want to have an automatic retry in case of 429, except if the delay is too long (which could be configurable by the app), it will be more difficult with your change.
So maybe keep the two cases (429 and canRetry) clearly separated.
There was a problem hiding this comment.
Ack. I close this PR and will create a new one.
| @@ -15,22 +15,24 @@ | |||
| */ | |||
|
|
|||
| package org.matrix.android.sdk.internal.network | |||
|
|
|||
| import org.matrix.android.sdk.internal.network.defaultRequestRetryPolicy as internalDefaultRequestRetryPolicy | |||
There was a problem hiding this comment.
I do not think it is necessary to create an alias in this case.
Not sure, this is the case of Tchap fork, but should we apply it for all login flows? I haven't tried to trigger the error using matrix.org yet. |
Previously, failed requests were retried according to a
canRetryboolean passed inexecuteRequestmethod params. There is an exception for the 429 error with theM_LIMIT_EXCEEDEDerror code which were automatically retried without taking account of thecanRetryflag.The goal of this PR is to have more flexibility on the retry policy to be able to have a custom policy for a specific request. For example, a 429 error with a very long retry delay should no be retried but notify the user about the delay.
To keep the previous behaviour, the current retry policy has been kept and passed as the default value of the new
canRetryOnFailuremethod parameter.