fix: fire close on failed WebSocket connection (#3628)

* remove PROCESSING * make close better
nodejs · Sep 20, 2024 · 54fd2df · 54fd2df
1 parent 290e0e1
commit 54fd2df
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 42 deletions.
diff --git a/lib/web/websocket/constants.js b/lib/web/websocket/constants.js
@@ -21,9 +21,8 @@ const states = {
 }
 
 const sentCloseFrameState = {
-  NOT_SENT: 0,
-  PROCESSING: 1,
-  SENT: 2
+  SENT: 1,
+  RECEIVED: 2
 }
 
 const opcodes = {

diff --git 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
@@ -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
@@ -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,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_.
+      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) {
@@ -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).
@@ -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
@@ -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
@@ -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 = ''
@@ -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