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

add new EdgeThrottled exception #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mohamedhafez
Copy link
Contributor

@mohamedhafez mohamedhafez commented Dec 10, 2024

Edge throttles much more aggressively than other browsers, so I added a new ResponseError subclass called EdgeThrottled and added code to verify_response to distinguish this case from a generic ResponseError. I'd still like to know if I ever get throttled by Chrome, Firefox, or Safari since it's never happened before, but it happens so much with Edge that I've just given up and ignore these exceptions personally. Others may want to handle this specific case differently as well.

If this PR would be accepted, let me know and I'll write some tests for it

Edge throttles *much* more aggressively than other browsers, added
a new ResponseError subclass called EdgeThrottled and added code
to verify_response to distinguish this case from a generic
ResponseError. I'd still like to know if I ever get throttled by
Chrome, Firefox, or Safari since it's never happened before, but
it happens so much with Edge that I've just given up and ignore
these exceptions personally. Others may want to handle this
specific case differently as well.
@collimarco
Copy link
Contributor

In general I would prefer to avoid vendor-specific exceptions... We already have a TooManyRequests error for the "standard" 429 status code.

The 406 status code returned by Microsoft is plain wrong - see the definition here:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/406

I already reported this issue years ago in their bug trackers.

In any case I recommend to retry all unknown ResponseErrors (we do that), so defining a new type of error should be unnecessary.

@mohamedhafez
Copy link
Contributor Author

Personally I'd argue that if Edge has been doing this incorrectly for a very long time, they are unlikely to fix it any time soon, and we'd be better off dealing with the facts on the ground as opposed to what the spec says it should be...

@mohamedhafez
Copy link
Contributor Author

mohamedhafez commented Dec 10, 2024

I do agree with your statment here though:

This proposal is semantically wrong: you cannot catch a 406, which has a very specific meaning, and raise TooManyRequests . It may produce exceptions that are extremely confusing and hard to debug in the future, with other push services, or with future changes.

That's why i think it should be dealt with as a special case, specifically tailored to the one browser that's doing it wrong.

I personally don't wish to retry all unhandled ResponseErrors: I'd rather just be notified of them, decide what it is that we've been missing handling, and add the code to handle it correctly. Some errors it isn't appropriate to retry, like things that are our fault like PayloadTooLarge for example, and who knows if a new, unhandled exception is one of those. In this case specifically, you'd need special exponential backoff retrying for throttling errors or it would just make the problem worse.

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.

2 participants