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

fix: Connection Instability with socketTimeout Parameter #9

Merged

Conversation

pinkiesky
Copy link
Contributor

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 with prependListener to ensure the correct order.

@pinkiesky pinkiesky force-pushed the fix-socketTimeout-connection-instability branch from e0c2d09 to 376d395 Compare October 8, 2024 11:55
Copy link
Collaborator

@mcollina mcollina left a 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?

@alzinin
Copy link

alzinin commented Oct 12, 2024

Sure, no problem

@pinkiesky pinkiesky force-pushed the fix-socketTimeout-connection-instability branch from ae1ca8b to 7475bb9 Compare October 23, 2024 20:02
@pinkiesky
Copy link
Contributor Author

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
is it a good way to handle this?

@mcollina
Copy link
Collaborator

Can you open a separate PR with those two commits? We'll land that before this.

@pinkiesky pinkiesky force-pushed the fix-socketTimeout-connection-instability branch from 7475bb9 to eec432e Compare October 30, 2024 14:37
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Collaborator

The test does not really cover the use case of the bug. Can you add a test that actually reproduce the loop?

@pinkiesky
Copy link
Contributor Author

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

@pinkiesky pinkiesky force-pushed the fix-socketTimeout-connection-instability branch from eec432e to 26904a0 Compare November 5, 2024 10:11
Signed-off-by: Aleksandr Zinin <[email protected]>
@pinkiesky pinkiesky force-pushed the fix-socketTimeout-connection-instability branch from 7f4ee5e to 19af5e6 Compare November 5, 2024 11:27
@pinkiesky
Copy link
Contributor Author

A functional test for socketTimeout was just added

@mcollina, could you check it and provide feedback please?

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit ae27385 into valkey-io:main Nov 5, 2024
8 checks passed
@pinkiesky pinkiesky deleted the fix-socketTimeout-connection-instability branch November 5, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants