socketio / socket.io-postgres-adapter

The Socket.IO Postgres adapter, allowing to broadcast events between several Socket.IO servers
https://socket.io
MIT License
24 stars 8 forks source link

Add errorHandler option #5

Closed sjoerdsmink closed 2 years ago

sjoerdsmink commented 2 years ago

It might happen that:

There is code to reconnect, but that's not executed as an uncaughtException is thrown. This is because this.emit("error", err); will cause the Node process to exit, as explained on https://nodejs.org/api/events.html#error-events

It would be better to offer the option to handle these errors.

Alternative would be to mention in the readme that errors can be handled when creating the adapter:

io.adapter(function (nsp) {
  const adapter = new PostgresAdapter(nsp, pool, {})
  adapter.on("error", (e) => {
    console.error("Postgres adapter error", e)
  })
  return adapter
})

The arrow notation (so io.adapter((nsp) => { instead of io.adapter(function (nsp) {) won't work in this case.

darrachequesne commented 2 years ago

Actually, I think we should rather remove the "error" events, as the end user has no constructive way to handle them.

See https://github.com/socketio/socket.io-postgres-adapter/pull/7. What do you think?

sjoerdsmink commented 2 years ago

I rewrote the code a bit to use an errorHandler inside the adapter. This way the type definitions of PostgresAdapterOptions are also correct. A user could directly create PostgresAdapter using the ops of PostgresAdapterOptions, in which case errorHandler should be implemented.

I think the benefit of using an errorHandler instead of removing all error events in #7 is that I can now log the errors. That is really useful for debugging, but also when going through server logging. I can understand your reasoning of not letting the server crash, but how would you feel about using the defaultErrorHandler to debug the message? So the user can still override this behaviour. So: const defaultErrorHandler = (err: any) => debug(err);

darrachequesne commented 2 years ago

Merged as https://github.com/socketio/socket.io-postgres-adapter/commit/ec1b78cf132147960f05402f6ae9b75ec77e1dd6. Thanks!