Incorrect check of lock

[otto-gebb]

This issue is for a: (mark with an x)

- [x] bug report -> please search issues before submitting
- [ ] feature request
- [ ] documentation issue or request
- [ ] regression (a behavior that used to work and stopped in a new release)

The current implementation of RedisConnection contains this code

try
{
    await _reconnectSemaphore.WaitAsync(RestartConnectionTimeout);
}
catch
{
    // If we fail to enter the semaphore, then it is possible that another thread has already done so.
    // ForceReconnectAsync() can be retried while connectivity problems persist.
    return;
}

Which is an incorrect way to check whether the lock was taken.

The doc of the used overload of WaitAsync says

Returns:
A task that will complete with a result of true if the current thread successfully
entered the System.Threading.SemaphoreSlim, otherwise with a result of false.

So the correct code is

bool lockTaken = await _reconnectSemaphore.WaitAsync(RestartConnectionTimeout);
if (!lockTaken)
{
    // If we fail to enter the semaphore, then it is possible that another thread has already done so.
    // ForceReconnect() can be retried while connectivity problems persist.
    return;
}

With the current implementation a thread that fails to take the lock because of a timeout will proceed with the reconnection, which is not what is intended.

Also, the "double check lock" pattern is implemented incorrectly.
The second check currently looks like this:

try
{
    var utcNow = DateTimeOffset.UtcNow;
    elapsedSinceLastReconnect = utcNow - previousReconnectTime;
// ...
    if (elapsedSinceLastReconnect < ReconnectMinInterval)
    {
        return; // Some other thread made it through the check and the lock, so nothing to do.
    }
//...

The point of the second check is to avoid doing work that has just been done by the thread
that has exited the lock. This goal is not achieved here, because the check is
based on a stale value of _lastReconnectTicks that was read before entering the lock.