You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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)
constredisClient=createClient({url: "redis://"+redisConfig.host,socket: {reconnectStrategy: (retries: number)=>{if(retries<=2){console.log(`Trying to reconnect, attempt #${retries+1}`)return3000}console.log("Will not attempt further reconnections, aborting")returnfalse},},})redisClient.on("error",(error)=>console.error("Redis client error",error))app.configure(redis({
redisClient,}),);(appasany).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:
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
constpub=redisClient||redis.createClient(options);constsub=pub.duplicate();errorHandlers=pub.listeners("error");if(errorHandlers.length>0){// If error handlers exists, copy them to suberrorHandlers.forEach(function(handler){sub.on("error",handler);});}else{// If not, make sure both pub and sub has an error handler to avoid unhandled rejectionsconstdefaultErrorHandler=(err)=>{debug("Redis error",err);};pub.on("error",defaultErrorHandler);sub.on("error",defaultErrorHandler);}constmsgFromRedisHandler=(data)=>{debug(`Got ${key} message from Redis`);app.emit("sync-in",data);};app.configure(core);app.sync={
deserialize,
serialize,
pub,
sub,type: "redis",ready: newPromise((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 throwingsub.connect().catch(reject);sub.once("ready",resolve);}).then(()=>sub.subscribe(key,msgFromRedisHandler,true)),};
The text was updated successfully, but these errors were encountered:
This is happening for me. If the redis connection drops for any reason, my app will crash.
Tobias' suggestion prevents the crash for me. Like they say, it's because the sub client doesn't have an error handler after being duplicated from the pub client.
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)
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:What is happening in redis.js:
On
connect
,pub
will posterror
events to the handler provided by the user outside of feathers-sync, and continue to try to reconnect until reconnectionStrategy returnsfalse
. When that happens it will also reject the promise returned byconnect()
resulting in the unhandled promise rejection seen last in the output above.For
sub
, the first failed connection attempt will be caught by theonce('error', ..
handler and theready
promise will be rejected. Thesub
redis client will however continue to try to reconnect. If the next attempt succeeds it does not matter since theready
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 thesub
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
The text was updated successfully, but these errors were encountered: