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

WebSocket: "close" is not emitted on error #3697

Closed
kettanaito opened this issue Oct 7, 2024 · 8 comments
Closed

WebSocket: "close" is not emitted on error #3697

kettanaito opened this issue Oct 7, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@kettanaito
Copy link
Contributor

kettanaito commented Oct 7, 2024

Bug Description

When a WebSocket connection is closed due to an error, the close event doesn't get emitted.

Reproducible By

Run this in Node.js REPL (assumes undici is installed):

const { WebSocket } = require('undici')

let ws = new WebSocket('wss://example.com/non-existing-url')
ws.onerror = () => console.log('ERROR!')
ws.onclose = () => console.log('CLOSE!')

Expected Behavior

The WebSocket instance must dispatch two events in the following order:

ERROR!
CLOSE!

Actual behavior

It doesn't. Only the error event gets dispatched. The close event never gets dispatched, which I believe to be incorrect.

ERROR!

Logs & Screenshots

I can also verify that the browser implementation of WebSocket behaves correctly:

Screenshot 2024-10-07 at 11 38 33

Environment

I believe the environment is irrelevant here.

  • undici 6.6.2

Additional context

The error event is fired when a connection with a WebSocket has been closed due to an error (some data couldn't be sent for example).

@kettanaito kettanaito added the bug Something isn't working label Oct 7, 2024
@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 7, 2024

@KhafraDev

@kettanaito
Copy link
Contributor Author

I'm also open to contributing a fix to this if we conclude that it's indeed an issue! Would appreciate guidance with tests during the code review.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 7, 2024

@kettanaito

Imho it was already fixed by @KhafraDev via #3628 and #3651 .

Just not backported?!

@kettanaito
Copy link
Contributor Author

Oh, that sounds nice! I'm trying that on Node.js v18, I believe. I'd love to have a backport to that version.

@KhafraDev
Copy link
Member

backporting to v6 will be difficult because websocket has had significant rewrites

@kettanaito
Copy link
Contributor Author

@KhafraDev, understandable. Is v22 the minimal to use to get this fix then? (Undici v7)

@fawazahmed0
Copy link
Contributor

fawazahmed0 commented Oct 8, 2024

@KhafraDev There is also issue with readyState in undici@main package

let ws = new WebSocket('wss://example.com/non-existing-url')
ws.onerror = () => console.log('ERROR!')
ws.onclose = () => console.log('CLOSE!')
setTimeout(()=>console.log(ws.readyState),7000)

Undici@main Output:

ERROR!
CLOSE!
0

Firefox Output:

ERROR!
CLOSE!
3

Safari Output:

ERROR!
CLOSE!
3

chrome Output:

ERROR!
3

@KhafraDev
Copy link
Member

@fawazahmed0 I have opened a PR with a fix, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants