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 timeout to web push workers and stop retries when getting 4xx #9434

Merged
merged 1 commit into from Dec 17, 2018

Conversation

Projects
None yet
2 participants
@Gargron
Copy link
Member

Gargron commented Dec 5, 2018

No description provided.

@Gargron Gargron added the performance label Dec 5, 2018

@Gargron Gargron force-pushed the fix-web-push-retry branch from 59f3062 to 4232587 Dec 5, 2018

@ThibG

This comment has been minimized.

Copy link
Collaborator

ThibG commented Dec 5, 2018

1-3s is a bit on the low end, no? Why not something closer to the timeouts we use for basically everything else?

@Gargron

This comment has been minimized.

Copy link
Member Author

Gargron commented Dec 5, 2018

Because webpush endpoints are either Mozilla or Google or Apple, or an app developer, i.e. I can expect it to not be under heavy load or to be able to handle big loads. 1 second for a request is really long! It's also not that critical.

@ThibG

This comment has been minimized.

Copy link
Collaborator

ThibG commented Dec 5, 2018

Hm, the instance issuing the notifications may itself have connectivity issues/be under load.

Are those tasks really causing delays? If so, those changes are probably stopping legitimate notifications from being delivered, right?

Do no retry web push workers if the server returns a 4xx response
Add timeout of 10s to web push requests

@Gargron Gargron force-pushed the fix-web-push-retry branch from edb9edd to 25d8b9c Dec 14, 2018

@Gargron Gargron merged commit 628da11 into master Dec 17, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details

@Gargron Gargron deleted the fix-web-push-retry branch Dec 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment