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

Avoid panic on connection failure #1035

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

jaymell
Copy link
Contributor

@jaymell jaymell commented Jan 24, 2024

This code is not unreachable, so let's handle the error instead of panic.

This code is not unreachable, so let's handle the error instead
of panic.
@nihohit
Copy link
Contributor

nihohit commented Jan 24, 2024

How did you reach that code?

@jaymell
Copy link
Contributor Author

jaymell commented Jan 24, 2024

I actually saw it while doing a bit more local testing of #968. If I manually terminate cluster instances to test recovery, I will occasionally see this. It's also fairly easy to force by doing something egregious like adding a sleep in the auth function. Anyway, clearly it's not unreachable code 😆.

@jaymell jaymell merged commit 087d861 into redis-rs:main Jan 24, 2024
8 of 10 checks passed
@jaymell jaymell deleted the no-panic-on-conn-failure branch January 24, 2024 16:00
@NathanFlurry
Copy link

If the MultiplexedConnection panics, then all future commands also panic. This means that one bad AUTH and connection (and therefore the entire server) is unusable until it's restarted.

Our Redis hosting provider triggers this panic any time they enter a maintenance window. I recommend cutting a release with this fix soon. :)


I can consistently reproduce this issue (before this is merged) by:

  1. Adding a timeout in the authenticate function to give us time to inject a fault

image 21

  1. Add a timeout after the authentication so the driver future finishes first

image 22

  1. Open a connection and kill the Redis server during the authenticate function.

I tested this same code with this PR, can confirm that the connection recovers correctly now.

framlog pushed a commit to framlog/redis-rs that referenced this pull request Feb 28, 2024
This code is not unreachable, so let's handle the error instead
of panic.
NathanFlurry added a commit to rivet-gg/redis-rs that referenced this pull request Feb 29, 2024
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.

None yet

3 participants