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 maximum retries to polling after network error. #472

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

Conversation

voliva
Copy link

@voliva voliva commented Feb 13, 2019

A possible solution to #440

I need that, as that infinite loop is giving a hard time to the load balancer when we have a couple of users having this issue.

What it does is that for every connection request, it stops retrying after 3 unsuccessful requests. It's the consumer's responsibility then to decide what to do (e.g. retry again?)

I'm not fully aware of the internals of this library, so I'm not too sure if this is fully safe to merge in, but it seems to cover the basic use case.

@brycekahle
Copy link
Contributor

This be would be a major breaking change for most users. Do you know what network conditions are causing a couple users to continuously get a network error?

@voliva
Copy link
Author

voliva commented Mar 25, 2019

I created a gist here, trying to reproduce the issue: https://gist.github.com/voliva/21191f5d1cb1f712a259f81e1b6bfce5

This code may look very fictional though - I just tried to show an oversimplified case. In our solution, the SockJS server is written in Java Spring, and we have a check in a HandshakeInterceptor that may drop the requests if they don't pass a validation case.

Currently, it does abort the request after a while (based on connection RTO timeout), but until it doesn't time out it's constantly creating new requests, which add up to thousands. This PR tries to solve this by also aborting the request after Polling.NETWORK_RETRIES consecutive unsuccessful retries.

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