fix: Connection Instability with socketTimeout Parameter #9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pinkiesky
force-pushed
the
fix-socketTimeout-connection-instability
branch
from
e0c2d09
to
376d395
Compare
mcollina
requested changes
Oct 12, 2024
|
Sure, no problem |
pinkiesky
force-pushed
the
fix-socketTimeout-connection-instability
branch
from
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. |
Signed-off-by: Aleksandr Zinin <fontercity@gmail.com>
Signed-off-by: Aleksandr Zinin <fontercity@gmail.com>
pinkiesky
force-pushed
the
fix-socketTimeout-connection-instability
branch
from
7475bb9
to
eec432e
Compare
|
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.