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

fix: fire close on failed WebSocket connection #3628

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

KhafraDev
Copy link
Member

probably relies on #3627 landing first

the way we previously handled closing made no sense; either side could send/receive a close frame, but the closing handshake is only completed _cleanly_ when the said side sends & receives a close frame. We were only tracking a single state, and then we had a receivedClose boolean property... which is very confusing

now we know if a closing handshake

  • has started (sent OR received close frame)
  • has completed (sent AND received close frame)
  • was closed cleanly (same logic as the bullet point above)

I have no idea if this will pass the autobahn tests, but it's too late for me to be running them (they take forever)

// 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

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Uzlopak Uzlopak changed the title Good close fix: fire close on failed WebSocket connection Sep 20, 2024
Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

How about avoiding a Set? Not tested

diff --git a/lib/web/websocket/constants.js b/lib/web/websocket/constants.js
index 76ed34a6..45484042 100644
--- a/lib/web/websocket/constants.js
+++ b/lib/web/websocket/constants.js
@@ -21,8 +21,9 @@ const states = {
 }
 
 const sentCloseFrameState = {
-  SENT: 1,
-  RECEIVED: 2
+  NOT_SENT: 0,
+  SENT: 1 << 0,
+  RECEIVED: 1 << 1
 }
 
 const opcodes = {
diff --git a/lib/web/websocket/receiver.js b/lib/web/websocket/receiver.js
index 69cfa1b3..9ab9c056 100644
--- a/lib/web/websocket/receiver.js
+++ b/lib/web/websocket/receiver.js
@@ -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) {
         // 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
@@ -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) {
@@ -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) {
         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 6d68bda4..637bb198 100644
--- a/lib/web/websocket/websocket.js
+++ b/lib/web/websocket/websocket.js
@@ -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
@@ -84,7 +84,7 @@ class WebSocket extends EventTarget {
 
     readyState: states.CONNECTING,
     socket: null,
-    closeState: new Set()
+    closeState: sentCloseFrameState.NOT_SENT
   }
 
   #url
@@ -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.
@@ -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
@@ -672,8 +672,8 @@ class WebSocket extends EventTarget {
     // 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)
+      this.#handler.closeState & sentCloseFrameState.SENT &&
+      this.#handler.closeState & sentCloseFrameState.RECEIVED
 
     let code = 1005
     let reason = ''
@@ -683,7 +683,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) {
       // 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

@KhafraDev
Copy link
Member Author

It's possible to avoid the set but nothing here is a hot path - websockets are not closed often.

@KhafraDev KhafraDev merged commit 54fd2df into nodejs:main Sep 20, 2024
33 checks passed
@KhafraDev KhafraDev deleted the good-close branch September 20, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants