Calls to disconnect doesn't stop reconnect

[pixtron]

If the connection to the redis server can't be established, a call to client.disconnect() won't stop the client from trying to reconnect.

Steps to reproduce

1. run the test script below, make sure there is nothing listening on port 65535 so the connection attempt errors with ECONNREFUSED
2. send SIGTERM or SIGINT to the process (eg. Ctrl + C)

Expected Result
Client stops trying to reconnect and process ends.

2022-02-26T14:24:09.754Z client error connect ECONNREFUSED 127.0.0.1:65535
reconnecting
2022-02-26T14:24:09.909Z client error connect ECONNREFUSED 127.0.0.1:65535
^C
connect error connect ECONNREFUSED 127.0.0.1:65535
command error The client is closed

Actual Result
The client keeps reconnecting, the process keeps running until a second SIGTERM or SIGINT is sent (script only listens once to the event)

2022-02-26T14:24:09.754Z client error connect ECONNREFUSED 127.0.0.1:65535
reconnecting
2022-02-26T14:24:09.909Z client error connect ECONNREFUSED 127.0.0.1:65535
^C
disconnect error The client is closed
reconnecting
2022-02-26T14:24:10.113Z client error connect ECONNREFUSED 127.0.0.1:65535
reconnecting
2022-02-26T14:24:10.365Z client error connect ECONNREFUSED 127.0.0.1:65535
reconnecting
^C

Environment:

• Node.js Version: 16.13
• Redis Server Version: 6.2.6
• Node Redis Version: 4.0.4
• Platform: Ubuntu 20.04.4

const { createClient } = require('redis');

let client;

const onError = (err) => {
	console.log(new Date(), 'onError', err.message);
}

const onReconnecting = () => {
	console.log(new Date(), 'onReconnecting');
}

const handleSignal =  async () => {
	if (client) try {
		await client.disconnect();
	} catch (err) {
		console.log('disconnect error', err.message);
	}
}

(async () => {
	process.once('SIGINT', handleSignal);
	process.once('SIGTERM', handleSignal);

	client = createClient({
		socket: {
			port: 65535
		}
	});

	client.on('connect', () => console.log('connect'));
	client.on('error', err => console.log(new Date(), 'client error', err.message));
	client.on('reconnecting', () => console.log('reconnecting'));

	try {
		await client.connect();
	} catch(err) {
		console.log('connect error', err.message);
	}

	try {
		await client.incr('mykey');
		console.log(await client.get('mykey'));
	} catch(err) {
		console.log('command error', err.message);
	}
})();

[Didas-git]

can you try to set keepAlive to false?

createClient({
        socket: {
            port: 65535,
            keepAlive: false
        }
    })

[pixtron]

Setting keepAlive to false, does not fix the issue. After some more debuging i figured out, that during the above scenario this.#socket is undefined when socket.disconnect. Therefore the ClientClosedError is thrown. At the same time this.#isOpen is true. As the error is thrown without setting this.#isOpen to false, socket.#retryConnection is retrying to establish a connection indefinitely.

b5360c0 would fix the issue. Not sure how to write a test for that though.

[Didas-git]

Setting keepAlive to false, does not fix the issue. After some more debuging i figured out, that during the above scenario this.#socket is undefined when socket.disconnect. Therefore the ClientClosedError is thrown. At the same time this.#isOpen is true. As the error is thrown without setting this.#isOpen to false, socket.#retryConnection is retrying to establish a connection indefinitely.

b5360c0 would fix the issue. Not sure how to write a test for that though.

You can try to run redis own tests and try it.
But @leibale will analyze this when he has time

[tugtugtug]

I have to say the v4 quality has been a disappointment, especially around the error handling of socket.
I've seen a lot of issues regarding reconnection, retryStrategy, and socket stuck in a loop of retries, or uncaughtException due to the isolated client pool.
In the end, I have to hack the socket retry logic to skip all retries implemented in the library by always throw an error in the 'reconnecting' event handler and detect that in the error event handler for my own retry logic.
I have to also overwrite the entire executeIsolated implementation of the client to use my own pool that actually implements proper error handling.
This is pretty sad, as we have been using v3 in production for a while and had no major issues. The maintenance of v3 had been pretty idle for a long time, and v4 had sparked some hope of this project. But now, we're almost at a decision point to migrate to another library completely. We'll try to hang on for a bit with the above hacks, but that's not a long term solution.

[abalam666]

Setting keepAlive to false, does not fix the issue. After some more debuging i figured out, that during the above scenario this.#socket is undefined when socket.disconnect. Therefore the ClientClosedError is thrown. At the same time this.#isOpen is true. As the error is thrown without setting this.#isOpen to false, socket.#retryConnection is retrying to establish a connection indefinitely.
b5360c0 would fix the issue. Not sure how to write a test for that though.

You can try to run redis own tests and try it. But @leibale will analyze this when he has time

Hello, i am facing the same issue, do you know when pixtron@b5360c0 will be merged ?

[philip-nicholls]

I'm running in to this same issue. In the docs there is mention of two functions to disconnect. Quit and Disconnect. It says use disconnect to "Forcibly close a client's connection to Redis immediately" so that should cleanup all socket resources and stop retrying any connection. The disconnect promise resolves to undefined whether it actually does a disconnect or not. In my tests, the disconnect fails about 1 in 10 times.

I guess I could wrap the redis client connection, mark it as "destroyed" and continuously call disconnect on every new received event until it shuts up, but I didn't expect to have to do this myself.

[philios33]

After some analysis, I think I've found the issue. I don't think the PR will fix anything. The issue is the recursion with this.#connect(retries + 1); It doesn't do any check and will continue to loop even if this.#isOpen has been set to false by .disconnect. This situation happens if you call .disconnect during a (re)connect routine because of the awaiting that happens in the #connect function. I don't think this.#connect should be setting this.#isOpen to true again, perhaps it could be moved to the public this.connect? I'm assuming the point of the variable is to keep track of the .connect and .disconnect calls.

I also noticed a possible workaround to halt an infinite retry loop on a closed client. You can currently "short circuit" a reconnect routine by returning an Error object from the reconnectStrategy function. So just call .disconnect and make sure the reconnectStrategy returns an error. But because of the above bug, you'll need to use your own tracking boolean because the this.#isOpen might be incorrectly true when it should be false.

[duannx]

Hello, is there any update on this issue?

[philip-nicholls]

With all due respect to the developers, the quality of the underlying connection code is poor in this library. If I had the time, I would fix these things myself, but considering the "redis" namespace that this package now has, I expect better quality from this library as it is the "official" node redis connector now. There is still an infinite loop and errors are not being caught properly. I love node and redis but I have migrated to ioredis (which has none of these issues) and I'm sorry to say that I will not use this package for the foreseeable future.

[leibale]

@philip-nicholls I'm sorry you feel this way, the code was rewritten in #2295 (which was released in the last version). If you found a specific issue please share it.

[duannx]

My code scenario is the following:

this.instance = await createClient({
      url: redisURL,
})
await this.instance.connect()
console.log('Redis connected', 'PING', await this.instance.ping())

this.instance.on('error', (err) => {
    console.error('Redis Error', err)
    this.instance.disconnect()
 })

Steps to reproduce:

1. Start the Redis server
2. Run the code. Redis client will connect successfully
3. Stop the Redis server. The error handler will fire continuously
   The console log will be:

Redis connected PING PONG
Redis Error {
  errno: -61
  code: ECONNREFUSED
  ...
}
Redis Error {
  errno: -61
  code: ECONNREFUSED
  ...
}
...
Redis Error {
  errno: -61
  code: ECONNREFUSED
  ...
}
... never stop 

redis version 4.4.0

[pixtron]

@leibale just by looking at #2295 this PR doesn't seem to solve this issue. A call to disconnect() should stop the reconnect loop. Maybe a new flag disconnecting is needed to address this issue?

[leibale]

@pixtron I think that an "ongoing" #connect promise should be stored in a private variable and reused in case it's there. I'll take a look tomorrow.