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

Retry all GCM 5xx errors #456

Merged
merged 3 commits into from Oct 20, 2018
Merged

Retry all GCM 5xx errors #456

merged 3 commits into from Oct 20, 2018

Conversation

rofreg
Copy link
Contributor

@rofreg rofreg commented Oct 12, 2018

According to the GCM docs, for errors in the 500-599 range, the sender "must retry later". Currently, rpush only retries these notifications if the error is specifically 500 or 503.

In production, we have seen this somewhat commonly with 502 Bad Gateway errors. This PR adds specific retry handling for 502 errors, as well as general purpose retry handling for other 5xx GCM errors.

@aried3r
Copy link
Member

aried3r commented Oct 15, 2018

Hey @rofreg, thanks, that looks good!

I'll try to have a look soon. But I'm wondering why Travis didn't pick up this PR 🤔

@aried3r
Copy link
Member

aried3r commented Oct 15, 2018

Ok, seems Travis changed their settings at some point. I activated building and testing PRs again, hopefully closing and opening will trigger a build.

@aried3r aried3r closed this Oct 15, 2018
@aried3r aried3r reopened this Oct 15, 2018
@rofreg
Copy link
Contributor Author

rofreg commented Oct 17, 2018

Updated! I believe that the one remaining test failure is a failure on master branch, as the same test appears to be failing for all other PRs too (e.g. #455). I might try to look into that later if time permits – if so, I'll open a separate PR for it.

@aried3r
Copy link
Member

aried3r commented Oct 20, 2018

Thanks @rofreg!

I might try to look into that later if time permits – if so, I'll open a separate PR for it.

That would be really awesome! It's curious, since it used to work and we use it in production at work with the latest Rails 5.x version 🤔

@aried3r aried3r merged commit fa9bbf6 into rpush:master Oct 20, 2018
@aried3r
Copy link
Member

aried3r commented Oct 20, 2018

Thanks for digging into this and the PR! Merging, since the errors on CI aren't caused by this PR.

@aried3r
Copy link
Member

aried3r commented Oct 25, 2018

Followup, might be rails/rails#33651, it's just strange that I can't reproduce it locally.

Adrian1707 pushed a commit to Adrian1707/rpush that referenced this pull request Apr 24, 2024
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.

None yet

2 participants