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

fix: clear script caches interval on each connection attempt #1232

Closed
wants to merge 1 commit into from

Conversation

ejose19
Copy link

@ejose19 ejose19 commented Nov 6, 2020

Follow-up fix for: #1215

After using tools like why-is-node-running, I saw:

this._addedScriptHashesCleanInterval = setInterval(() => {
  this._addedScriptHashes = {};
}, this.options.maxScriptsCachingTime);

appearing multiple times (after quit was called), which means quit was only clearing the last of them, but there were many that were overwritten by that same call when connect is called multiple times (as in not connecting the first time). This PR ensures that the interval is cleared before assigning a new one to the variable holding them.

@AVVS
Copy link
Collaborator

AVVS commented Nov 6, 2020

Hey, this looks awesome. Any chance you are able to add a test case for this so that we can ensure there are no regressions in the future?

@ejose19
Copy link
Author

ejose19 commented Nov 6, 2020

I couldn't think of any test for this particular fix, as the way I found it is either using a low level tool why-is-node-running, or noticing why the server was hanging on SIGTERM, but the tests are ignoring background intervals so I don't know how I would be able to test them (ioredis still closes without issues, but the node process does not end). If you know of any test case for it using the existing test tools I may be able to work on it.

@ShogunPanda
Copy link
Contributor

@ejose19 Thanks for filing this.

After your bug report, I've investigated separately and I found the same fix, which I incorporated in #1226.

@AVVS I've tried to add a test case in that PR as well, but honestly I was not able to reproduce the failure as well.

@luin luin closed this in 515d9ea Dec 13, 2020
ioredis-robot pushed a commit that referenced this pull request Dec 13, 2020
## [4.19.4](v4.19.3...v4.19.4) (2020-12-13)

### Bug Fixes

* prevent duplicate intervals being set. ([#1244](#1244)) ([515d9ea](515d9ea)), closes [#1232](#1232) [#1226](#1226) [#1232](#1232) [/github.com/luin/ioredis/blob/v4.19.2/lib/cluster/index.ts#L311-L313](https://github.com//github.com/luin/ioredis/blob/v4.19.2/lib/cluster/index.ts/issues/L311-L313)
@ioredis-robot
Copy link
Collaborator

🎉 This issue has been resolved in version 4.19.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this pull request Mar 1, 2024
## [4.19.4](redis/ioredis@v4.19.3...v4.19.4) (2020-12-13)

### Bug Fixes

* prevent duplicate intervals being set. ([#1244](redis/ioredis#1244)) ([515d9ea](redis/ioredis@515d9ea)), closes [#1232](redis/ioredis#1232) [#1226](redis/ioredis#1226) [#1232](redis/ioredis#1232) [/github.com/luin/ioredis/blob/v4.19.2/lib/cluster/index.ts#L311-L313](https://github.com//github.com/luin/ioredis/blob/v4.19.2/lib/cluster/index.ts/issues/L311-L313)
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 this pull request may close these issues.

4 participants