Add http 429 error handling#189
Merged
non-Jedi merged 5 commits intomatrix-org:masterfrom Apr 18, 2018
eternal-flame-AD:add_http_429_error_handling
Merged
Add http 429 error handling#189non-Jedi merged 5 commits intomatrix-org:masterfrom eternal-flame-AD:add_http_429_error_handling
non-Jedi merged 5 commits intomatrix-org:masterfrom
eternal-flame-AD:add_http_429_error_handling
Conversation
added 3 commits
April 3, 2018 17:28
The matrix.org server returns HTTP 429 TOO MANY REQUESTS error in this format:
{'errcode': 'M_UNKNOWN', 'error': '{"errcode":"M_LIMIT_EXCEEDED","error":"Too Many Requests","retry_after_ms":3438}'}
Which would cause an KeyError
So I made some change to fix the error and added error handling to prevent an uncaught error caused by a non-standard server response.
Collaborator
|
I believe this is addressing same bug as #193. @eternal-flame-AD are you seeing this behavior on all 429 responses or just a subset of them? |
non-Jedi
requested changes
Apr 17, 2018
Collaborator
non-Jedi
left a comment
non-Jedi
left a comment
There was a problem hiding this comment.
Thanks for taking the time to fix this. Really appreciate it.
| """ | ||
|
|
||
| def __init__(self, base_url, token=None, identity=None): | ||
| def __init__(self, base_url, token=None, identity=None, default_429_wait_ms=5000): |
Collaborator
There was a problem hiding this comment.
Please add documentation on this kwarg to the docstring. After that, I can
certainly merge this; hopefully the weird behavior from Synapse will be fixed
upstream soon, and then we can remove at least a bit of this logic.
Collaborator
|
(We'll keep #193 open even after merging this to track the upstream issue with Synapse.) |
Contributor
Author
|
I saw this behaviour every time I triggered a 429 response at matrix.org home server. |
added 2 commits
April 18, 2018 10:41
Update the documentation for default_429_wait_ms kwarg
Collaborator
|
LGTM. Thanks! |
non-Jedi
approved these changes
Apr 18, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request included an error handling process while handling HTTP 429 TOO MANY REQUESTS error.
Here's a sample of matrix.org's HTTP 429 reply:
Which would cause an uncaught KeyError in the original implementation.
Also, in order to deal with other non-standard 429 replies, I added a default_429_wait_ms=5000 attribute to be used as a default waiting time if the server didn't provide one.
Signed-off-by: Andy Fu webmaster@andycloud.dynu.net