Cannot create a RedisDistributedSemaphore with extensionCadence set to Timeout.InfiniteTimeSpan

[LinkForce]

I am trying to instantiate a RedisDistributedSemaphore that doesn't auto extends the locks, and according to the source code, I should set extensionCadence set Timeout.InfiniteTimeSpan. But when the RedisDistributedSyncronizationOptions is built, I get an ArgumentOutOfRangeException stating that "extensionCadence must be less than expiry". I believe this is not right, as Timeout.InfiniteTimeSpan is less than the default expiry.

[madelson]

@LinkForce thanks for reporting. This does seem like a bug. Out of curiosity why do you want to disable auto-extension?

[LinkForce]

I want to have a Semaphore to limit some operation to the maximum of 25 operations per second, but the thing is it does not matter if the operation finishes or not, I just need to limit that no more than 25 operations starts in the same second window.
So my idea was to make the semaphore never auto extend and set the expiry to one second, this way I can aquire the lock on the code and it will automatically expire after one second.

[madelson]

I see thanks for the context. The way the library works it will auto-release the semaphore if the handle gets garbage collected. So the way I’d recommend doing this is:

var handle = await semaphore.AcquireAsync();
Task.Delay(timeout).ContinueWith(() => handle.DisposeAsync());

You should still set the overall timeout like you’re doing, but you can skip disabling extension.

Just to be 100% clear:

• If you follow the pattern shown above, the only role that the underlying semaphore timeout plays is if the system crashes or we lose connection to Redis
• This issue is still a bug (especially since you are doing exactly what the error message says you should do).
• Even when this issue is fixed you should still probably use the pattern above because it protects the handle from GC up until the intended timeout.

[Skier23]

@madelson Continuing from the other issue:
I do agree that just using the using pattern is the better pattern. However, I’m using this library in an industrial application for a backend library that is being used by a bunch of other teams already that use the expiration logic so being able to have an option to keep the lock expiring is a win for backwards compatibility.

[madelson]

@Skier23 ok. Just want to confirm that you're aware that if you let the handle be GC'd it will be released before the expiry. E.g. you can't do something like this:

await this._semaphore.AcquireAsync(); // discard result
// assume semaphore is held here... but it might not be due to GC!

I'm curious what coding pattern you are planning on using and how you expect it to behave. For example I could imagine doing something like this:

await using (var handle = this._semaphore.AcquireAsync())
{
    // do stuff here; semaphore is held but allowed to expire
} // here we will release; which will no-op if we've already timed out

[Skier23]

yes. I have teams that are doing something like
var lock = ...acquireasync(10)
//assume lock will last 10 seconds and know that their logic will take less than 10seconds
//do logic

I know this isn't good practice and I'm trying to get them away from doing it like this but that's how some teams are doing it now.

[madelson]

@Skier23 is the code you show using the DistributedLock library already or some other implementation? If you're using the DistributedLock library like this it isn't just bad practice, you might have a bug because if the handle becomes eligible for GC (which could happen any time once there are no more references to it

[Skier23]

Its currently using another implementation.

[madelson]

I finally had some time to look into this in more depth.

I'm coming around to the opinion that the thing I want to change here is the misleading error message that implies that one should be allowed to set ExtensionCadence to infinite. This error message appears to have been copied from AzureBlobLeaseOptionsBuilder, which does have a use-case for disabling extension because the underlying provider supports infinite lease durations. There are a couple indicators that the current behavior is what's intended:

• We have a test for it in RedisDistributedSynchronizationOptionsBuilderTest
• The doc comments on the ExtensionCadence setter do not document infinite as an option.

In order to support disabling extension, we'd need to actually do quite a bit more than just change the one check. We'd have to:

• Update RedLockHandle's implementation of ILeaseMonitor to handle this case in both MonitoringCadence and RenewOrValidateLeaseAsync.
• To support handle-loss-checking, we'd need to implement an alternative check-but-don't renew script for each of the primitives (mutex, rwlock, and semaphore).
• Add test coverage for this alternate flow

This wouldn't be that big a deal if the use-case was really strong, but I still don't feel like I see it.

For @LinkForce's use-case, the error actually stopped them from accidentally writing broken code. If this change were implemented, the code would have failed to do the right thing in a silent and subtle way rather than failing fast.

For @Skier23's use-case it feels like we're just trying to support a known-to-be-bad pattern, when the default behavior of this library makes it trivial to support a very robust pattern instead.

Furthermore, even as-written the library does indirectly support the pattern of using the semaphore as a time-based rate-limiter rather than a true semaphore. This can be done simply with:

var handle = await sempahore.AquireAsync();
Task.Delay(timeout).ContinueWith(() => handle.Dispose());

This can easily be wrapped up in a utility method.

If this use-case of "distributed rate-limiter" is a popular one, I wonder if it merits consideration for a dedicated API in the library.? For example, we could have something like this:

class RedisDistributedRateLimiter
{
    public RedisDistributedRateLimiter(string name, int maxCount, TimeSpan period);

    // WaitAsync returns only maxCount times/period
    public async Task WaitAsync(CancellationToken);
}

Thoughts? I really appreciate all the engagement here!

[LinkForce]

Yes, I agree that it would be better to fix the error message instead of allowing Timeout.InfiniteTimeSpan.
For my use I did like your suggestion of setting up a Task, and I think it is clearer that way because if I was able to set the extensionCadence and rely on the expiration, the handle Dispose method would have to change its behavior to not release the lock based on those input parameters, and I can see this getting confusing and generating unexpected behavior.