Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

socket.reconnectStrategy is not applied #1985

Closed
micodix opened this issue Feb 15, 2022 · 11 comments · Fixed by #2092
Closed

socket.reconnectStrategy is not applied #1985

micodix opened this issue Feb 15, 2022 · 11 comments · Fixed by #2092
Labels

Comments

@micodix
Copy link

micodix commented Feb 15, 2022

Hi,

I have a TypeScript application in which I am using the new socket.reconnectStrategy option described in https://github.com/redis/node-redis/blob/master/docs/client-configuration.md#reconnect-strategy.
However, this does not seem to be applied. After stopping the Redis server, the reconnecting event is still fired within the default delay of 500ms. But according to my configuration, this should happen after 3000ms.

My code:

(async () => {
  try {
    const client = createClient({ socket: { host: '127.0.0.1', port: 6379, connectTimeout: 3000, reconnectStrategy: (_retryAttempt: number): number | Error => 3000 } })
    client.on('connect', () => {
      console.info('Redis client is connecting to redis://127.0.0.1:6379...')
    })
    client.on('ready', () => {
      console.info('Redis client is connected')
    })
    client.on('reconnecting', () => {
      console.info('Redis client is reconnecting...')
    })
    client.on('error', error => {
      console.error(`Redis client error: ${JSON.stringify(error)}`)
    })

    await client.connect().catch(error => console.error(JSON.stringify(error)))
  } catch (error) {
    console.error(`Service start error: ${JSON.stringify(error)}`)
  }
})().catch(() => {})

Environment:

  • Node.js Version: 14.16.1
  • Redis Server Version: 6.2.0
  • Node Redis Version: 4.0.3
  • Platform: Windows 10
@micodix micodix added the Bug label Feb 15, 2022
@leibale
Copy link
Collaborator

leibale commented Feb 15, 2022

import { createClient } from '@node-redis/client';

console.time('reconnectStrategy');
(async () => {
    try {
        const client = createClient({
            socket: {
                host: '127.0.0.1',
                port: 6379,
                connectTimeout: 3000,
                reconnectStrategy() {
                    console.timeLog('reconnectStrategy', 'reconnectStrategy');
                    return 3000;
                }
            }
        }).on('connect', () => {
            console.timeLog('reconnectStrategy', 'connect');
        }).on('ready', () => {
            console.timeLog('reconnectStrategy', 'ready');
        }).on('reconnecting', () => {
            console.timeLog('reconnectStrategy', 'reconnecting');
        }).on('error', error => {
            console.timeLog('reconnectStrategy', 'error');
            console.error(`Redis client error: ${JSON.stringify(error)}`);
        });

        await client.connect();
    } catch (err) {
        console.error(`Service start error`, err);
    }
})();

I've just tested it locally, and the code above tries to reconnect to redis every 3 seconds.

@micodix
Copy link
Author

micodix commented Feb 15, 2022

Hi leibale,

thanks for trying out.
I've just c&p and run your code example, but still no luck. The following debug output runs quickly across the console:

...
reconnectStrategy: 29.181s error
Redis client error: {}
reconnectStrategy: 29.182s reconnecting
reconnectStrategy: 29.184s connect
reconnectStrategy: 29.185s ready
reconnectStrategy: 29.197s error
Redis client error: {}
reconnectStrategy: 29.198s reconnecting
reconnectStrategy: 29.200s connect
reconnectStrategy: 29.201s ready
reconnectStrategy: 29.213s error
Redis client error: {}
reconnectStrategy: 29.215s reconnecting
reconnectStrategy: 29.216s connect
reconnectStrategy: 29.217s ready
reconnectStrategy: 29.232s error
Redis client error: {}
reconnectStrategy: 29.233s reconnecting
...
...

Any idea?

@leibale
Copy link
Collaborator

leibale commented Feb 15, 2022

please run npm ls redis in the directory of the script and make sure the version is 4.0.3

@micodix
Copy link
Author

micodix commented Feb 15, 2022

Yes, it's 4.0.3

image

@leibale
Copy link
Collaborator

leibale commented Feb 16, 2022

@micodix "it works on my machine" :P
wanna debug it together?

@micodix
Copy link
Author

micodix commented Feb 16, 2022

Ok, I was able to find the reason for the different behavior. Basically, it's the type of connection error.

In my test setup, I connect from my local development machine to another server running the Redis server in a Docker container. If I stop the container, I get a socket closed unexpectedly and a read ECONNRESET as error messages and the configured reconnect wait time is not considered.
If I test an invalid host address, I provoke a connect ECONNREFUSED with which the own reconnect strategy works and the waiting time is applied.

@tugtugtug
Copy link

tugtugtug commented Mar 16, 2022

I have the same issue.
When the socket closes unexpectedly (which happens to be the case if the server no longer listens on the port),

this.#onSocketError(new SocketClosedUnexpectedlyError());
fires,
and socket.#connect gets called immediately,
this.#connect(true).catch(() => {
,
then socket.#retryConnection gets called with retries reset to 0,
this.#socket = await this.#retryConnection(0, hadError);
,
then socket.#createSocket gets called to repeat the error again.
This is running into a constant loop, and in my case starved my entire node, and I had to kill the node to get it out of the bad condition.
What bothers me most was that the server was definitely gone, and yet, the socket fired, 'connect' and 'ready' events, meaning the #createSocket seemed to have succeeded after the connectEvent fired.
In my case, both the redis client and the redis server were running as k8s services in the same namespace of a cluster.
Anyway, this is a showstopper for us to migrate to v4.

@nicktalb
Copy link

nicktalb commented Mar 30, 2022

We've been encountering similar problems and on investigation I have found that:

  1. ✅ If Redis is not running when the client service is started, the reconnectStrategy triggers and keeps trying to connect. If I then start the Redis service the client service connects successfully.
  2. ❌ If Redis goes down while the client service is already running and connected to Redis, it receives socket closed unexpectedly - I then quickly see the following sequence of events (and reconnectStrategy never triggers):
  • error - Socket closed unexpectedly
  • reconnecting
  • connect
  • error - Socket closed unexpectedly
  • reconnecting
  • end

If the client service tries to use the connection after this an error ClientClosedError: The client is closed is triggered. It never reconnects automatically. I assume this is not expected behaviour.

@leibale leibale linked a pull request Apr 25, 2022 that will close this issue
@leibale leibale closed this as completed Apr 25, 2022
@rpong
Copy link

rpong commented May 4, 2022

@leibale i just tested 4.1.0 and it seems this issue still persists, reconnecting still doesn't work like it used to like in v3. after "Socket closed unexpectedly", it just doesn't reconnect without restarting the client application. We simulate this by restarting redis (docker).

@leibale
Copy link
Collaborator

leibale commented May 4, 2022

@rpong this does not reproduce the issue (I ran docker restart a couple of times while this code is running)

import { createClient } from '@redis/client';

const client = createClient();

client.on('error', err => console.error('client err', err));

await client.connect();

setInterval(async () => {
  try {
    console.log(await client.ping());
  } catch (err) {
    console.error('command error', err);
  }
}, 1000);

@rpong
Copy link

rpong commented May 4, 2022

Thanks @leibale , it turns out we needed this before "await client.connect();"

client.on('error', err => console.error('client err', err));

All working now. Thank you, we can finally upgrade to v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants