-
Notifications
You must be signed in to change notification settings - Fork 21
fix: Connection Instability with socketTimeout Parameter #9
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: Connection Instability with socketTimeout Parameter #9
Conversation
e0c2d09 to
376d395
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening a PR! Can you please add a unit test?
|
Sure, no problem |
ae1ca8b to
7475bb9
Compare
|
I just added the small unit for the changed subscribe logic, but I also noted that we missed commits that add "socketTimeout" to the lib. It happened because the socketTimeout was added after the fork was created I can propose merging this PR (if you don't mind), and later, we will copy these missed commits to the repo. @mcollina |
|
Can you open a separate PR with those two commits? We'll land that before this. |
7475bb9 to
eec432e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
The test does not really cover the use case of the bug. Can you add a test that actually reproduce the loop? |
The best idea I got is to create functional one for the case. Yes, I will |
Signed-off-by: Aleksandr Zinin <[email protected]>
Signed-off-by: Aleksandr Zinin <[email protected]>
eec432e to
26904a0
Compare
Signed-off-by: Aleksandr Zinin <[email protected]>
7f4ee5e to
19af5e6
Compare
|
A functional test for socketTimeout was just added @mcollina, could you check it and provide feedback please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This PR is fixing redis/ioredis#1919 issue (When setting the
socketTimeoutparameter to a non-zero value, the Redis connection becomes unstable after startup)The problem is that the
authcommand is sent before DataHandler is created on connection startup. This ruins the correct listener orders--the parser listener shall execute before the other ones.To avoid this, I replaced
onwithprependListenerto ensure the correct order.