h0x91b / redis-fast-driver

78 stars 13 forks source link

Redis Disconnect triggers error and disconnect event #31

Closed FanamyWirato closed 2 years ago

FanamyWirato commented 4 years ago

We're using the driver for pub/sub currently. After a given amount of time with no data beeing published, the subscriber receives a redis side disconnect. Error: Server closed the connection at Redis._onDisconnect (/var/www/html/node_modules/redis-fast-driver/index. js:141:26) Note: import Redis from 'redis-fast-driver'; Config:

const config = {
                host: process.env.REDIS_HOST, //Can be IP or hostname
                port: process.env.REDIS_PORT,
                maxRetries: 10, //Reconnect retries, default -1 (infinity)
                db: 0, //Optional db selection
                autoConnect: true, //Will connect after creation
                //Will set connection name (you can see connections by running CLIENT LIST on redis server)
                doNotSetClientName: false,
                //When you call `end()`, driver tries to send `QUIT` command to redis before actual end
                doNotRunQuitOnEnd: false,
            };

Subscribe function:

ConnectionContainer.redis.subscribe = function (callback) {
                    ConnectionContainer.redis.rawCall(['SUBSCRIBE', process.env.REDIS_CHANNEL], (e, data) => {
                        if (e) {
                            Logger.error(`Subscriber Error: ${e}`);

                            return;
                        }

                        /**
                         * A message is a Array reply with three elements.
                         * The first element is the kind of message:
                         *  subscribe: means that we successfully subscribed to the channel given
                         *      as the second element in the reply
                         *
                         * @see https://redis.io/topics/pubsub
                         */
                        if (data[0] === 'subscribe') {
                            Logger.info('Successfully subscribed');
                        } else {
                            if (data[2]) {
                                callback(JSON.parse(data[2]));
                            }
                        }
                    });
                };

Basically the disconnect is a wanted behavior and no bug in Reconnect tries from driver side in my eyes - but when receiving this disconnect the error event is triggered too and before the disconnect.

In my opinion, only a disconnect should be emitted or the callback of the error should be an object (string parsing isn't that much fun there).

redis-fast-driver Version: 2.1.5 Redis Version 3.2.11 NodeJS Version: 11.15.0 using experimental modules Using Docker for Windows with Redis and NodeJs Container

h0x91b commented 4 years ago

Error is Server closed the connection e.g. Connection is closed by the server, so the disconnect is an error actually...

I can move the trigger of error below trigger of disconnect this will help you? Because replacing the error to an object isn't a good idea since you will lose call stack description...

https://github.com/h0x91b/redis-fast-driver/blob/master/index.js#L140-L142

FanamyWirato commented 4 years ago

Makes sense with the raised error.

The trigger move could actually help - in my case I can handle the error, and it's more a nice to have.

Because replacing the error to an object isn't a good idea since you will lose call stack description...

With "error should be an object" i meant advancing. Smth. like Error Code, Error Message and Call Stack. But as far as i know redis is only sending strings so I guess there is a lot of implementation work then and also only an Improvemente/Nice to Have.

Thanks a lot for the fast response!

h0x91b commented 2 years ago

Published v2.1.7