-
Notifications
You must be signed in to change notification settings - Fork 42
Fix WebSocket ping/pong to use protocol frames instead of text messages #25
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
Changed the WebSocket ping/pong implementation to use proper WebSocket
protocol frames rather than sending text "ping"/"pong" messages. The
server was responding to WebSocket protocol pings, not text messages,
causing the previous implementation to fail silently.
Changes:
- Replaced `this.ws.send("ping")` with `this.ws.ping()` to send
protocol-level ping frames
- Updated pong listener from `this.ws.pong` assignment to proper event
listener `this.ws.on('pong', this.onPong)` for the 'ws' library
- Added conditional check for `this.ws.on` method availability before
attaching pong listener to ensure compatibility
This ensures the client correctly implements the WebSocket ping/pong
heartbeat mechanism as defined in RFC 6455, allowing proper connection
health monitoring.
|
|
||
| this.ws.send("ping", (err: Error | undefined) => { | ||
| this.ws.ping((err: Error | undefined) => { |
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.
Bug: Ping method unavailable in browser WebSocket environments
The code uses isomorphic-ws for cross-platform WebSocket support, but the this.ws.ping() method only exists in the Node.js ws library, not in browser WebSocket implementations. While the code correctly checks if (this.ws.on) before attaching the pong listener (acknowledging browser incompatibility), no equivalent check exists for this.ws.ping(). In browser environments, calling this.ws.ping() will fail because the method doesn't exist, breaking the heartbeat mechanism entirely.
Additional Locations (1)
|
Did this solve the issue with the freeze after around 20 minutes you mentioned before where the datastream stopped? @feldblick |
|
No unfortunately not. It just sends out the necessary ping properly. The automatic review might be right though. I tested it with the ws lib. Isomorphic-ws cannot handle ping pong properly as it seems but I haven't digged deeply into it. |
|
Ran it for approx 12 hours and here are some statistics: { Time span monitored: ~11.5 hours One temporary way to handle this is with 10-minute proactive reconnect: We should try to find a way to get the socket connection stable. |
Let's continue here with the discussion, as I don't think it's ping pong related: #26 |
Changed the WebSocket ping/pong implementation to use proper WebSocket protocol frames rather than sending text "ping"/"pong" messages. The server was responding to WebSocket protocol pings, not text messages, causing the previous implementation to fail silently.
Changes:
this.ws.send("ping")withthis.ws.ping()to send protocol-level ping framesthis.ws.pongassignment to proper event listenerthis.ws.on('pong', this.onPong)for the 'ws' librarythis.ws.onmethod availability before attaching pong listener to ensure compatibilityThis ensures the client correctly implements the WebSocket ping/pong heartbeat mechanism as defined in RFC 6455, allowing proper connection health monitoring.
Note
Switches the client to protocol-level ping/pong with a guarded 'pong' event listener for compatibility with the ws library.
src/client.ts):this.ws.send("ping")with protocol-levelthis.ws.ping(...).this.ws.on('pong', this.onPong)(only ifonexists); remove directthis.ws.pongassignment.Written by Cursor Bugbot for commit 04ada4a. This will update automatically on new commits. Configure here.