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.

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: handle websocket closed correctly #3565

Merged
merged 1 commit into from
Sep 7, 2024
Merged

Conversation

KhafraDev
Copy link
Member

Fixes #3546
Closes #3548

@KhafraDev KhafraDev marked this pull request as ready for review September 7, 2024 21:15
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Uzlopak Uzlopak merged commit 57f1b4e into nodejs:main Sep 7, 2024
33 checks passed
@Uzlopak Uzlopak changed the title handle websocket closed correctly fix: handle websocket closed correctly Sep 7, 2024
@KhafraDev KhafraDev deleted the fix-is-closed branch September 7, 2024 21:52
@eXhumer
Copy link
Contributor

eXhumer commented Sep 8, 2024

Related issue needs to be re-opened as I don't think issue is fixed. Infact, the test was modified which doesn't check the close event firing.

Issue re-producible with the following code

import { WebSocket } from 'undici';

const ws = new WebSocket('ws://localhost:8080');

ws.onclose = e => {
  console.log('close', e);
};

ws.onerror = e => {
  console.log('error', e);
};

Console output

➜  test-websocket git:(main) ✗ yarn ts-node test.ts 
yarn run v1.22.22
$ /Users/exhumer/Projects/test-websocket/node_modules/.bin/ts-node test.ts
error ErrorEvent {
  type: 'error',
  defaultPrevented: false,
  cancelable: false,
  timeStamp: 718.825333
}
✨  Done in 1.08s.

const { tspl } = require('@matteo.collina/tspl')

test('first error than close event is fired on failed connection', async (t) => {
const { completed, strictEqual } = tspl(t, { plan: 2 })
Copy link
Contributor

@eXhumer eXhumer Sep 8, 2024

Choose a reason for hiding this comment

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

{ plan: 2 } skips close event check

@eXhumer
Copy link
Contributor

eXhumer commented Sep 8, 2024

Test output with fixed websocket test.

➜  undici git:(main) yarn test:websocket
yarn run v1.22.22
$ borp -p "test/websocket/*.js"
▶ WebSocketInit
  ✔ WebSocketInit as 2nd param (37.936791ms)
▶ WebSocketInit (38.6495ms)
▶ isValidSubprotocol
  ✔ empty string returns false (2.16575ms)
  ✔ simple valid value returns false (1.865417ms)
  ✔ empty string returns false (1.099334ms)
  ✔ value with "(),/:;<=>?@[\]{} returns false (0.762792ms)
▶ isValidSubprotocol (6.949709ms)
✔ Sending >= 2^16 bytes (43.259375ms)
✔ Sending >= 126, < 2^16 bytes (6.401209ms)
✔ Sending < 126 bytes (4.908042ms)
✔ Sending data after close (8.025542ms)
✔ Sending data before connected (2.06475ms)
▶ Sending data to a server
  ✔ Send with string (11.411375ms)
  ✔ Send with ArrayBuffer (8.749917ms)
  ✔ Send with Blob (13.136917ms)
  ✔ Cannot send with SharedArrayBuffer (12.56ms)
▶ Sending data to a server (46.437ms)
✔ check cloned (44.237375ms)
✔ Receiving a frame with a payload length > 2^31-1 bytes (39.807084ms)
✔ Receiving an ArrayBuffer (7.472417ms)
✔ Receives ping and parses body (40.736ms)
✔ Receives pong and parses body (6.461583ms)
✔ WebSocket connecting to server that isn't a Websocket server (39.134292ms)
✔ Open event is emitted (6.276667ms)
✔ Multiple protocols are joined by a comma (5.4405ms)
✔ Server doesn't send Sec-WebSocket-Protocol header when protocols are used (10.549625ms)
✔ Server sends invalid Upgrade header (4.053292ms)
✔ Server sends invalid Connection header (5.831458ms)
✔ Server sends invalid Sec-WebSocket-Accept header (12.565042ms)
✔ Server sends invalid Sec-WebSocket-Extensions header (7.32725ms)
✔ Server sends invalid Sec-WebSocket-Extensions header (15.754334ms)
✔ test/parallel/test-messageevent-brandcheck.js (2.147916ms)
✔ test/parallel/test-worker-message-port.js (3.207208ms)
✔ bug in node core (0.144083ms)
✖ first error than close event is fired on failed connection (8.1675ms)
  'Promise resolution is still pending but the event loop has already resolved'

✔ readyState is set on fail (5.174041ms)
✔ Receiving frame with payload length 0 works (25.618208ms)
✔ Fragmented frame with a ping frame in the first of it (24.409375ms)
✔ The server must reply with at least one subprotocol the client sends (36.308958ms)
✔ The connection fails when the client sends subprotocols that the server does not responc with (9.595459ms)
✔ Close without receiving code does not send an invalid payload (3029.1465ms)
✔ Don not use pooled buffer in mask pool (1.11575ms)
✔ Writing 16-bit frame length value at correct offset when buffer has a non-zero byteOffset (0.320625ms)
✔ Fragmented frame with a ping frame in the middle of it (34.053959ms)
✔ MessageEvent (1.460375ms)
✔ CloseEvent (0.304583ms)
✔ ErrorEvent (0.1585ms)
▶ Event handlers
  ▶ onopen
    ✔ should be null initially (0.119375ms)
    ✔ should not allow non-function assignments (0.071ms)
    ✔ should allow function assignments (0.122375ms)
  ▶ onopen (0.424584ms)
  ▶ onerror
    ✔ should be null initially (0.156708ms)
    ✔ should not allow non-function assignments (0.193292ms)
    ✔ should allow function assignments (0.250833ms)
  ▶ onerror (0.908125ms)
  ▶ onclose
    ✔ should be null initially (0.113292ms)
    ✔ should not allow non-function assignments (0.05725ms)
    ✔ should allow function assignments (0.04375ms)
  ▶ onclose (0.272625ms)
  ▶ onmessage
    ✔ should be null initially (0.062375ms)
    ✔ should not allow non-function assignments (0.043625ms)
    ✔ should allow function assignments (0.034375ms)
  ▶ onmessage (0.231541ms)
▶ Event handlers (4.203041ms)
▶ CloseEvent WPTs ported
  ✔ initCloseEvent (0.073625ms)
  ✔ CloseEvent constructor (0.096833ms)
▶ CloseEvent WPTs ported (0.225792ms)
▶ ErrorEvent WPTs ported
  ✔ Synthetic ErrorEvent (0.173583ms)
  ✔ webidl (0.062625ms)
  ✔ initErrorEvent (0.034625ms)
▶ ErrorEvent WPTs ported (0.318875ms)
▶ diagnostics channel
  ✔ undici:websocket:open (36.164959ms)
  ✔ undici:websocket:close (5.774417ms)
▶ diagnostics channel (43.490792ms)
✔ Setting custom headers (3.430959ms)
✔ Receiving multiple continuation frames works as expected (28.503459ms)
✔ Constructor (1.908542ms)
▶ Close
  ✔ Close with code (23.061959ms)
  ✔ Close with code and reason (3.295125ms)
  ✔ Close with invalid code (3.016375ms)
  ✔ Close with invalid reason (4.419167ms)
  ✔ Close with no code or reason (3.371834ms)
  ✔ Close with a 3000 status code (2.238459ms)
  ✔ calling close twice will only trigger the close event once (2.692875ms)
▶ Close (43.034666ms)
✔ Receiving a close frame with invalid utf-8 (25.635125ms)
✔ Client fails the connection if receiving a masked frame (24.697625ms)
✔ Client fails the connection if receiving a masked frame (23.566167ms)
ℹ tests 76
ℹ suites 12
ℹ pass 75
ℹ fail 0
ℹ cancelled 1
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 3528.956583

✖ failing tests:

test at test/websocket/issue-3546.js:7:1
✖ first error than close event is fired on failed connection (8.1675ms)
  'Promise resolution is still pending but the event loop has already resolved'
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
➜  undici git:(main) ✗

Uzlopak pushed a commit that referenced this pull request Sep 20, 2024
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebSockets do not fire 'close' event if the connection failed to be established
3 participants