-
-
Notifications
You must be signed in to change notification settings - Fork 98
feat: websocket reconnect #405
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
feat: websocket reconnect #405
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused, why reconnection should happen in proxy but not the client?
The goal of this feature is to allow for the proxy to reconnect the socket in case the origin process has to (gracefully or ungracefully) shut down. |
ac1a210
to
ae2a31b
Compare
9be3c1a
to
ac1225a
Compare
31dfd1d
to
bfaf30a
Compare
'use strict' | ||
|
||
const DEFAULT_PING_INTERVAL = 30_000 | ||
const DEFAULT_MAX_RECONNECTION_RETRIES = Infinity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Max retries should be limited by default.
There is no reason to reconnected unlimited since in reconnecting period the client may continue sending data. The data will buffered in stream unlimited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is to keep the connection alive, assuming the error messages are being monitored, that's why I set Infinity
We can remove the default value as Infinity, but in that case we can't set an arbitrary default value, it really depends by the context, ping interval, reconnection delay and so on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Those should be limited, possibly to 30 seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to add a test for buffering data when reconnecting.
If we lost data in that period of time, this feature may not be very useful.
I think buffering messages is out of scope, in that case we need a storage, and the logic for retrieving is not responsibility of the proxy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would need a way to buffer incoming message or at least have some sort of callback to deal with them if one so desires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I'd do that using hooks, I'm adding an example |
You can see the example. In that case, using a defensive strategy, messages are sent more than once, but using hooks we can custom logic on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Checklist
npm run test
andnpm run benchmark
and the Code of conduct
The goal of this PR is to add reconnection capability on the websocket proxy.
This would be the first of a series of PR, I'm going to