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

fix(websocket): use binary operations instead of a Set for closeStatus #3635

Closed
wants to merge 3 commits into from
Closed

fix(websocket): use binary operations instead of a Set for closeStatus #3635

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fbb8f7e
fix(websocket): use binary operations instead of a Set for closeStatus
Uzlopak Sep 21, 2024
b3871cb
Apply suggestions from code review
Uzlopak Sep 21, 2024
4d974cd
rename NOT_SENT to NONE
Uzlopak Sep 26, 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: 3 additions & 2 deletions lib/web/websocket/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ const states = {
}

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

const opcodes = {
Expand Down
8 changes: 4 additions & 4 deletions lib/web/websocket/receiver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ class ByteParser extends Writable {

// 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 ((this.#handler.closeState & sentCloseFrameState.SENT) === 0) {
// 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 @@ -369,14 +369,14 @@ class ByteParser extends Writable {
const closeFrame = new WebsocketFrameSend(body)

this.#handler.socket.write(closeFrame.createFrame(opcodes.CLOSE))
this.#handler.closeState.add(sentCloseFrameState.SENT)
this.#handler.closeState |= 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.closeState.add(sentCloseFrameState.RECEIVED)
this.#handler.closeState |= sentCloseFrameState.RECEIVED

return false
} else if (opcode === opcodes.PING) {
Expand All @@ -385,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.closeState.has(sentCloseFrameState.RECEIVED)) {
if ((this.#handler.closeState & sentCloseFrameState.RECEIVED) === 0) {
const frame = new WebsocketFrameSend(body)

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

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

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

#url
Expand Down Expand Up @@ -592,7 +592,7 @@ 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.has(sentCloseFrameState.SENT) && !this.#handler.closeState.has(sentCloseFrameState.RECEIVED)) {
} else if (this.#handler.closeState === sentCloseFrameState.NOT_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.
Expand Down Expand Up @@ -630,7 +630,7 @@ class WebSocket extends EventTarget {

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

this.#handler.closeState.add(sentCloseFrameState.SENT)
this.#handler.closeState |= 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 @@ -671,9 +671,7 @@ 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.has(sentCloseFrameState.SENT) &&
this.#handler.closeState.has(sentCloseFrameState.RECEIVED)
const wasClean = this.#handler.closeState === (sentCloseFrameState.SENT & sentCloseFrameState.RECEIVED)
Uzlopak marked this conversation as resolved.
Show resolved Hide resolved

let code = 1005
let reason = ''
Expand All @@ -683,7 +681,7 @@ class WebSocket extends EventTarget {
if (result && !result.error) {
code = result.code ?? 1005
reason = result.reason
} else if (!this.#handler.closeState.has(sentCloseFrameState.RECEIVED)) {
} else if ((this.#handler.closeState & sentCloseFrameState.RECEIVED) === 0) {
// 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