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

WebSockets do not fire 'close' event if the connection failed to be established #3546

Open
marximimus opened this issue Sep 4, 2024 · 4 comments · Fixed by #3565 · May be fixed by #3566
Open

WebSockets do not fire 'close' event if the connection failed to be established #3546

marximimus opened this issue Sep 4, 2024 · 4 comments · Fixed by #3565 · May be fixed by #3566
Labels
bug Something isn't working

Comments

@marximimus
Copy link

Consider the following code:

const webSocket = new undici.WebSocket("wss://invalid-domain.example.com/");
webSocket.onopen = () => { console.log("open"); };
webSocket.onclose = () => { console.log("close"); };
webSocket.onerror = () => { console.log("error"); };

It outputs:

error

However, a standard-compliant WebSocket implementation would output:

error
close

This is because establish a WebSocket connection algorithm would invoke fail the WebSocket Connection algorithm in step 11.1, which would Close the WebSocket Connection. The WebSocket standard states:

When the WebSocket connection is closed, possibly cleanly, the user agent must queue a task to run the following substeps:
...
3. Fire an event named close at the WebSocket object.

This bug is reproducible on commit 4b8958a.
@marximimus marximimus added the bug Something isn't working label Sep 4, 2024
@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 4, 2024

Is it connected to #3506 and maybe already fixed in main but not released yet?

@eXhumer
Copy link
Contributor

eXhumer commented Sep 4, 2024 •
edited
Loading

The close event is never fired even though the readyState is set to 3 (WebSocket.CLOSED). Tested on latest commit (4b8958a) on main.

Code

import { WebSocket } from 'undici';

const webSocket = new WebSocket("wss://invalid-domain.example.com/");
webSocket.onopen = () => { console.log("open"); };
webSocket.onclose = () => { console.log("close"); };
webSocket.onerror = () => {
  console.log("error");
  console.log("readyState", webSocket.readyState);
};

Console output

➜  test-issue git:(main) ✗ yarn ts-node src/index.ts
yarn run v1.22.22
$ /Users/exhumer/test-issue/node_modules/.bin/ts-node src/index.ts
error
readyState 3
✨  Done in 1.00s.

On v6.x branch (latest stable release branch), the readyState is instead at 0 (WebSocket.CONNECTING) at error. Still no close event is fired.

@eXhumer
Copy link
Contributor

eXhumer commented Sep 4, 2024

Trying the code in a local HTML page on a browser, a close event is fired after the error event.

test-issue-browser.html

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
  </head>
  <body></body>
  <script>
    const webSocket = new WebSocket('ws://localhost:8080')
    webSocket.onopen = () => { console.log("open"); };
    webSocket.onclose = () => { console.log("close"); };
    webSocket.onerror = () => {
      console.log("error");
      console.log("readyState", webSocket.readyState);
    };
  </script>
</html>

Console output
Screenshot 2024-09-04 at 3 07 40 PM

@eXhumer
Copy link
Contributor

eXhumer commented Sep 4, 2024 •
edited
Loading

This can be fixed by changing the the following.

undici/lib/web/websocket/websocket.js

Lines 527 to 533 in 4b8958a

if (reason) {
// TODO: process.nextTick
fireEvent('error', this, (type, init) => new ErrorEvent(type, init), {
error: new Error(reason),
message: reason
})
}

First, adding a this.#handler.onSocketClose() or this.#onSocketClose() after the if block to fire the error event.

And then changing this line

undici/lib/web/websocket/websocket.js

Line 669 in 4b8958a

const result = this.#parser.closingInfo

to const result = this.#parser?.closingInfo as this.#parser is undefined when it is accessed before a connection is established.

I can open a PR to fix with the mentioned changes if it is considered acceptable fix.

eXhumer added a commit to eXhumer/undici that referenced this issue Sep 4, 2024
@eXhumer
fix: fire close on fail for WebSocket
a0e1f8e 
* see nodejs#3546

Signed-off-by: eXhumer <[email protected]>
@eXhumer eXhumer mentioned this issue Sep 4, 2024
Closed
5 tasks
eXhumer added a commit to eXhumer/undici that referenced this issue Sep 5, 2024
@eXhumer
chore: add test related to nodejs#3546
e7beb86 
Signed-off-by: eXhumer <[email protected]>
eXhumer added a commit to eXhumer/undici that referenced this issue Sep 5, 2024
@eXhumer
chore: add test related to nodejs#3546
5e2cd77 
* updated with requested changes

Signed-off-by: eXhumer <[email protected]>
@KhafraDev KhafraDev mentioned this issue Sep 7, 2024
Merged
eXhumer added a commit to eXhumer/undici that referenced this issue Sep 8, 2024
@eXhumer
fix: fire close on failed WebSocket connection
6818acc 
* see nodejs#3546, nodejs#3548 & nodejs#3565

Signed-off-by: eXhumer <[email protected]>
@eXhumer eXhumer linked a pull request Sep 8, 2024 that will close this issue
Open
5 tasks
@Uzlopak Uzlopak reopened this Sep 12, 2024
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
3 participants
@Uzlopak @marximimus @eXhumer