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

Added timeoutPerRequest option #1320

Merged
merged 5 commits into from
Apr 2, 2021
Merged

Conversation

mjomble
Copy link
Contributor

@mjomble mjomble commented Apr 1, 2021

Closes #61

Based on the fact that this feature was discussed back in 2015 (#61) and is still not implemented, I'm assuming the implementation in this PR is a very naive one and there's a bunch of special cases it's not covering. But maybe it can still get the ball rolling 🙂

@luin you've said that "It's not easy to support timeoutPerRequest option efficiently". Could you elaborate on this?
Is setTimeout inefficient? Or would we need to add a bunch of additional logic to this PR that would slow things down?

@luin
Copy link
Collaborator

luin commented Apr 1, 2021

you've said that "It's not easy to support timeoutPerRequest option efficiently". Could you elaborate on this?

Hmm...good question. I thought having hundreds of timers would cause performance impact, but seems it isn't the case. Another reason I didn't add this feature was I thought it could be easily implemented on client side via ex Promise.race, and I didn't want to mislead users to think that the timeout was applied on the server side (like a Lua script can't be interrupted if it timed out). However, as users pointed out that if they used third-party libraries they won't be able to wrap Promise.race so it may be worth to add this feature.

@mjomble
Copy link
Contributor Author

mjomble commented Apr 1, 2021

I now noticed that the other timeout options follow a consistent naming pattern:

  • connectTimeout
  • disconnectTimeout
  • slotsRefreshTimeout

so I renamed this option to commandTimeout.
I also added a sentinelCommandTimeout option that's passed on to the internally created Redis client for sentinels.

I've also documented commandTimeout, but I noticed that no sentinel-specific options have currently been documented, so I left sentinelCommandTimeout undocumented for now. Perhaps the sentinel options could be documented all at once in a separate PR.

@mjomble mjomble marked this pull request as ready for review April 1, 2021 13:19
@luin luin merged commit 56f0272 into redis:master Apr 2, 2021
ioredis-robot pushed a commit that referenced this pull request Apr 2, 2021
# [4.25.0](v4.24.6...v4.25.0) (2021-04-02)

### Features

* added commandTimeout option ([#1320](#1320)) ([56f0272](56f0272))
@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.25.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@luin
Copy link
Collaborator

luin commented Apr 2, 2021

Awesome! Great work Andres!

@varungbt
Copy link

varungbt commented Nov 9, 2021

From the code I see that when a command timeout occurs, the promise is rejected. Is there a way to listen for command timeout events?

janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this pull request Mar 1, 2024
# [4.25.0](redis/ioredis@v4.24.6...v4.25.0) (2021-04-02)

### Features

* added commandTimeout option ([#1320](redis/ioredis#1320)) ([56f0272](redis/ioredis@56f0272))
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.

Retries per request limit
4 participants