web-push icon indicating copy to clipboard operation
web-push copied to clipboard

add new EdgeThrottled exception

Open mohamedhafez opened this issue 11 months ago • 4 comments

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

mohamedhafez avatar Dec 10 '24 14:12 mohamedhafez

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.

collimarco avatar Dec 10 '24 15:12 collimarco

This issue has been going on for a very long time...

See all these related issues:

  • https://github.com/zaru/webpush/issues/109
  • https://github.com/zaru/webpush/pull/103
  • https://github.com/MicrosoftDocs/windows-dev-docs/issues/4166
  • https://github.com/microsoft/WindowsAppSDK/issues/3501

collimarco avatar Dec 10 '24 15:12 collimarco

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 avatar Dec 10 '24 15:12 mohamedhafez

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.

mohamedhafez avatar Dec 10 '24 15:12 mohamedhafez