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

Jump to bottom

Returning a response with a body during an upgrade crashes the Node http server on Windows #139

Open
eltigerchino opened this issue Feb 14, 2025 · 2 comments · May be fixed by #140
Open

Returning a response with a body during an upgrade crashes the Node http server on Windows #139

eltigerchino opened this issue Feb 14, 2025 · 2 comments · May be fixed by #140
Labels
bug Something isn't working

Comments

@eltigerchino
Copy link
Contributor

eltigerchino commented Feb 14, 2025 •
edited
Loading

Environment

Windows 11 24H2
Node.js v18.20.5
[email protected]

Reproduction

https://github.com/eltigerchino/crossws-windows-socket-error-repro
Reproduction steps included in the repository README.md

Describe the bug

Returning/throwing a Response that has a body during the upgrade hook on a Windows machine causes the Node http server to crash.

Additional context

Related function:

crossws/src/adapters/node.ts

Lines 203 to 220 in cad4a65

async function sendResponse(socket: Duplex, res: Response) {
const head = [
`HTTP/1.1 ${res.status || 200} ${res.statusText || ""}`,
...[...res.headers.entries()].map(
([key, value]) =>
`${encodeURIComponent(key)}: ${encodeURIComponent(value)}`,
),
];
socket.write(head.join("\r\n") + "\r\n\r\n");
if (res.body) {
for await (const chunk of res.body) {
socket.write(chunk);
}
}
return new Promise<void>((resolve) => {
socket.end(resolve);
});
}

I experimented a bit and calling socket.destroy() after the upgrade seems to prevent the crash but I don't think that's the right solution?

I also tried awaiting the socket.write(chunk) operations by passing it a callback that resolves a Promise but that doesn't solve the issue either (it just ensures writes are all done before socket.end())

Logs

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.
@eltigerchino eltigerchino added the bug Something isn't working label Feb 14, 2025
@pi0
Copy link
Member

pi0 commented Feb 14, 2025

Thanks for report. I also randomly saw this issue non windows.

Calling socket.destroy seems logical (will have to confirm later) why do you think is not a right choice?

(feel free to open a draft PR)

@eltigerchino
Copy link
Contributor Author

eltigerchino commented Feb 14, 2025 •
edited
Loading

Calling socket.destroy seems logical (will have to confirm later) why do you think is not a right choice?

I think I misunderstood the API. I got the impression that socket.end is preferred over socket.destroy when googling around the other day but I guess that's not the case.

Maybe sendResponse needs to return something like this? (I'm not sure about the call order/sync/async nature of the operations):

  return new Promise((resolve) => {
    socket.end(() => {
      socket.destroy();
      resolve(null);
    });
  });

Destroy the stream. Optionally emit an 'error' event, and emit a 'close' event (unless emitClose is set to false). After this call, the readable stream will release any internal resources and subsequent calls to push() will be ignored.

https://nodejs.org/api/stream.html#readabledestroyerror

By default, stream.end() is called on the destination Writable stream when the source Readable stream emits 'end', so that the destination is no longer writable.

https://nodejs.org/api/stream.html#readablepipedestination-options

This was referenced Feb 17, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: avoid Error: read ECONNRESET on Windows when sending response eltigerchino/crossws
2 participants
@pi0 @eltigerchino