-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
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. |
I now noticed that the other timeout options follow a consistent naming pattern:
so I renamed this option to I've also documented |
# [4.25.0](v4.24.6...v4.25.0) (2021-04-02) ### Features * added commandTimeout option ([#1320](#1320)) ([56f0272](56f0272))
🎉 This PR is included in version 4.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Awesome! Great work Andres! |
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? |
# [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))
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?