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

fix: fire close on failed WebSocket connection #3628

Merged
merged 3 commits into from
Sep 20, 2024
Merged

fix: fire close on failed WebSocket connection #3628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbedf67
remove PROCESSING
KhafraDev Sep 20, 2024
df0dd75
make close better
KhafraDev Sep 20, 2024
2699a9b
Merge branch 'main' into good-close
KhafraDev Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions lib/web/websocket/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@ const states = {
}

const sentCloseFrameState = {
NOT_SENT: 0,
PROCESSING: 1,
SENT: 2
SENT: 1,
RECEIVED: 2
}

const opcodes = {
Expand Down
18 changes: 7 additions & 11 deletions lib/web/websocket/receiver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,9 @@ class ByteParser extends Writable {
return false
}

if (this.#handler.closeState !== sentCloseFrameState.SENT) {
// Upon receiving such a frame, the other peer sends a
// Close frame in response, if it hasn't already sent one.
if (!this.#handler.closeState.has(sentCloseFrameState.SENT)) {
// If an endpoint receives a Close frame and did not previously send a
// Close frame, the endpoint MUST send a Close frame in response. (When
// sending a Close frame in response, the endpoint typically echos the
Expand All @@ -366,21 +368,15 @@ class ByteParser extends Writable {
}
const closeFrame = new WebsocketFrameSend(body)

this.#handler.socket.write(
closeFrame.createFrame(opcodes.CLOSE),
(err) => {
if (!err) {
this.#handler.closeState = sentCloseFrameState.SENT
}
}
)
this.#handler.socket.write(closeFrame.createFrame(opcodes.CLOSE))
this.#handler.closeState.add(sentCloseFrameState.SENT)
}

// Upon either sending or receiving a Close control frame, it is said
// that _The WebSocket Closing Handshake is Started_ and that the
// WebSocket connection is in the CLOSING state.
this.#handler.readyState = states.CLOSING
this.#handler.receivedClose = true
this.#handler.closeState.add(sentCloseFrameState.RECEIVED)

return false
} else if (opcode === opcodes.PING) {
Expand All @@ -389,7 +385,7 @@ class ByteParser extends Writable {
// A Pong frame sent in response to a Ping frame must have identical
// "Application data"

if (!this.#handler.receivedClose) {
if (!this.#handler.closeState.has(sentCloseFrameState.RECEIVED)) {
const frame = new WebsocketFrameSend(body)

this.#handler.socket.write(frame.createFrame(opcodes.PONG))
Expand Down
54 changes: 26 additions & 28 deletions lib/web/websocket/websocket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ const { channels } = require('../../core/diagnostics')
*
* @property {number} readyState
* @property {import('stream').Duplex} socket
* @property {number} closeState
* @property {boolean} receivedClose
* @property {Set<number>} closeState
*/

// https://websockets.spec.whatwg.org/#interface-definition
Expand Down Expand Up @@ -85,8 +84,7 @@ class WebSocket extends EventTarget {

readyState: states.CONNECTING,
socket: null,
closeState: sentCloseFrameState.NOT_SENT,
receivedClose: false
closeState: new Set()
}

#url
Expand Down Expand Up @@ -186,8 +184,6 @@ class WebSocket extends EventTarget {
// be CONNECTING (0).
this.#handler.readyState = WebSocket.CONNECTING

this.#handler.closeState = sentCloseFrameState.NOT_SENT

// The extensions attribute must initially return the empty string.

// The protocol attribute must initially return the empty string.
Expand Down Expand Up @@ -516,35 +512,33 @@ class WebSocket extends EventTarget {
}

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

// If _The WebSocket Connection is Established_ prior to the point where
// the endpoint is required to _Fail the WebSocket Connection_, the
// endpoint SHOULD send a Close frame with an appropriate status code
// (Section 7.4) before proceeding to _Close the WebSocket Connection_.
if (isEstablished(this.#handler.readyState)) {
this.#onClose(code, reason, reason?.length)
} else {
// If the WebSocket connection could not be established, it is also said
// that _The WebSocket Connection is Closed_, but not _cleanly_.
Copy link
Member Author

Choose a reason for hiding this comment

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

this was the original bug - we were handling cases when the connection was established and then closed, but not cases when the connection was not established

fireEvent('close', this, (type, init) => new CloseEvent(type, init), {
wasClean: false, code, reason
})
}

this.#controller.abort()

if (this.#handler.socket && !this.#handler.socket.destroyed) {
this.#handler.socket.destroy()
}

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

if (!this.#parser && !this.#handler.receivedClose) {
fireEvent('close', this, (type, init) => new CloseEvent(type, init), {
wasClean: false,
code: 1006,
reason
})
}
}

#onMessage (type, data) {
Expand Down Expand Up @@ -598,7 +592,11 @@ class WebSocket extends EventTarget {
// to CLOSING (2).
failWebsocketConnection(this.#handler, 1002, 'Connection was closed before it was established.')
this.#handler.readyState = states.CLOSING
} else if (this.#handler.closeState === sentCloseFrameState.NOT_SENT) {
} else if (!this.#handler.closeState.has(sentCloseFrameState.SENT) && !this.#handler.closeState.has(sentCloseFrameState.RECEIVED)) {
// Upon either sending or receiving a Close control frame, it is said
// that _The WebSocket Closing Handshake is Started_ and that the
// WebSocket connection is in the CLOSING state.

// If the WebSocket closing handshake has not yet been started
// Start the WebSocket closing handshake and set this's ready
// state to CLOSING (2).
Expand All @@ -609,8 +607,6 @@ class WebSocket extends EventTarget {
// - If reason is also present, then reasonBytes must be
// provided in the Close message after the status code.

this.#handler.closeState = sentCloseFrameState.PROCESSING

const frame = new WebsocketFrameSend()

// If neither code nor reason is present, the WebSocket Close
Expand All @@ -634,7 +630,7 @@ class WebSocket extends EventTarget {

this.#handler.socket.write(frame.createFrame(opcodes.CLOSE))

this.#handler.closeState = sentCloseFrameState.SENT
this.#handler.closeState.add(sentCloseFrameState.SENT)

// Upon either sending or receiving a Close control frame, it is said
// that _The WebSocket Closing Handshake is Started_ and that the
Expand Down Expand Up @@ -675,7 +671,9 @@ class WebSocket extends EventTarget {
// If the TCP connection was closed after the
// WebSocket closing handshake was completed, the WebSocket connection
// is said to have been closed _cleanly_.
const wasClean = this.#handler.closeState === sentCloseFrameState.SENT && this.#handler.receivedClose
const wasClean =
this.#handler.closeState.has(sentCloseFrameState.SENT) &&
this.#handler.closeState.has(sentCloseFrameState.RECEIVED)

let code = 1005
let reason = ''
Expand All @@ -685,7 +683,7 @@ class WebSocket extends EventTarget {
if (result && !result.error) {
code = result.code ?? 1005
reason = result.reason
} else if (!this.#handler.receivedClose) {
} else if (!this.#handler.closeState.has(sentCloseFrameState.RECEIVED)) {
// If _The WebSocket
// Connection is Closed_ and no Close control frame was received by the
// endpoint (such as could occur if the underlying transport connection
Expand Down
Loading