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

Redis hangs indefinitely on connection lost #2205

Closed
bssergy opened this issue Jul 20, 2022 · 24 comments · Fixed by #2328
Closed

Redis hangs indefinitely on connection lost #2205

bssergy opened this issue Jul 20, 2022 · 24 comments · Fixed by #2328
Labels

Comments

@bssergy
Copy link

bssergy commented Jul 20, 2022

Here is my code sample:

const redis = require('redis');

const client = redis.createClient({
    url: '127.0.0.1',
    password: 'password',
    disableOfflineQueue: true,
    socket: {
        timeout: 1000,
        connectTimeout: 1000,
        tls: true
    },
});

client.on('error', (err) => console.log('Redis Client Error', err));

// Connection is on here
client.connect().then(async () => {
    // I disconnect the network here and expected to get an error and handle it
    const value = await client.get('key'); // <--- application hangs here
    // this code has never been executed
    await client.quit();
    console.log(value);
}).catch(err => console.log(err));

My application hangs indefinitely if connection lost for some reasons and I want to handle this case. I try to catch every possible errors but it doesn't help - application hangs on get() method and nothing happens.

So I just need to handle redis error on lost connection to be able to log this error and move next in my code.

Environment:

  • Node.js Version: v16.13.0
  • Redis Server Version: 6.0.5
  • Node Redis Version: 4.2.0
  • Platform: Unknown
@bssergy bssergy added the Bug label Jul 20, 2022
@sunilgsuthar
Copy link

Facing the same issue, client hangs at get().

@leibale
Copy link
Collaborator

leibale commented Jul 20, 2022

import { createClient } from 'redis';
import { setTimeout } from 'node:timers/promises';

const client = createClient({
  disableOfflineQueue: true
});

client.on('error', (err) => console.log('Redis Client Error', err.message));

await client.connect();

console.log('Client is connected, stop redis-server');

await setTimeout(5000);

try {
  console.log(await client.ping());
} catch (err) {
  console.log(err.message);
}

console.log('AFTER');
Client is connected, stop redis-server
Redis Client Error Socket closed unexpectedly
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
connect ECONNREFUSED 127.0.0.1:6379 <--
AFTER <--
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379

@sunilgsuthar
Copy link

import { createClient } from 'redis';
import { setTimeout } from 'node:child_process';

const client = createClient({
  disableOfflineQueue: true
});

client.on('error', (err) => console.log('Redis Client Error', err.message));

await client.connect();

console.log('Client is connected, stop redis-server');

await setTimeout(5000);

try {
  console.log(await client.ping());
} catch (err) {
  console.log(err.message);
}

console.log('AFTER');
Client is connected, stop redis-server
Redis Client Error Socket closed unexpectedly
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
connect ECONNREFUSED 127.0.0.1:6379 <--
AFTER <--
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379
Redis Client Error connect ECONNREFUSED 127.0.0.1:6379

Didn't understand this!

@leibale
Copy link
Collaborator

leibale commented Jul 21, 2022

@sunilgsuthar I've tried to reproduce the error using the code in my last comment, but it seems like it "works" (throw an error) as exptected

@sunilgsuthar
Copy link

@bssergy did you find any solution to this?

@graned
Copy link

graned commented Sep 28, 2022

An alternative is to inject the redis client, HOWEVER, you need to use the redis version 3.x.x, if you use version 4.x.x the library never resolves the promises because the callbacks are never called. I must admit this is very annonying

@schmod
Copy link

schmod commented Oct 4, 2022

@leibale Try again with the timeout and connectTimeout settings as described in the original issue. I have a suspicion that's part of the condition necessary to trigger this bug...

@GledsonAfonso
Copy link

Any news about this? I have been facing this problem for a while now.

@leibale
Copy link
Collaborator

leibale commented Nov 12, 2022

@GledsonAfonso have you set disableOfflineQueue to true?

@GledsonAfonso
Copy link

GledsonAfonso commented Nov 16, 2022

@GledsonAfonso have you set disableOfflineQueue to true?

Thanks for the reply and sorry for my late reply, I just manage to test this today. It didn't work, though. Here's the code from my Redis client:

    _client = createClient({
      legacyMode: true,
      disableOfflineQueue: true,
      socket: {
        port: environment.redisPort,
        host: environment.redisHost
      }
    });
    
    await _client.connect();

The way I'm testing this is through Docker Compose. I'm starting the Redis server along with an API that needs to connect to it and then I disconnect the Redis server and watch the logs from the API. When this happens, it fails and the application is successfully closed, but when I start it without starting the Redis server first, it gives me this log:

ConnectionTimeoutError: Connection timeout
    at Socket.<anonymous> (/app/node_modules/@redis/client/dist/lib/client/socket.js:168:124)
    at Object.onceWrapper (node:events:641:28)
    at Socket.emit (node:events:527:28)
    at Socket.emit (node:domain:475:12)
    at Socket._onTimeout (node:net:522:8)
    at listOnTimeout (node:internal/timers:559:17)
    at processTimers (node:internal/timers:502:7)

And the API doesn't fail, allowing the application to hang infinitely whenever some data are requested.

What I needed was that the application to throw some error when data are requested, instead of just hanging like that. I need this because the API needs to redirect the request to S3 storage whenever this problem happens.

@leibale
Copy link
Collaborator

leibale commented Nov 16, 2022

@GledsonAfonso wanna give #2328 a shot?

@GledsonAfonso
Copy link

@GledsonAfonso wanna give #2328 a shot?

Sure! How do I do that?

@leibale
Copy link
Collaborator

leibale commented Nov 20, 2022

@GledsonAfonso if you are testing locally: clone my fork, checkout to the "fix-2205" branch, run npm i && npm run build-all in the repo main folder, and change your package.json to install from the local folder ("redis": "file:..." instead of "redis": "4.5.0")

@GledsonAfonso
Copy link

@GledsonAfonso if you are testing locally: clone my fork, checkout to the "fix-2205" branch, run npm i && npm run build-all in the repo main folder, and change your package.json to install from the local folder ("redis": "file:..." instead of "redis": "4.5.0")

Okay, I'm going to try that.

@GledsonAfonso
Copy link

@GledsonAfonso if you are testing locally: clone my fork, checkout to the "fix-2205" branch, run npm i && npm run build-all in the repo main folder, and change your package.json to install from the local folder ("redis": "file:..." instead of "redis": "4.5.0")

It still hangs.

@leibale
Copy link
Collaborator

leibale commented Nov 21, 2022

@GledsonAfonso wanna debug it together (because I'm having a hard time reproducing it)?

@GledsonAfonso
Copy link

@GledsonAfonso wanna debug it together (because I'm having a hard time reproducing it)?

Sure! What time you will be available and through which channel?

@leibale
Copy link
Collaborator

leibale commented Nov 22, 2022

@GledsonAfonso I'll be in this zoom call for the next few hours, hope you'll have time to join :)

@GledsonAfonso
Copy link

@GledsonAfonso I'll be in this zoom call for the next few hours, hope you'll have time to join :)

Okay! Thanks!

@GledsonAfonso
Copy link

Thanks for your help, @leibale!

Just in case someone else needs this, what solved my problem was basically some changes to the client code, as below:

_client = createClient({
  legacyMode: true,
  disableOfflineQueue: true,
  socket: {
    port: environment.redisPort,
    host: environment.redisHost,
    reconnectStrategy: () => 3000
  }
});

_client.on('error', (error) =>
  console.error(`Connection to Redis server failed! Error ${error}`)
);

_client.connect();

Some pointers of what changed:

  • Setting disableOfflineQueue: true, as suggested by @leibale before
  • Adding implementation for the error event (_client.on() part)
  • Removing the await when connecting to the server on _client.connect()

With those changes, now my application keeps retrying to reconnect with the server when the latter is offline and is able to default to the S3 storage after the timeout set on the reconnectStrategy param (without hanging too, which is great!).

Cheers guys! Thanks again!

@leibale
Copy link
Collaborator

leibale commented Nov 22, 2022

@GledsonAfonso thanks for summarizing it :)

just one note, I would suggest using the default reconnectStrategy instead of passing a custom one (retries => Math.min(retries * 50, 500))

@GledsonAfonso
Copy link

@GledsonAfonso thanks for summarizing it :)

just one note, I would suggest using the default reconnectStrategy instead of passing a custom one (retries => Math.min(retries * 50, 500))

Thanks for the heads up. Will do!

@leibale
Copy link
Collaborator

leibale commented Nov 24, 2022

@GledsonAfonso [email protected]/@redis/[email protected] is on npm :)

@GledsonAfonso
Copy link

GledsonAfonso commented Mar 3, 2023

@GledsonAfonso [email protected]/@redis/[email protected] is on npm :)

A little late to say that, but thanks! ^^

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.

6 participants