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.

Sign up for GitHub

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

fix(node): destroy socket #140

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix(node): destroy socket #140

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c942c19
Update node.ts
eltigerchino Feb 12, 2025
d3e010f
chore: apply automated updates
autofix-ci[bot] Feb 12, 2025
04e82f8
Merge branch 'unjs:main' into main
eltigerchino Feb 17, 2025
8ea36f1
Update node.ts
eltigerchino Feb 17, 2025
40df44b
Update node.ts
eltigerchino Feb 18, 2025
a1f2538
Merge branch 'unjs:main' into main
eltigerchino Mar 10, 2025
ab68dab
destroy only after socket.end()
eltigerchino Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/adapters/node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,18 @@
socket.write(chunk);
}
}
if (socket.writable) {
socket.end();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still little worried that we are not using end callback on socket.

This way, handleUpgrade promise is resolved even before socket closes.

Do you have a stable reproduction that all this changes fixes it? (asking cause I tried closing socket on windows by enabling this condition => pnpm play:node. i can't see errors on windows.

Copy link
Contributor Author

@eltigerchino eltigerchino Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the code so that we destroy the socket in the socket.end() callback.

I've also updated the reproduction from the issue with the solution that fixes the issue https://github.com/eltigerchino/crossws-windows-socket-error-repro . There's a line you can uncomment to destroy the socket to fix it.

Strangely, I can reproduce this error on Windows by running pnpm play:node then sending a WebSocket request through Hoppscotch at the URL http://localhost:3001/?unauthorized.

PS C:\git-windows\crossws> pnpm play:node

> [email protected] play:node C:\git-windows\crossws
> jiti test/fixture/node.ts

Server running at http://localhost:3001/
node:events:495
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:217:20)
Emitted 'error' event on Socket instance at:
    at emitErrorNT (node:internal/streams/destroy:151:8)
    at emitErrorCloseNT (node:internal/streams/destroy:116:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  errno: -4077,
  code: 'ECONNRESET',
  syscall: 'read'
}

Node.js v18.20.5
 ELIFECYCLE  Command failed with exit code 1.

}
return new Promise<void>((resolve) => {
socket.end(resolve);
const cleanup = () => {
socket.destroy();
resolve();
};
if (socket.writableFinished) {
cleanup();

Check warning on line 226 in src/adapters/node.ts

View check run for this annotation

Codecov / codecov/patch

src/adapters/node.ts#L226 

Added line #L226 was not covered by tests
} else {
socket.once("finish", cleanup);
}
});
}