fix(node): destroy socket on upgrade abort

[teemingc]

fixes #139

This PR is an attempt to to avoid the error that's occurring on Windows from calling socket.write(chunk) by destroying the socket stream after all data has been written. It copies the Socket.destroySoon() implementation from Node.js.

Also worth noting that the ws examples use socket.destroy() to end their upgrade requests:

• https://github.com/websockets/ws#multiple-servers-sharing-a-single-https-server
• https://github.com/websockets/ws#client-authentication

And the duplex stream/socket class uses the Writable.destroy() implementation:
https://nodejs.org/api/stream.html#writabledestroyerror

EDIT: seems like ws also calls socket.destroy() when sending their responses too, according to their implementation

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@39485dc). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #140   +/-   ##
=======================================
  Coverage        ?   76.59%           
=======================================
  Files           ?        9           
  Lines           ?      735           
  Branches        ?      147           
=======================================
  Hits            ?      563           
  Misses          ?      170           
  Partials        ?        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[luke-hagar-sp]

@pi0 Can you review this when you get a chance please good Sir?

[pi0]

Sure :) I have to double check socket.end(); behavior from Node.js impl since we are calling it outside promise otherwise latest changes look good.

[pi0]

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.

[teemingc]

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

> crossws@0.3.4 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.

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

[pi0]

thanks for update and sorry for delay!