Lock can be extended for unlimited times

[willem810]

When trying out the expiry property, it came to my concerns that my locks did not expire.
After some research i found out the problem arised because the lock gets auto-extended.

I agree, when a process takes longer than expected, a client should be able to extend it's lock.
But i do have concerns about the way this is implemented. The client is now able to extend it's lock for unlimited times as far as I understand?

My concern is when a client gets stuck in a loop for example. It didn't crash or became unresponsive, so it's still able to extend the lock in the background. Now the lock get's auto-extended and the client won't manually unlock the lock, thus deadlocking the process.

I did find a property inside the RedLock class, ExtendCount, keeping track of the amount of times the lock is extended by the client, but it's not used.

There should be a limit on how often a lock can be extended, as described by this article:

so the maximum number of lock reacquisition attempts should be limited, otherwise one of the liveness properties is violated

Am i missing something here?

[zhershds]

I have the same issue. If some client takes the lock for very long time lock is not unlocked automatically.

[MatthewLymer]

This is how other concurrency primitives work, lock (object), and SemaphoreSlim, are you suggesting that this should have a separate behavior by automatically releasing the lock?

You can achieve this already by making a timer and calling Dispose on the acquired lock.

[li-zhixin]

We want the lock to be released automatically after the expiration time rather than being extended indefinitely (even if the code surrounded by the lock finishes executing), so we can't use dispose.
I would like to respect the semantics of the expiration time, at least the extension time should be optional, what do you think?

[MatthewLymer]

We want the lock to be released automatically after the expiration time rather than being extended indefinitely (even if the code surrounded by the lock finishes executing), so we can't use dispose.
I would like to respect the semantics of the expiration time, at least the extension time should be optional, what do you think?

I don't see what prevents you from using Dispose, in fact, you don't even need to use a timer, you can use a CancellationTokenSource and cts.Register(theLock.Dispose)

[stg609]

So, what dose the expiryTime here actually mean if the lock is not released after that time? It really confused me.

[duraes-fd]

Hi, I think this is still an issue.

We started to use Redlock recently, and from time to time, we are getting resources locked forever.

We use an IDisposable wrapper around the RedLock instance to add some logging behaviours, we linked the wrapper Dispose() directly to the underlying RedLock, and we can see that most locks are being disposed of in the logs correctly, but some seem not to be.

But from time to time, the lock keeps renewing itself forever.
We have long-living instances, so this ends up in memory/cpu/redis leaks creeping into our resource usage.

Can we either specify a maximum automatic renewal time window or auto-renewal count?

[samcook]

In these situations, is the lock continuing to renew itself due to execution still being inside your code within the redlock using block? Or do you think that execution has left the using block, but the lock disposal has somehow failed and it is still being renewed?

If it's the former, it sounds like it's probably a bug in your code, and having the lock stop renewing would just be masking the issue at best (you would still have a thread stuck somewhere doing... something). If it's not something fixable you could at least mitigate it by using something like a timed cancellation token (mentioned in an earlier comment) to properly abort whatever action is happening rather than just cancelling the lock renewal.

If it's the latter it sounds like a bug in RedLock, and if that's the case I'd like to get to the bottom of it.

[duraes-fd]

No, i have a using block.

I will try to get some working example on a separate git repo to link to you so you can get a proper look at it.

[SanGreen74]

No, i have a using block.

I will try to get some working example on a separate git repo to link to you so you can get a proper look at it.

We have the same issue in our project with endless locks. We use version 2.3.2 and we also have using in our code which creates RedLock instance. Were you able to reproduce the problem or find its cause?

[duraes-fd]

We have the same issue in our project with endless locks. We use version 2.3.2 and we also have using in our code which creates RedLock instance. Were you able to reproduce the problem or find its cause?

Some priorities changed and I never got back to this.
Unfortunately, I don't have a prediction on when I will be able to set time aside to prepare a working example.

Sorry for the late reply, I was on vacation.

[fabio-s-franco]

I had similar issues in the past and the only fix was to restart the container to get rid of orphaned locks.

I found that due to a lack of attention, I didn't await before the using. And that's quite critical because it is not an IDisposable we get as the result of the using (when using CreateAsync), but instead an IAsyncDisposable.

My assumption is that this caused a fire and forget behavior which would have the DisposeAsync be called only during GC (with luck if its generation is 0 and has no dangling references to it).

@samcook if this turns out to be the case for more people, perhaps put a bit of emphasis in the readme example about this. It is a very easy mistake to make.

[samcook]

And that's quite critical because it is not an IDisposable we get as the result of the using (when using CreateAsync), but instead an IAsyncDisposable.

That's not quite true. The RedLock object implements both IDisposable and IAsyncDisposable so, whether you create it using Create or CreateAsync, it should still be correctly disposed as the using block ends (either synchronously with Dispose if you're just using, or asynchronously with DisposeAsync if you await using).

Obviously the latter is preferable if you're writing async code, and maybe there's an issue with sync dispose in otherwise async code (perhaps some threads are getting deadlocked). It would be good to get a reliable repro of this.

[fabio-s-franco]

@samcook , maybe I was not very clear on what I meant. If you use CreateLock, you get IDisposable:

If you use CreatLockAsync, you get IDisposableAsync:

If instead of:

await using (var lockuse2 = await fac.CreateLockAsync(number.ToString(), TimeSpan.FromDays(1)))
{
    Console.WriteLine(number);
};

I write this:

using (var lockuse2 = await fac.CreateLockAsync(number.ToString(), TimeSpan.FromDays(1)))
{
    Console.WriteLine(number);
};

I will get into trouble. Because in my case the method would return before the lock is released. That somehow within the lifecycle of the aspnet request would cause the lock to renew itself indefinitely. I think the difficulty to reproduce this is because a lot can happen depending on many external factors, including the nature of the application itself.

I know the await fixed. But didn't get down to the root cause. I know there are some particularities OnActionExcuting and OnActionExecutingAsync that I was going for at the time. But I honestly can't remember the details. I do know the "await" before the using is quite important, because as is documented (sorry, don't have a reference right now). Whatever is still executing triggered by a controller action after it finishes does not have its completion warranted. Quite sure I read this on ms docs somewhere and my best assumptiuon is that it created an orphan that kept renewing the lock.

[samcook]

Both CreateLock and CreateLockAsync return an instance of IRedLock (see here).

And IRedLock is both IDisposable and IAsyncDisposable (see here).

The creation of the lock object and the disposal of the lock object are two separate operations, and each of them can be invoked either synchronously or asynchronously. You could even call them separately if you want to (or without using a using block at all).

e.g. (none of these are advisable, but just showing you can do it)

var redLock = await fac.CreateLockAsync("foo", TimeSpan.FromSeconds(30)); // create lock async
using (redLock) // calls Dispose at end of block
{
    if (redLock.IsAcquired) { ... }
}

or

var redLock = fac.CreateLock("foo", TimeSpan.FromSeconds(30)); // create lock sync
await using (redLock) // calls DisposeAsync at end of block
{
    if (redLock.IsAcquired) { ... }
}

That said, I don't doubt that there could be an issue here, as multiple people have reported, but so far without a repro it's difficult to diagnose exactly what the problem is.

[fabio-s-franco]

@samcook you're completely right and I didn't understand what you said. It's been a while so some of it is a bit foggy. But I am still convinced that it is related to orphaned locks and is related to async workflows. I will be working with it soon again and may have more info to help debug this.

[userAXC001]

Following this thread... It sounds related to the issue I posted here #107 . I was able to look up the key in redis using the TTL <key> command and it looks like the key kept getting extended indefinitely.

I'm using the c# 8 using declaration syntax. i.e no curly braces. Though I can't imagine this to be the issue.

await using var redLock = await fac.CreateLockAsync(number.ToString(), TimeSpan.FromSeconds(30))
//Do stuff

I've also tried to dispose manually in a try/finally

try
{
   var redLock = await fac.CreateLockAsync(number.ToString(), TimeSpan.FromSeconds(30))
}
finally
{
   redLock?.Dispose
}

At this point, it would seem related to "orphaned locks". Similar to @fabio-s-franco , the issue is only fixed after I restart my services. Unfortunately, I haven't been able to get this replicated locally

[fabio-s-franco]

@samcook I was checking the implementation of dispose and the different flavors of Timer, whic led me to:

TimerCallback Delegate Reference

Callbacks can occur after the Dispose() method overload has been called, because the timer queues callbacks for execution by thread pool threads. You can use the Dispose(WaitHandle) method overload to wait until all callbacks have completed.

I checked that ExtendLockLifetime, doesn't check for isDisposed and dispose is not using the waithandle.

I am not sure what side effects could be the result of this, but maybe worth a look.