Bug: `_disconnect` causes uncaught exception and process to crash if using `ws` library

[titanism]

There is a bug in the codebase right now in that _disconnect is invoked which subsequently calls this._ws.close. Despite there being a try/catch wrapper, if you use the ws library, an exception is thrown and your process will exit.

ReconnectingWebSocket.prototype._disconnect = function (code, reason) {
  if (code === undefined) {
    code = 1000;
  }

  this._clearTimeouts();
  if (!this._ws) {
    return;
  }

  this._removeListeners();
  try {
+    if (this._ws.readyState === ReconnectingWebSocket.OPEN) {
+      this._ws.close(code, reason);
+    }
-    this._ws.close(code, reason);
    this._handleClose(new CloseEvent(code, reason, this));
  } catch (err) {
    console.log('das error', err);
  }
};

cc @bytemain @abdelmagied94 @Zerounary can you please merge this fix into your library at https://github.com/opensumi/reconnecting-websocket for the npm package @opensumi/reconnecting-websocket and patch version bump and publish to npm afterwards to v4.4.1 (it currently on v4.4.0)

[titanism]

You can see the code here from ws that causes this uncaught exception:

  close(code, data) {
    if (this.readyState === WebSocket.CLOSED) return;
    if (this.readyState === WebSocket.CONNECTING) {
      const msg = 'WebSocket was closed before the connection was established';
      abortHandshake(this, this._req, msg); // <----------- RIGHT HERE EXCEPTION IS THROWN
      return;
    }

https://github.com/websockets/ws/blob/0d1b5e6c4acad16a6b1a1904426eb266a5ba2f72/lib/websocket.js#L290-L294

abortHandshake will throw the error on the process.nextTick here https://github.com/websockets/ws/blob/0d1b5e6c4acad16a6b1a1904426eb266a5ba2f72/lib/websocket.js#L1096

[bytemain]

How do you call _disconnect? this error seems to happen when you manually invoke reconnect method? could you check the readyState before invoke?

[titanism]

@bytemain This gets invoked when you call wsp.close() or when a timeout occurs, e.g. _handleError is called which calls this._disconnect, which then triggers this.

[bytemain]

I see it, seems a bug here, might we should check this._ws.readyState === this.CLOSING first?

[titanism]

Actually what is probably better is to check if readyState !== CONNECTING

[bytemain]

but when ws abort connection, it set readyState to CLOSING first: https://github.com/websockets/ws/blob/0d1b5e6c4acad16a6b1a1904426eb266a5ba2f72/lib/websocket.js#L1077

[titanism]

The logic that throws this error results from checking if it is equal to CONNECTING as mentioned already.

Please see the code here https://github.com/websockets/ws/blob/0d1b5e6c4acad16a6b1a1904426eb266a5ba2f72/lib/websocket.js#L290-L294.

Therefore, it is is NOT connecting, !== CONNECTING then don't call close.

@bytemain

[bytemain]

why? we actually need abort connection that is connecting, because here we are reconnecting, if dont abort it, we will have orphen conection

[titanism]

It seems to throw an uncaught exception if you try to call close in an instance of ws that is in a CONNECTING state (even if you wrap it with a try/catch block).

[titanism]

@bytemain just making sure you saw my comment here https://github.com/opensumi/reconnecting-websocket/pull/10#discussion_r1656500138

Thank you! 🙏 🙇 😄