StackExchange / StackExchange.Redis

General purpose redis client
https://stackexchange.github.io/StackExchange.Redis/
Other
5.86k stars 1.5k forks source link

Reconnection is attempted continuously regardless of ReconnectRetryPolicy #2557

Open user-45-20 opened 10 months ago

user-45-20 commented 10 months ago

I'm creating my connection like this:

_readConn = ConnectionMultiplexer.Connect(
    new ConfigurationOptions
    {
        EndPoints = { { "localhost", 6380 } },
        Password = dbPass,
        Ssl = false,
        AbortOnConnectFail = true,
        ConnectRetry = 3,
        ReconnectRetryPolicy = new MyPolicy(),
        ConnectTimeout = 2000,
        KeepAlive = 5,
    }
);
_readConn.ConnectionFailed += (object? sender, ConnectionFailedEventArgs args) =>
{
    _logger.LogDebug("ConnectionFailed");
};
_readConn.ConnectionRestored += (object? sender, ConnectionFailedEventArgs args) =>
{
    _logger.LogDebug("ConnectionRestored");
};

public class MyPolicy : IReconnectRetryPolicy
{
    public MyPolicy() { }
    public bool ShouldRetry(long currentRetryCount, int timeElapsedMillisecondsSinceLastRetry)
    {
        return false;
    }
}

I then do two different tests:

in both cases the multiplexer continuously attempts to reconnect even though the policy says not to.

I can verify this by:

Inspecting the code a bit more, it looks like when the connection is lost, ReadFromPipe will call OnDisconnected, which will then call TryConnect, which, when that connection attempt fails (inside BeginConnectAsync) will once again call OnDisconnected, etc etc.

Shouldn't the policy be applied here and prevent any new connections? Or am I misunderstanding and the reconnect policy is meant for some other case?

NickCraver commented 10 months ago

This may be a documentation deal more than anything - it's really only intended for initial connections and ill-suited for reconnects (it'd almost always cause more packet drops with the default policy in place for disconnects). But I also see that instant reconnects here isn't desirable - we need think about this a bit more because the nuances of how many-at-once connections disconnected and reconnect behave (e.g. you want it instant during normal failovers) and some other bits aren't so obvious. Plugging this in as-is would immediately make a lot of reconnects take 0-1000ms longer and that's going to hurt way more cases than it helps.

It might be a 3.0 thing, where we enhance the interface or...something. Curious on everyone's thoughts here, I agree it doesn't behave correctly at the moment looking at your issue, but nor can we just plug the existing bool calculation in, IMO.

user-45-20 commented 10 months ago

it's really only intended for initial connections

Just FYI, I tried this case too by setting AbortOnConnectFail to false, starting my code without the database running, then I started the DB a few seconds later. In the period before the DB came up the multiplexer was still trying to connect in a loop, and as soon as the DB came up it instantly connected to it.

Not sure if this flow is what you mean by initial connection, though.

And yeah, the way the docs currently word it, I took it to mean that it applies to the case of an existing connection being interrupted:

StackExchange.Redis automatically tries to reconnect in the background when the connection is lost for any reason. It keeps retrying until the connection has been restored.