node-redis icon indicating copy to clipboard operation
node-redis copied to clipboard

Memory leak : ReddisClient () slowly increases the memory and finally crashes the server

Open srikrishnaturlapati opened this issue 3 years ago • 7 comments

I'm using "redis": "^3.1.2". My containers are restarting/ crashing in prod due to a memory leak. The traffic is very very less around 650 in 24 hours. Following are my observations:

  1. For the first few 100 requests the memory would be around 200/300 MB and it would stay the same even after an hour without traffic.GC couldn't clear the memory. so after subsequent few 100 requests, the memory increases and it crashes my container (my container setting in prod is 1GB.)

  2. I have used google chrome inspect to understand which object or function is causing this. And it turned out to be Redis client is holding very less memory at the beginning and after leaving it for a while the memory it holds keeps on increasing.(I have taken the snapshots of heap over time and compared the difference).

  3. I'm actually connecting to reddis for every request and closing the connection using the connection.quit(). But another observation I found inside the Redis module there is a stream that continuously runs and does something recursively. upon debugging I found the connection is still alive. So I changed the method to end() instead of quit and it still produces the stream. and the connection is still alive. At this point, I'm literally stuck. please help me with the following question 👍

    I) is it advised to connect to Redis and close for every request. 2) Is it normal to have a memory leak when using Redis module 3) irrespective of quit or end the stream doesn't stop in the background (for example keep a debug in individual.commands.js -> info_callback method and monitor the connection status.)

Code

`connect(config) { const connectionRetryTime = 1000 * 60 * 60; const clientOptions = { host: config.endpoint.host, port: config.endpoint.port, socket_keepalive: true, password: config.redisAuth, tls: true, retry_strategy: (options) => { if (options.error && options.error.code === 'ECONNREFUSED') { return new Error('The server refused the connection'); } if (options.total_retry_time > connectionRetryTime) { return new Error('Retry time exhausted'); } if (options.attempt > 2) { return undefined; }

    return Math.min(options.attempt * 100, 3000);
  },
};

return new Promise(((resolve, reject) => {
  try {
    this.cacheProvider = redis.createClient(clientOptions);
    this.cacheProvider.on('error', () => {
      // eslint-disable-next-line no-console
      console.error('Error Connecting to Redis error');
    });

    // Get promisified version of RedisClient functions
    this.getAsync = promisify(this.cacheProvider.get).bind(this.cacheProvider);
    this.setAsync = promisify(this.cacheProvider.set).bind(this.cacheProvider);
    this.setexAsync = promisify(this.cacheProvider.setex).bind(this.cacheProvider);
    this.keysAsync = promisify(this.cacheProvider.keys).bind(this.cacheProvider);
    this.pingAsync = promisify(this.cacheProvider.ping).bind(this.cacheProvider);
    this.quitAsync = promisify(this.cacheProvider.quit).bind(this.cacheProvider);

    return resolve(this.cacheProvider);
  } catch (error) {
    return reject(error);
  }
}));

}

init(config) { return this.connect(config); }

healthCheck() { return this.pingAsync(); }

async release() { return this.cacheProvider.quitAsync; } `

For each request I'm

init(); get(); release();

Environment:

  • **Node.js Version14: <!-- e.g. "node --version" -->
  • Redis Server Version: <!-- e.g. "redis-server --version" -->
  • **Node Redis Version3.1.2:
  • **PlatformmacOS 12.4:

srikrishnaturlapati avatar Jul 19 '22 18:07 srikrishnaturlapati

Maybe the problem is that you dont call the quitAsync function?

async release() {
  return this.cacheProvider.quitAsync();
}

BTW, there is no reason to open and close a client per request, you should use one global client for all concurrently running requests (unless you are using per connection feature, like blocking commands)

leibale avatar Jul 19 '22 20:07 leibale

Thanks, Leibale , It was causing the same memory leak after removing that too. It is still creating the recurse. Also Do you know I can override the behavior of that background stream.

srikrishnaturlapati avatar Jul 19 '22 21:07 srikrishnaturlapati

The code below does not reproduce the memory leak

import { createServer } from 'node:http';
import redis from 'redis';

createServer((req, res) => {
  const client = redis.createClient();
  client.get('key', (err, reply) => {
    client.quit(err => {
      if (err) client.end();
    });

    if (err) {
      res.writeHead(500);
      res.end(err.message);
      return;
    }

    res.writeHead(200);
    res.end(JSON.stringify(reply));
  });
}).listen(3000);
npx loadtest -n 1000000 -c 500 http://localhost:3000

Any idea why?

(and as I already said, you should not create a client per request)

leibale avatar Jul 20 '22 11:07 leibale

Thank you for the response. And yes I have considered having one active connection rather per request. I will try to make those changes and will update here soon. Thank you So much Leibale

srikrishnaturlapati avatar Jul 20 '22 15:07 srikrishnaturlapati

I am also experiencing a similar issue. We are using node-cache-manager-redis-store, which has a dependency "redis":"^3.0.2"; currently pulling in redis:3.1.2

We are using a global instance of the NestJS cache-manager, doing cache operations on an interval of 2.5 seconds, and every 7 hours Node runs out of memory and crashes.

JoeCap08055 avatar Jul 21 '22 12:07 JoeCap08055

@JoeCap08055 Thank you for the post. Are you using single connection per node ? or creating connections per request. Also how are you planning to fix this ?

srikrishnaturlapati avatar Jul 21 '22 15:07 srikrishnaturlapati

I am using the NestJS CACHE_MANAGER. It is a global instance in the app, I believe. I don't know how Nest manages the actual connections.

For now, I've solved the problem by eliminating NestJS CACHE_MANAGER and going back to using ioredis (which is what we we doing prior to upgrading to NestJS 8)

JoeCap08055 avatar Jul 21 '22 20:07 JoeCap08055