chore: realtime api improvements #1340
Conversation
|
Summary By MatterAI
🔄 What ChangedThis pull request updates the 🔍 Impact of the ChangeThese changes significantly enhance the reliability and robustness of the real-time API. By ensuring the upstream WebSocket connection is established before use, potential race conditions are mitigated. The improved error management, including explicit listener removal and graceful close handling, reduces the likelihood of unhandled errors and resource leaks, leading to a more stable and predictable real-time communication system. 📁 Total Files Changed
🧪 Test AddedN/A 🔒Security VulnerabilitiesN/A MotivationTo improve the reliability and error handling of the real-time API by ensuring upstream WebSocket connections are established and managing errors more robustly. Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
totalScore: 3 Related IssuesN/A Tip Quality Recommendations
Tanka Poem ♫
Sequence DiagramsequenceDiagram
participant Client
participant Gateway as Hono Node Server
participant RealtimeHandler as realTimeHandlerNode
participant WebSocketUtils as websocketUtils
participant UpstreamAPI as Upstream WebSocket API
Client->>Gateway: WebSocket Upgrade Request
Gateway->>RealtimeHandler: upgradeWebSocket(realTimeHandlerNode)
RealtimeHandler->>UpstreamAPI: new WebSocket(url)
Note over RealtimeHandler: Creates outgoingWebSocket
RealtimeHandler->>RealtimeHandler: checkWebSocketOpen = new Promise()
RealtimeHandler->>UpstreamAPI: Add 'open' event listener
UpstreamAPI-->>RealtimeHandler: 'open' event
RealtimeHandler-->>RealtimeHandler: resolve(checkWebSocketOpen)
RealtimeHandler->>RealtimeHandler: await checkWebSocketOpen
RealtimeHandler->>Gateway: Return WebSocket handlers (onOpen, onMessage, onClose)
Gateway->>WebSocketUtils: setupWebSocketListeners(incomingWebsocket, outgoingWebSocket)
Note over WebSocketUtils: Configures error and close listeners
WebSocketUtils->>UpstreamAPI: Add 'close' listener
WebSocketUtils->>UpstreamAPI: Add 'error' listener (errorListener)
WebSocketUtils->>Gateway: Add 'error' listener (errorListener)
alt Outgoing WebSocket Error
UpstreamAPI-->>WebSocketUtils: 'error' event (ErrorEvent)
WebSocketUtils->>Gateway: console.error('outgoingWebSocket error', event)
WebSocketUtils->>Gateway: server?.close()
else Outgoing WebSocket Close
UpstreamAPI-->>WebSocketUtils: 'close' event (event.code, event.reason)
WebSocketUtils->>Gateway: removeEventListener('error', errorListener)
opt Normal Close (code 1005)
WebSocketUtils->>Gateway: server?.close()
else Other Close
WebSocketUtils->>Gateway: server?.close(event.code, event.reason)
end
end
alt Incoming WebSocket Error
Gateway-->>WebSocketUtils: 'error' event (ErrorEvent)
WebSocketUtils->>Gateway: console.error('serverWebSocket error', event)
WebSocketUtils->>UpstreamAPI: outgoingWebSocket?.close()
end
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| "@cfworker/json-schema": "^4.0.3", | ||
| "@hono/node-server": "^1.3.3", | ||
| "@hono/node-ws": "^1.0.4", | ||
| "@hono/node-ws": "^1.2.0", |
There was a problem hiding this comment.
📦 Dependency Issue
Issue: Updated @hono/node-ws from 1.0.4 to 1.2.0
Fix: Verify compatibility with existing WebSocket usage
Impact: Ensures latest bug fixes and features
| "@hono/node-ws": "^1.2.0", | |
| "@hono/node-ws": "^1.2.0", |
| // wait for the upstream websocket to be open | ||
| const checkWebSocketOpen = new Promise((resolve) => { | ||
| outgoingWebSocket.addEventListener('open', () => { | ||
| resolve(true); | ||
| }); | ||
| }); | ||
|
|
||
| await checkWebSocketOpen; |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Added promise to wait for upstream WebSocket to be open
Fix: Ensures proper connection sequence before proceeding
Impact: Improves reliability of WebSocket connections
| // wait for the upstream websocket to be open | |
| const checkWebSocketOpen = new Promise((resolve) => { | |
| outgoingWebSocket.addEventListener('open', () => { | |
| resolve(true); | |
| }); | |
| }); | |
| await checkWebSocketOpen; | |
| // wait for the upstream websocket to be open | |
| const checkWebSocketOpen = new Promise((resolve) => { | |
| outgoingWebSocket.addEventListener('open', () => { | |
| resolve(true); | |
| }); | |
| }); | |
| await checkWebSocketOpen; |
| const errorListener = (event: ErrorEvent) => { | ||
| console.error('outgoingWebSocket error: ', event); | ||
| server?.close(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Added error listener for outgoing WebSocket and proper cleanup
Fix: Centralizes error handling and removes listener on close
Impact: Prevents memory leaks and improves error handling
| const errorListener = (event: ErrorEvent) => { | |
| console.error('outgoingWebSocket error: ', event); | |
| server?.close(); | |
| }; | |
| const errorListener = (event: ErrorEvent) => { | |
| console.error('outgoingWebSocket error: ', event); | |
| server?.close(); | |
| }; | |
| server.addEventListener('error', errorListener); |
| console.error('serverWebSocket error: ', event); | ||
| outgoingWebSocket?.close(); | ||
| }); | ||
| server.addEventListener('error', errorListener); |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Removed duplicate error listener and centralized it
Fix: Avoids duplicate error handling logic
Impact: Reduces code duplication and improves maintainability
| server.addEventListener('error', errorListener); | |
| server.addEventListener('error', errorListener); |
2 participants
Description
Motivation
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Related Issues