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

feat(general): add heartbeat to websocket control #460

Merged
merged 6 commits into from
Dec 30, 2024

Conversation

Mmx233
Copy link
Contributor

@Mmx233 Mmx233 commented Dec 27, 2024

When neko runs behind an L3 or L7 load balancer, it may encounter TCP connection timeout issues. Sometimes the maximum timeout cannot be set to a very high value, causing neko to frequently reconnect. Moreover, I believe that setting too large a timeout value might result in the server retaining too many unused connections.

This PR adds the event client/heartbeat and ignores this event in the backend.

The heartbeat is configured through the environment variable NEKO_HEARTBEAT_INTERVAL or the configuration setting heartbeat_interval, with a default value of 120 seconds. When the value is less than or equal to 0, the heartbeat mechanism is disabled.

After heartbeat_interval is passed to the frontend via the system/init event, the frontend starts sending heartbeat events at regular intervals.

Tested with Images: mmx233/neko:base mmx233/neko:google-chrome

@m1k1o
Copy link
Owner

m1k1o commented Dec 28, 2024

For v3 we already have heartbeat but it is not configurable - const in the code and it is sending pings from server to the client, therefore it would be great to have this feature ported to v3 as well.

I wonder if it actually makes difference for loadbalancers, whether client sends the heartbeat or server. Alternatively, client could just reply to ping event from server, and we could avoid passing the configuration value to the client,

Similarly how we do ping-pong for webrtc in v3 from client to measure latency.

@Mmx233
Copy link
Contributor Author

Mmx233 commented Dec 28, 2024

For v3 we already have heartbeat but it is not configurable - const in the code and it is sending pings from server to the client, therefore it would be great to have this feature ported to v3 as well.

I wonder if it actually makes difference for loadbalancers, whether client sends the heartbeat or server. Alternatively, client could just reply to ping event from server, and we could avoid passing the configuration value to the client,

Similarly how we do ping-pong for webrtc in v3 from client to measure latency.

Regarding who should send heartbeats. From a product perspective, since the client is the consumer of the connection and is responsible for opening and closing it, and may suddenly disappear due to network issues without properly closing the TCP connection, the client should proactively maintain the connection by sending heartbeats. Additionally, from a technical standpoint, this approach can conserve more resources. Having the client send heartbeats can save one goroutine per connection. When using gorilla/websocket, the server only needs to set the HandshakeTimeout in the upgrader to automatically clean up unused connections:

upgrader: websocket.Upgrader{
	CheckOrigin: func(r *http.Request) bool {
		return true
	},
	HandshakeTimeout: time.Minute * 5,   // <--- add this line
},

Regarding the minimum heartbeat frequency required for the entire connection link, the client cannot determine this. Therefore, the heartbeat frequency should be set by the mainetainer of the server, rather than make it configurable in the client settings or writing it in the code.

In WebSocket, successfully receiving or sending a message without error already indicates that the connection is not in an abnormal state. Therefore, I believe the ping-pong mechanism is unnecessary. In addition, once read method of gorilla/websocket connection returns an error, ReadMessage and WriteMessage will no longer succeed, this will happen when either party initiates a connection close or the client exceeds the HandshakeTimeout. Therefore, it is unnecessary for the server to attempt sending ping messages anymore when the connection is returning errors. This means that in the scenario of maintaining a connection, there is no need to use the ping-pong mechanism. What we can do is sending heartbeat messages from one side.

https://github.com/gorilla/websocket/blob/5e002381133d322c5f1305d171f3bdd07decf229/conn.go#L1005-L1008

// Applications must break out of the application's read loop when this method
// returns a non-nil error value. Errors returned from this method are
// permanent. Once this method returns a non-nil error, all subsequent calls to
// this method return the same error.
func (c *Conn) NextReader() (messageType int, r io.Reader, err error) {

m1k1o added a commit that referenced this pull request Dec 30, 2024
@m1k1o m1k1o mentioned this pull request Dec 30, 2024
41 tasks
m1k1o added a commit that referenced this pull request Dec 30, 2024
@m1k1o
Copy link
Owner

m1k1o commented Dec 30, 2024

@Mmx233 thank you for the explanation. I ported the changed to v3 so we can get this merged.

@m1k1o m1k1o merged commit e3b6e00 into m1k1o:master Dec 30, 2024
@Mmx233 Mmx233 deleted the fix/ws_alive branch December 30, 2024 14:48
Mmx233 added a commit to Mmx233/neko that referenced this pull request Jan 7, 2025
feat(general): add heartbeat to websocket control (m1k1o#460)
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.

2 participants