From dbedf6773005459966e50f2ef0045f9364bac9b9 Mon Sep 17 00:00:00 2001 From: Khafra Date: Fri, 20 Sep 2024 00:46:44 -0400 Subject: [PATCH 1/2] remove PROCESSING --- lib/web/websocket/constants.js | 3 +-- lib/web/websocket/websocket.js | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/web/websocket/constants.js b/lib/web/websocket/constants.js index 66f27245246..eb87175a472 100644 --- a/lib/web/websocket/constants.js +++ b/lib/web/websocket/constants.js @@ -22,8 +22,7 @@ const states = { const sentCloseFrameState = { NOT_SENT: 0, - PROCESSING: 1, - SENT: 2 + SENT: 1 } const opcodes = { diff --git a/lib/web/websocket/websocket.js b/lib/web/websocket/websocket.js index 7e465f56e99..75ed0d1240a 100644 --- a/lib/web/websocket/websocket.js +++ b/lib/web/websocket/websocket.js @@ -601,8 +601,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 From df0dd7590f282b692e50b8cddc41234ab1ce1284 Mon Sep 17 00:00:00 2001 From: Khafra Date: Fri, 20 Sep 2024 01:17:38 -0400 Subject: [PATCH 2/2] make close better --- lib/web/websocket/constants.js | 4 ++-- lib/web/websocket/receiver.js | 18 ++++++-------- lib/web/websocket/websocket.js | 44 ++++++++++++++++++++-------------- test/websocket/issue-3546.js | 2 +- 4 files changed, 36 insertions(+), 32 deletions(-) diff --git a/lib/web/websocket/constants.js b/lib/web/websocket/constants.js index eb87175a472..76ed34a650e 100644 --- a/lib/web/websocket/constants.js +++ b/lib/web/websocket/constants.js @@ -21,8 +21,8 @@ const states = { } const sentCloseFrameState = { - NOT_SENT: 0, - SENT: 1 + SENT: 1, + RECEIVED: 2 } const opcodes = { diff --git a/lib/web/websocket/receiver.js b/lib/web/websocket/receiver.js index 5364de0784a..69cfa1b3da5 100644 --- a/lib/web/websocket/receiver.js +++ b/lib/web/websocket/receiver.js @@ -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 @@ -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) { @@ -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)) diff --git a/lib/web/websocket/websocket.js b/lib/web/websocket/websocket.js index 75ed0d1240a..6d68bda43b3 100644 --- a/lib/web/websocket/websocket.js +++ b/lib/web/websocket/websocket.js @@ -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} closeState */ // https://websockets.spec.whatwg.org/#interface-definition @@ -85,8 +84,7 @@ class WebSocket extends EventTarget { readyState: states.CONNECTING, socket: null, - closeState: sentCloseFrameState.NOT_SENT, - receivedClose: false + closeState: new Set() } #url @@ -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. @@ -516,12 +512,26 @@ 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_. + fireEvent('close', this, (type, init) => new CloseEvent(type, init), { + wasClean: false, code, reason + }) } this.#controller.abort() @@ -529,14 +539,6 @@ class WebSocket extends EventTarget { 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 - }) - } } #onMessage (type, data) { @@ -590,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). @@ -624,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 @@ -665,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 = '' @@ -675,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 diff --git a/test/websocket/issue-3546.js b/test/websocket/issue-3546.js index 5d328902538..59a7bd0f093 100644 --- a/test/websocket/issue-3546.js +++ b/test/websocket/issue-3546.js @@ -5,7 +5,7 @@ const { WebSocket } = require('../..') 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 }) + const { completed, strictEqual } = tspl(t, { plan: 4 }) const ws = new WebSocket('ws://localhost:1') let orderOfEvents = 0