clears commandTimeout timer as each respective command gets fulfilled

[JosePedroDiasSky]

Currently, using the commandTimeout option is creating one timer per command and they're not being cleared as each command gets fulfilled. This PR does that.

[luin]

@mjomble Can you take a look?

[mjomble]

Should be able to just use NodeJS.Timeout as the type

[JosePedroDiasSky]

Looks ok, but I'm not sure if it's necessary.
Are the timers causing actual measurable performance problems or other issues without this change?

If we start clearing them just in case, we should also consider the cost of increased code complexity (maintenance overhead, wider surface area for possible future bugs etc).
Sometimes an optimization saves a few microseconds of CPU time, but can cost hours of developer time at some point in the future.
Might not be the case here, but just something to consider 🙂

If you have a lingering timeout per command and you're doing several commands I'd say it's relevant and the additional complexity for this single operation is marginal, but it's up to you.

Regarding the type change, I can do as your suggested if you feel it's a better fit than using return type. I had tried Timeout without the namespace and number as well and ended up here.

[JosePedroDiasSky]

updated the type as suggested.

[mjomble]

Looks ok, but I'm not sure if it's necessary.
Are the timers causing actual measurable performance problems or other issues without this change?

If we start clearing them just in case, we should also consider the cost of increased code complexity (maintenance overhead, wider surface area for possible future bugs etc).
Sometimes an optimization saves a few microseconds of CPU time, but can cost hours of developer time at some point in the future.
Might not be the case here, but just something to consider 🙂

[mjomble]

In case more comments are expected from me before merging this:

I haven't done any actual performance testing (either with or without this change), but it looks ok to merge 🙂

[luin]

@mjomble I just added another commit to fix leaking timers caused by commands being sent with Redis#sendCommand() twice (because once client is ready, commands in the offline queue will be sent with Redis#sendCommand() again). Can you review again and squash this (you are a collaborator so should have permission to do this)?

I think comparing to performance, it makes more sense to merge this commit for functionality reasons as pending timers prevent the process from exiting. For example, before this PR the following code won't exit:

const Redis = require("ioredis");
const redis = new Redis({ commandtimeout: 100000000 });

redis.set("foo", "bar").finally(() => {
  redis.disconnect();
  console.log("should exit now");
});

BTW We should always use squash instead of merge, the commit message for this PR should have a prefix of fix: . Refer to https://github.com/luin/ioredis/blob/master/.github/CONTRIBUTING.md for details. Thanks!

[mjomble]

Now that #1328 is merged, you can use await server.disconnectPromise() here

[mjomble]

LGTM

---

I think comparing to performance, it makes more sense to merge this commit for functionality reasons as pending timers prevent the process from exiting

Good point, I hadn't though of that.

---

Can you review again and squash this (you are a collaborator so should have permission to do this)?

I'm not sure if I should accept the collaborator invitation. Most of my changes were related to a project I'm working on, which is almost complete, so I probably won't be able to contribute much in the future 🙁

[luin]

Most of my changes were related to a project I'm working on, which is almost complete.

Oh, gotcha. Congratulations on your project completion! 😄

[ioredis-robot]

🎉 This PR is included in version 4.27.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀