-
Notifications
You must be signed in to change notification settings - Fork 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
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
socketTimeout
parameter to a non-zero value, the Redis connection becomes unstable after startup)The problem is that the
auth
command 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
on
withprependListener
to ensure the correct order.