Skip to content

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

Merged
merged 25 commits into from
Feb 19, 2025

Conversation

simone-sanfratello
Copy link
Contributor

@simone-sanfratello simone-sanfratello commented Feb 11, 2025

Checklist


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

  • update deps
  • refactor websocket code
  • introduce connection on timeout on first connection, with the same reconnection logic
  • add more test on websocket cases and events

Copy link
Member

@climba03003 climba03003 left a 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?

@mcollina
Copy link
Member

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.

@simone-sanfratello simone-sanfratello marked this pull request as ready for review February 13, 2025 10:52
'use strict'

const DEFAULT_PING_INTERVAL = 30_000
const DEFAULT_MAX_RECONNECTION_RETRIES = Infinity
Copy link
Member

@climba03003 climba03003 Feb 14, 2025

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.

Copy link
Contributor Author

@simone-sanfratello simone-sanfratello Feb 14, 2025

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

Copy link
Member

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.

Copy link
Member

@climba03003 climba03003 left a 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.

@simone-sanfratello
Copy link
Contributor Author

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.
I've just introduced onTargetRequest and onTargetResponse specifically to handle data that are lost due to disconnection.

Copy link
Member

@mcollina mcollina left a 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.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@simone-sanfratello
Copy link
Contributor Author

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'd do that using hooks, I'm adding an example

@simone-sanfratello
Copy link
Contributor Author

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 6c115ff into fastify:main Feb 19, 2025
11 checks passed
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.

3 participants