Feathers sync does not respect the built-in reconnection strategy in node-redis

[tobiasheldring]

Hello! We have a problem with the sub redis client within feathers-sync not waiting for reconnection attempts to redis server on connection issues such as timeouts.

Steps to reproduce

Try the following configuration of feathers-sync with the redis server offline/non-existent (to force connection issues)

  const redisClient = createClient({
    url: "redis://" + redisConfig.host,
    socket: {
      reconnectStrategy: (retries: number) => {
        if (retries <= 2) {
          console.log(`Trying to reconnect, attempt #${retries + 1}`)
          return 3000
        }
        console.log("Will not attempt further reconnections, aborting")
        return false
      },
    },
  })
  redisClient.on("error", (error) => console.error("Redis client error", error))

  app.configure(
    redis({
      redisClient,
    }),
  )
  ;(app as any).sync.ready
    .then(() => {
      console.info("Feathers sync is ready")
    })
    .catch((e: unknown) =>
      console.error("Feathers sync failed to become ready", e),
    )

Expected behavior

Expectation is that any reconnection strategy set in the redis client is respected when initialising feathers-sync, and that all errors and promise rejections can be handled by the user.

Actual behavior

feathers-sync will only subscribe to redis pubsub if the initial connection attempt succeeds, even if the redis client will continue trying to reconnect more times than the initial one.
Also, the client passed into feathers-sync (pub) is duplicated (sub) and since event handlers are lost during duplication the duplicated client will throw unhandled promise rejection on the first reconnection attempt rather than trying to reconnect according to the set reconnectionStrategy. See output below for the code example above:

Trying to reconnect, attempt #1
Redis client error {
  error: Error: connect ECONNREFUSED 127.0.0.1:6379
      at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1247:16) {
    errno: -61,
    code: 'ECONNREFUSED',
    syscall: 'connect',
    address: '127.0.0.1',
    port: 6379
  }
}
Trying to reconnect, attempt #1
Feathers sync failed to become ready {
  error: Error: connect ECONNREFUSED 127.0.0.1:6379
      at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1247:16) {
    errno: -61,
    code: 'ECONNREFUSED',
    syscall: 'connect',
    address: '127.0.0.1',
    port: 6379
  }
}
Trying to reconnect, attempt #2
Redis client error {
  error: Error: connect ECONNREFUSED 127.0.0.1:6379
      at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1247:16) {
    errno: -61,
    code: 'ECONNREFUSED',
    syscall: 'connect',
    address: '127.0.0.1',
    port: 6379
  }
}
Trying to reconnect, attempt #2
Unhandled promise rejection {
  error: Error: connect ECONNREFUSED 127.0.0.1:6379
      at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1247:16) {
    errno: -61,
    code: 'ECONNREFUSED',
    syscall: 'connect',
    address: '127.0.0.1',
    port: 6379
  }
}
Trying to reconnect, attempt #3
Redis client error {
  error: Error: connect ECONNREFUSED 127.0.0.1:6379
      at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1247:16) {
    errno: -61,
    code: 'ECONNREFUSED',
    syscall: 'connect',
    address: '127.0.0.1',
    port: 6379
  }
}
Will not attempt further reconnections, aborting
Redis client error {
  error: Error: connect ECONNREFUSED 127.0.0.1:6379
      at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1247:16) {
    errno: -61,
    code: 'ECONNREFUSED',
    syscall: 'connect',
    address: '127.0.0.1',
    port: 6379
  }
}
Unhandled promise rejection {
  error: Error: connect ECONNREFUSED 127.0.0.1:6379
      at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1247:16) {
    errno: -61,
    code: 'ECONNREFUSED',
    syscall: 'connect',
    address: '127.0.0.1',
    port: 6379
  }
}

What is happening in redis.js:
On connect, pub will post error events to the handler provided by the user outside of feathers-sync, and continue to try to reconnect until reconnectionStrategy returns false. When that happens it will also reject the promise returned by connect() resulting in the unhandled promise rejection seen last in the output above.

For sub, the first failed connection attempt will be caught by the once('error', .. handler and the ready promise will be rejected. The sub redis client will however continue to try to reconnect. If the next attempt succeeds it does not matter since the ready promise has already been rejected and the subscription will not be initiated. If the next attempt fails it will result in an unhandled promise rejection that kills any further reconnection attempts for the sub client.

System configuration

feathers-sync 3.0.3
redis 4.6.7

Suggestion

feathers-sync should wait for reconnection attempts before rejecting ready. Also, since a redis client without error handler is strongly discouraged feathers-sync should make sure there is always one in place so that any reconnection strategies can proceed as expected.

Potentially redis.js could be updated to something like below

    const pub = redisClient || redis.createClient(options);
    const sub = pub.duplicate();

    errorHandlers = pub.listeners("error");
    if (errorHandlers.length > 0) {
      // If error handlers exists, copy them to sub
      errorHandlers.forEach(function (handler) {
        sub.on("error", handler);
      });
    } else {
      // If not, make sure both pub and sub has an error handler to avoid unhandled rejections
      const defaultErrorHandler = (err) => {
        debug("Redis error", err);
      };
      pub.on("error", defaultErrorHandler);
      sub.on("error", defaultErrorHandler);
    }

    const msgFromRedisHandler = (data) => {
      debug(`Got ${key} message from Redis`);
      app.emit("sync-in", data);
    };

    app.configure(core);
    app.sync = {
      deserialize,
      serialize,
      pub,
      sub,
      type: "redis",
      ready: new Promise((resolve, reject) => {
        pub.connect() // Potentially catch this one as well and log with debug, to avoid unhandled rejection
        // Move the ready-rejection to here, since this will wait for any reconnection attempts before throwing
        sub.connect().catch(reject);
        sub.once("ready", resolve);
      }).then(() => sub.subscribe(key, msgFromRedisHandler, true)),
    };