-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add a watchdog for the heartbeat #601
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
base: main
Are you sure you want to change the base?
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.
thanks for the patch, can you please add some tests for the change?
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.
Pull Request Overview
This PR adds a watchdog timer mechanism to detect lost WebSocket connections that aren't properly detected by the socket client. The watchdog is reset by heartbeat messages (which occur every 25 seconds) and triggers a connection close if no heartbeat is received within 30 seconds.
- Adds a 30-second timeout constant for the watchdog mechanism
- Implements a watchdog callback function that closes the connection with code 1006
- Sets up and resets the watchdog timer on heartbeat messages and connection open
SockJS.prototype._websocketWatchdogCb = function(websocket) { | ||
debug("watchdog websocket timeout"); | ||
websocket._close(1006, 'Server lost session'); |
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 parameter name 'websocket' is misleading as it refers to the SockJS instance, not a WebSocket. This should be 'sockjs' or 'self' to accurately reflect what it represents.
SockJS.prototype._websocketWatchdogCb = function(websocket) { | |
debug("watchdog websocket timeout"); | |
websocket._close(1006, 'Server lost session'); | |
SockJS.prototype._websocketWatchdogCb = function(sockjs) { | |
debug("watchdog websocket timeout"); | |
sockjs._close(1006, 'Server lost session'); |
Copilot uses AI. Check for mistakes.
@@ -257,6 +263,8 @@ SockJS.prototype._transportMessage = function(msg) { | |||
case 'h': | |||
this.dispatchEvent(new Event('heartbeat')); | |||
debug('heartbeat', this.transport); | |||
clearTimeout(this.heartbeatWatchdog); | |||
this.heartbeatWatchdog = setTimeout(this._websocketWatchdogCb, SockJS.WATCHDOG_WEBSOCKET_TIMEOUT, this); |
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 watchdog timer should be cleared when the connection is closed to prevent potential memory leaks and unexpected behavior. Consider adding clearTimeout(this.heartbeatWatchdog) in the _close method.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
If your client device loose the network connection to the device running the websocket server this will not be detected by the socket client.
This watchdog will be reseted by the heartbeat message (appears every 25 seconds) if the watchdog will not be resetted in a 30 second window the connection can be marked as lost => 1006
Found with OctoPrint, if you connect to the OctoPrint instance with a laptop and this goes to sleep mode for some time (network will be disabled)
After you wake the device again you still see the UI but the websocket connection is not connected anymore.
You can reproduce this by disabling your network on the laptop, wait some time and connect again.