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

_addedScriptHashesCleanInterval is not always cleared on redis.quit() #1215

Closed
trentm opened this issue Oct 26, 2020 · 4 comments
Closed

_addedScriptHashesCleanInterval is not always cleared on redis.quit() #1215

trentm opened this issue Oct 26, 2020 · 4 comments
Labels

Comments

@trentm
Copy link

trentm commented Oct 26, 2020

Hi. I believe 3b22f8f added a bug where the new this._addedScriptHashesCleanInterval = setInterval(...) (3b22f8f#diff-d3d9763dec19cf3dd977c437e0be3e6068b13161ee5a2b9b0bf225bddd2d83f8R188) isn't cleared via clearInterval() in all cases. This results in a dangling active handle that will hang a node process.

Here is a repro:

var Redis = require('.'); // ioredis
var redis = new Redis();
redis.on('ready', function () {
    console.log('ready');
    redis.quit();
    console.log('done');
});
% node hang.js
ready
done
<hangs here>

or with ioredis debug output:

% DEBUG=ioredis:* node hang.js
  ioredis:redis status[localhost:6379]: [empty] -> connecting +0ms
  ioredis:redis status[127.0.0.1:6379]: connecting -> connect +6ms
  ioredis:redis write command[127.0.0.1:6379]: 0 -> info([]) +2ms
  ioredis:redis status[127.0.0.1:6379]: connect -> ready +4ms
ready
  ioredis:redis write command[127.0.0.1:6379]: 0 -> quit([]) +1ms
done
  ioredis:redis status[127.0.0.1:6379]: ready -> close +3ms
  ioredis:connection skip reconnecting since the connection is manually closed. +0ms
  ioredis:redis status[127.0.0.1:6379]: close -> end +1ms
<hangs here>

This does not happen if one uses redis.disconnect() instead of redis.quit().

When calling redis.quit(), the closeHandler (in event_handler.ts) is called with self.manuallyClosing == true which does not call the clearInterval added to Redis.prototype.disconnect().

Some options I see (though I am not very familiar with ioredis, so I'm definitely happy to be overriden):

  1. Add this._addedScriptHashesCleanInterval.unref() (https://nodejs.org/api/timers.html#timers_timeout_unref) to the connect method. That'll prevent node from hanging, but will persist the setInterval after a program is presumably finished with the ioredis session.
  2. Refactor the use of the closeHandler to explicitly call clearInterval.
  3. Refactor access to this._addedScriptHashes so that it doesn't require a setInterval to clear it. E.g. perhaps something like https://github.com/isaacs/node-lru-cache using its maxAge option.
trentm added a commit to elastic/apm-agent-nodejs that referenced this issue Oct 26, 2020
[email protected] includes a bug when using `redis.quit()` that can leave
a setInterval uncleared. See redis/ioredis#1215.

Fixes #1838
trentm added a commit to elastic/apm-agent-nodejs that referenced this issue Oct 26, 2020
[email protected] includes a bug when using `redis.quit()` that can leave
a setInterval uncleared. See redis/ioredis#1215.

Fixes #1838
@AVVS
Copy link
Collaborator

AVVS commented Oct 27, 2020

@ShogunPanda could you take a stab at fixing this?

@AVVS AVVS added the bug label Oct 27, 2020
@ShogunPanda
Copy link
Contributor

@AVVS Sure. Tomorrow (CET) I will take a look!

@ShogunPanda
Copy link
Contributor

@AVVS Fixed in #1219 :)

@AVVS AVVS closed this as completed Oct 28, 2020
ioredis-robot pushed a commit that referenced this issue Oct 28, 2020
## [4.19.1](v4.19.0...v4.19.1) (2020-10-28)

### Bug Fixes

* Make sure script caches interval is cleared. [[#1215](#1215)] ([d94f97d](d94f97d))
janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this issue Mar 1, 2024
## [4.19.1](redis/ioredis@v4.19.0...v4.19.1) (2020-10-28)

### Bug Fixes

* Make sure script caches interval is cleared. [[#1215](redis/ioredis#1215)] ([d94f97d](redis/ioredis@d94f97d))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants