ShareDB/ws occasionally crashes Node when "readyState 2 (CLOSING)"

[houshuang]

We expose the ShareDB server within the same Node process that runs our Meteor app (separate websockets from Meteor). It's quite important for us that this main Node process should never crash, since that would negatively impact all connected clients (even if the restart time is quick). Therefore we use try...catch for things that could possibly fail, and deal with them internally.

However, I see a few crashes in the log due to ShareDB's use of Websocket (we use ws on the backend, and reconnecting-websocket on the frontend). I see that ShareDB does check whether the websocket is closed in this snippet

(lib/agent.js:165)

   if (this.closed) return;

  this.backend.emit('send', this, message);
  this.stream.write(message);

But that seems to not be enough, maybe the websocket changes status between line 1 and line 3 above, or maybe this.closed doesn't capture readyState 2?

It seems that attaching a callback function on this.stream.write function also avoids throwing, but I'm not sure if it's safe for ShareDB to just disregard (which it seems to be, since it justs returns if this.closed)?

Error: WebSocket is not open: readyState 2 (CLOSING)
    at WebSocket.send (/built_app/programs/server/npm/node_modules/ws/lib/websocket.js:322:19)
    at WebSocketJSONStream._write (/built_app/programs/server/npm/node_modules/websocket-json-stream/index.js:27:11)
    at doWrite (_stream_writable.js:397:12)
    at writeOrBuffer (_stream_writable.js:383:5)
    at WebSocketJSONStream.Writable.write (_stream_writable.js:290:11)
    at Agent.send (/built_app/programs/server/npm/node_modules/sharedb/lib/agent.js:165:15)
    at Agent._reply (/built_app/programs/server/npm/node_modules/sharedb/lib/agent.js:220:8)
    at callback (/built_app/programs/server/npm/node_modules/sharedb/lib/agent.js:241:15)
    at /built_app/programs/server/npm/node_modules/sharedb/lib/agent.js:497:7
    at /built_app/programs/server/npm/node_modules/sharedb/lib/backend.js:433:9
    at /built_app/programs/server/npm/node_modules/sharedb-mongo/index.js:368:16
    at handleCallback (/built_app/programs/server/npm/node_modules/mongodb/lib/utils.js:120:56)
    at /built_app/programs/server/npm/node_modules/mongodb/lib/cursor.js:683:5
    at handleCallback (/built_app/programs/server/npm/node_modules/mongodb-core/lib/cursor.js:171:5)
    at nextFunction (/built_app/programs/server/npm/node_modules/mongodb-core/lib/cursor.js:691:5)
    at /built_app/programs/server/npm/node_modules/mongodb-core/lib/cursor.js:602:7
    at queryCallback (/built_app/programs/server/npm/node_modules/mongodb-core/lib/cursor.js:232:18)
    at /built_app/programs/server/npm/node_modules/mongodb-core/lib/connection/pool.js:469:18
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)

Versions:
ws 6.1.3
sharedb 1.0.0-beta.18

[gkubisa]

There seems to be a race condition between the stream and the socket closing, when the connection is terminated by the client. I'm not sure, if it can be fixed, however, the error can definitely be handled. To do that, switch to @teamwork/websocket-json-stream, which is a drop-in replacement for websocket-json-stream but reports errors correctly. Then you'd just need to listen for the error events on the stream and ignore those related to writing to a closed or closing socket. We've been using this approach in production for months without problems.

@ericyhwang Should README.md be updated to recommend @teamwork/websocket-json-stream and advice about handling the above mentioned errors?

[houshuang]

This is brilliant. Just to clarify, when you say "listen for error events", what does that look like in practice?

…

On Mon, Mar 4, 2019 at 7:34 AM Greg Kubisa ***@***.***> wrote: There seems to be a race condition between the stream and the socket closing, when the connection is terminated by the client. I'm not sure, if it can be fixed, however, the error can definitely be handled. To do that, switch to @teamwork/websocket-json-stream ***@***.***/websocket-json-stream>, which is a drop-in replacement for websocket-json-stream <https://www.npmjs.com/package/websocket-json-stream> but reports errors correctly. Then you'd just need to listen for the error events on the stream and ignore those related to writing to a closed or closing socket. We've been using this approach in production for months without problems. @ericyhwang <https://github.com/ericyhwang> Should README.md be updated to recommend @teamwork/websocket-json-stream and advice about handling the above mentioned errors? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADwh5dFC8O5Xvl5NFM0hfqaiaSDdjAkks5vTL53gaJpZM4bbQRw> .

-- http://reganmian.net/blog -- Random Stuff that Matters

[alecgibson]

Yeah, we wound up also rewriting our own version of websocket-json-stream with much better error handling. It's not a particularly big library to rewrite. Something like this:

export default class WebSocketJsonStream extends Duplex {
  private readonly webSocket: WebSocket;

  public constructor(webSocket: WebSocket) {
    super({ objectMode: true });
    this.webSocket = webSocket;
    this.registerWebSocketListeners();
    this.registerStreamListeners();
  }

  public _read() {}

  public _write(message: any, encoding: any, callback: (error: Error) => void) {
    this.webSocket.send(JSON.stringify(message), callback);
  }

  private registerWebSocketListeners() {
    this.webSocket.on('message', (message: string) => {
      this.receiveMessage(message);
    });

    this.webSocket.on('close', () => {
      this.closeStreams();
    });
  }

  private registerStreamListeners() {
    this.on('error', (error: any) => {
      logger.debug('Websocket JSON stream error', { error });
      this.closeSocket();
    });

    this.on('end', () => {
      this.closeSocket();
    });
  }

  private receiveMessage(message: string) {
    this.push(JSON.parse(message));
  }

  private closeStreams() {
    this.push(null);
    this.end();

    this.emit('close');
    this.emit('end');
  }

  private closeSocket() {
    this.webSocket.close();
  }
}

[gkubisa]

This is brilliant. Just to clarify, when you say "listen for error events", what does that look like in practice?

stream.on('error', error => {
    if (error.message.startsWith('WebSocket is not open')) {
        // No point reporting this error, as it happens often and is harmless.
        return
    }
    // Log the error, etc.
})

[houshuang]

Just to be 100% sure, does this look good to you?

[gkubisa]

Yes, that should stop the server crashes related to stream errors.

[ericyhwang]

@gkubisa

Should README.md be updated to recommend @teamwork/websocket-json-stream and advice about handling the above mentioned errors?

Some time last year, I believe @nateps mentioned he wanted to switch all of Share's examples over to use the Teamwork version of websocket-json-stream? Did he talk to you about that? I'm not sure how far he got.

I can check with him on that, but in the meantime, if you make a PR for the README change, I can get that merged in, especially since you've been using it in production for a while now. (Nate and I are using the browserchannel adapter in production, so for the websocket adapter, it makes sense to leverage your production experience there.)

[gkubisa]

PR for the README change: #276.

I've just improved @teamwork/websocket-json-stream a bit, so that when trying to write to a closing or closed socket, it will emit an error with the name property value equal to Error [ERR_CLOSED], so that it won't be necessary to rely on the message. cc @houshuang

Some time last year, I believe @nateps mentioned he wanted to switch all of Share's examples over to use the Teamwork version of websocket-json-stream? Did he talk to you about that? I'm not sure how far he got.

Yes, he mentioned that last year but I haven't heard anything more about it since.

I can check with him on that, but in the meantime, if you make a PR for the README change, I can get that merged in, especially since you've been using it in production for a while now. (Nate and I are using the browserchannel adapter in production, so for the websocket adapter, it makes sense to leverage your production experience there.)

Well, we've recently switched over to SockJS to be able to work over corporate proxies, however, @teamwork/websocket-json-stream worked well for us for a few months.

[curran]

@gkubisa I'm evaluating SockJS and I'm curious, how does it look like to use SockJS with ShareDB? Are there any examples of how to set up the backend connection?

[gkubisa]

Hi @curran ,

on the server-side just wrap SockJS in a stream and then pass it to the listen method. Here's an example wrapper:

const { Duplex } = require('stream')

module.exports = class SockJsJsonStream extends Duplex {
    constructor(sockJs) {
        super({
            objectMode: true
        })
        this._sockJs = sockJs

        this._sockJs.on('data', data => {
            let parsedData
            try {
                parsedData = JSON.parse(data)
            } catch (error) {
                this.emit('error', error)
                return
            }
            this.push(parsedData)
        })

        this._sockJs.on('close', () => {
            this.push(null)
        })

        this._sockJs.on('error', error => {
            this.emit('error', error)
        })
    }

    _read(size) {
        // The SockJS API is very simple, so all we can do is to push the data through
        // as it comes in and that's implemented in the constructor.
    }

    _write(data, encoding, callback) {
        try {
            this._sockJs.write(JSON.stringify(data))
        } catch (error) {
            callback(error)
            return
        }
        callback(null)
    }

    _final(callback) {
        this._sockJs.end()
        callback(null)
    }

    _destroy(error, callback) {
        this._sockJs.close()
        callback(error)
    }
}

On the client-side SockJS exposes a WebSocket interface, so you can simply use it instead of the native WebSocket.

[curran]

Thank you so much for sharing that snippet! It may prove useful one day.

[gkubisa]

No problem, it can certainly be improved but it's a good start.

[curran]

FWIW I'm working with Webpack Dev Server, and I'm frequently encountering the error with the following message:

Error [ERR_CLOSED]: WebSocket CLOSING or CLOSED.

The suggested workaround helped a lot. Here's what I'm doing, which appears to replace crashes with a benign console.log:

new WebSocket.Server({server}).on('connection', ws => {
  const stream = new JSONStream(ws);

  // Prevent server crashes on errors.
  stream.on('error', error => {
    console.log(error.message); // Tends to print "WebSocket CLOSING or CLOSED."
  });

  share.listen(stream);
});