fix: fire close on failed WebSocket connection

[eXhumer]

This relates to...

See #3546, #3548 & #3565

CC: @Uzlopak @KhafraDev

Rationale

#3565 attempted to fix #3546, however the test for #3546 made in #3548 was modified in #3565 and skipped the close event check, which ultimately ended up with not actually fixing the original issue. This PR will check the close event correctly and correctly fixes the original issue.

Additionally, this commit has been already tested to make sure it passes the autobahn testsuite

Changes

• correctly fire close event if there is no successful connection to server.
• fix test to actually check the close event correctly firing.

Features

N/A

Bug Fixes

• correctly fire close event on unsuccessful connection

Breaking Changes and Deprecations

N/A

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• [S] Benchmarked (optional)
• [S] Documented
• Review ready
• In review
• Merge ready

[eXhumer]

Not sure why the Node CI / test (20, macos-latest) test failed, but it should work on re-run. Had a similar issue before.

[eXhumer]

Some details on what is actually the issue.

The issue revolves around why the event is not being fired on unsuccessful connection. It is because the function that does the event firing is linked to an object that does not exist before a connection is actually successfully established. WebSocket.#parser is only initialized in WebSocket.#onConnectionEstablished.

undici/lib/web/websocket/websocket.js

Lines 481 to 490 in 6818acc

	#onConnectionEstablished (response, parsedExtensions) {
	// processResponse is called when the "response’s header list has been received and initialized."
	// once this happens, the connection is open
	this.#handler.socket = response.socket
	const parser = new ByteParser(this.#handler, parsedExtensions)
	parser.on('drain', () => this.#handler.onParserDrain())
	parser.on('error', (err) => this.#handler.onParserError(err))
	this.#parser = parser

[eXhumer]

Thoughts? Reviews? Any change requested?

The current implementation of WebSocket is not firing the close event at all on connection establish fail in any branch (this PR attempts to fix that case in main) and the error event is not being fired correctly on connection establish fail with the WebSocket.readyState being set to WebSocket.CONNECTING instead of WebSocket.CLOSED in latest release (v6.x branch) (fixed in main with #3507, not backported yet to v6.x). Currently implemented test at test/websocket/issue-3546.js bypasses the close event related checks added with #3565 by setting the number of plans to 2 instead of 4.

undici/test/websocket/issue-3546.js

Line 8 in 990df2c

const { completed, strictEqual } = tspl(t, { plan: 2 })

[KhafraDev]

I do not like this patch but I haven't had time to look into a real fix.

• The condition is too "smart" by relying on the order of parameters being initialized. Why can't it rely on handler.readyState being OPEN?
• It relies on the connection being failed, but a websocket can be closed without it failing.

[eXhumer]

• The condition is too "smart" by relying on the order of parameters being initialized. Why can't it rely on handler.readyState being OPEN?

AKA why not !isEstablished(this.#handler.readyState) or !isEstablished(this.#handler.readyState) && !this.#handler.receivedClose instead.

It is because it fails autobahn test suite, see https://github.com/eXhumer/undici/actions/runs/10837039351/job/30072121866 & https://github.com/eXhumer/undici/actions/runs/10837160880/job/30072507418

• It relies on the connection being failed, but a websocket can be closed without it failing.

Correct, but in a case where connection is closed without failing, it is instead handled by WebSocket.#onSocketClose instead, which is only executed when a connection is actually successfully established and WebSocket.#parser has been initialized

[eXhumer]

Currently, close event is only fired if WebSocket.#onSocketClose is executed.

And handler.onSocketClose (which then executes WebSocket.#onSocketClose) is attached to the socket only if a connection has been successfully established as the lines below are never reached if a connection is failed due to protocol error or network error.

undici/lib/web/websocket/connection.js

Lines 197 to 199 in 618005e

	response.socket.on('data', handler.onSocketData)
	response.socket.on('close', handler.onSocketClose)
	response.socket.on('error', handler.onSocketError)

Also, should the close code be 1002 if there is a connection establish error? Should it not be 1006 instead as the connection is not considered clean and a close frame is never received?

[Uzlopak]

I atleast reopened the issue.