Avoid retrying on IO errors when it’s unclear if the server received the request #2479
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
13a11da to
53bfd90
Compare
…the request Signed-off-by: barshaul <barshaul@amazon.com>
53bfd90 to
fe9b603
Compare
eifrah-aws
reviewed
Oct 20, 2024
Signed-off-by: barshaul <barshaul@amazon.com>
eifrah-aws
approved these changes
Oct 21, 2024
| @@ -7,7 +7,7 @@ use crate::connection::{ | |||
| resp2_is_pub_sub_state_cleared, resp3_is_pub_sub_state_cleared, ConnectionAddr, ConnectionInfo, | |||
| Msg, RedisConnectionInfo, | |||
| }; | |||
| #[cfg(any(feature = "tokio-comp"))] | |||
| #[cfg(feature = "tokio-comp")] | |||
| @@ -83,7 +83,7 @@ macro_rules! reconnect_if_dropped { | |||
| macro_rules! reconnect_if_io_error { | |||
There was a problem hiding this comment.
nit: macro should be renamed to reflect the change: e.g. reconnect_if_conn_dropped
There was a problem hiding this comment.
i'll change it, but in a different PR we need to remove this whole file - we have no use in redis-rs's connection_manager
Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
prateek-kumar-improving
pushed a commit
that referenced
this pull request
Oct 21, 2024
…the request (#2479) * Avoid retrying on IO errors when it’s unclear if the server received the request Signed-off-by: barshaul <barshaul@amazon.com>
avifenesh
pushed a commit
to avifenesh/valkey-glide
that referenced
this pull request
Oct 21, 2024
…the request (valkey-io#2479) * Avoid retrying on IO errors when it’s unclear if the server received the request Signed-off-by: barshaul <barshaul@amazon.com> Signed-off-by: avifenesh <aviarchi1994@gmail.com>
Muhammad-awawdi-amazon
pushed a commit
to Muhammad-awawdi-amazon/valkey-glide
that referenced
this pull request
Oct 22, 2024
…the request (valkey-io#2479) * Avoid retrying on IO errors when it’s unclear if the server received the request Signed-off-by: barshaul <barshaul@amazon.com>
Muhammad-awawdi-amazon
pushed a commit
to Muhammad-awawdi-amazon/valkey-glide
that referenced
this pull request
Oct 22, 2024
…the request (valkey-io#2479) * Avoid retrying on IO errors when it’s unclear if the server received the request Signed-off-by: barshaul <barshaul@amazon.com>
cyip10
pushed a commit
that referenced
this pull request
Oct 23, 2024
…the request (#2479) * Avoid retrying on IO errors when it’s unclear if the server received the request Signed-off-by: barshaul <barshaul@amazon.com>
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.
Main Change:
ATM, on I/O errors, we would reconnect to the failed node and retry the request if there were more retries left. This approach had a critical issue: we couldn’t reliably determine if the server had already received the request before the connection was broken. Retrying in such cases could result in duplicate command execution.
Example:
INCR key.INCR key.This PR differentiates between errors where it’s safe to retry and those where it’s not. Specifically, with multiplexed connections, if the
sendfunction returns an error, it guarantees that the server never received the data, making retries safe (see https://docs.rs/tokio/latest/tokio/sync/mpsc/struct.Sender.html#method.send). For other errors, where we can’t be certain, retries are unsafe and will not be automatically attempted. Instead, these errors will now be returned to the user, who must manually retry if they determine it’s safe.Test Changes:
Since I/O errors are now returned to the user, tests that previously killed the server now loop to retry the request, simulating the handling of I/O errors on the user side.
Refresh Slots Change:
While testing this fix, I found that when all connections were unavailable and
refresh_slotswas called, it didn’t raise the expectedallConnectionsUnavailableerror. This has been fixed by updatingrandom_connectionsfunction to return anOption. Now, if no connections are found,refresh_slotsraises theallConnectionsUnavailableerror immediately. The state then shifts to reconnecting to the initial nodes, and slot refreshes are attempted using the new connections.Out of Scope:
A future PR could introduce a new configuration option to enable retries on connection error, allowing users to control this behavior at the client level.