Ping over the DTD WebSocket every 15s to avoid some proxies (like Norton) dropping the connection #5828
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
…ton) dropping the connection Users with Norton 360 installed found that if there is no traffic over the WebSocket for 60s, it gets dropped. This appears to be that the connections are proxied (even if all scanning is disabled) and "idle" connections are dropped. This forces traffic ever 15s which will prevent them from dropping the connection. Fixes #5794
4e83106 to
6534470
Compare
|
@codex review |
|
I've received your request for a code review. Here's a summary of my findings for this pull request: Overall Impression: Key Points:
No critical or high-severity issues were identified. The changes are a solid improvement for connection stability. If you intended to trigger a full, line-by-line review with specific suggestions, please note that this comment provides a high-level summary. For a detailed review, you would typically initiate a new review submission through the platform's review features. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| describe("attachPing", () => { | ||
| let server: http.Server; | ||
| let wss: ws.Server; | ||
|
|
||
| beforeEach(() => { | ||
| server = http.createServer(); | ||
| wss = new ws.Server({ server }); | ||
| server.listen(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| wss.close(); | ||
| server.close(); | ||
| }); | ||
|
|
||
| it("sends pings", async () => { | ||
| const serverReceivedPingPromise = new Promise((resolve) => wss.once("connection", (c) => c.once("ping", resolve))); | ||
|
|
||
| const port = (server.address() as any).port; | ||
| const socket = new ws.WebSocket(`ws://localhost:${port}`); | ||
| attachPing(socket, 10); // 10ms interval to avoid test taking long. | ||
|
|
||
| // Wait for the server to get a ping. | ||
| await serverReceivedPingPromise; | ||
|
|
||
| socket.close(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[nitpick] The test suite only covers the happy path (sending pings). Consider adding tests for edge cases such as:
- Socket closing before/after ping starts - verify cleanup happens correctly
- Socket error during ping - verify error handler clears the interval
- Attaching ping to an already-open socket vs. a connecting socket
These additional tests would ensure the cleanup logic (removeListener calls) and error handling work correctly.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Users with Norton 360 installed found that if there is no traffic over the WebSocket for 60s, it gets dropped. This appears to be that the connections are proxied (even if all scanning is disabled) and "idle" connections are dropped.
This forces traffic ever 15s which will prevent them from dropping the connection.
Fixes #5794