Skip to content
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.

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

[Login] Handle correctly M_LIMIT_EXCEEDED error code #334

Closed
wants to merge 3 commits into from

Conversation

Claire1817
Copy link
Contributor

@github-actions
Copy link

github-actions bot commented Dec 28, 2021

Unit Test Results

11 files  ±0  11 suites  ±0   7s ⏱️ -2s
83 tests ±0  83 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit c10a379. ± Comparison against base commit 0b5fb6f.

♻️ This comment has been updated with latest results.

// // 429, we can retry
// delay(exception.getRetryDelay(1_000))
// } else
if (canRetry && currentRetryCount < maxRetriesCount && exception.shouldBeRetried()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree about this change.
If I understood correctly, you commented this block about exception.error.code == MatrixError.M_LIMIT_EXCEEDED, in order to prevent the application to be blocked until the end of the delay provided by the exception (which is 30 mins in our case). If this is true, this means canRetry and/or exception.shouldBeRetried() was false, which let you trigger the exception.

I think there is a bug in Element-android sdk here, we should check exception.shouldBeRetried() and canRetry booleans even when exception.error.code == MatrixError.M_LIMIT_EXCEEDED

About my suggestion:

  • we have to check whether the login request is triggered by disabling the retry option
  • we have to test the other cases where M_LIMIT_EXCEEDED error may be received to be sure we don't disturb them. For example when you send several messages in a room in a very short delay, you will trigger this error. I think this request (send message) enables the retry

Copy link
Contributor

Choose a reason for hiding this comment

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

For more context the new retry policy regarding 429 has been added element-hq/element-android#3142.

Copy link
Contributor

Choose a reason for hiding this comment

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

And in case of 429, the SDK automatically retry, whatever the value of canRetry

@giomfo
Copy link
Contributor

giomfo commented Dec 29, 2021

@ClaireGizard After reviewing your PR, I don't think we are going in the right direction to fix this issue.
I assigned @Florian14 to consider again this issue with my point of view. I let you sync with him before considering my comment

@yostyle yostyle requested review from Florian14 and removed request for yostyle January 4, 2022 09:55
@Florian14 Florian14 requested review from bmarty and removed request for Florian14 January 10, 2022 13:48
@Florian14 Florian14 marked this pull request as draft January 14, 2022 15:31
@giomfo giomfo closed this Jan 24, 2022
@Claire1817 Claire1817 deleted the cgizard/ISSUE-322 branch April 8, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants