Redis semaphore: current count #250
Conversation
|
@madelson can you review this PR for the semaphore count? |
madelson
left a comment
There was a problem hiding this comment.
This looks great @teesofttech thanks for the contribution.
I left a few comments with minor changes I'd like to see before merging it in. If you have a chance to go through those leave a comment and @mention me to take the next steps.
| public int GetCurrentCount() | ||
| { | ||
| return GetCurrentCountAsync().GetAwaiter().GetResult(); | ||
| } |
There was a problem hiding this comment.
Should be:
public int GetCurrentCount() => SyncViaAsync.Run(s => s.GetCurrentCountAsync(), state: this);
This avoids the sync over async pattern that GetResult() is, which is inefficient and can result in thread starvation.
| /// </summary> | ||
| public string Name => this.Key.ToString(); | ||
|
|
||
| public int GetCurrentCount() |
There was a problem hiding this comment.
These are public methods; please add doc comments to each. In this case I think we can just say that this functionality is equivalent to SemaphoreSlim.CurrentCount
|
|
||
| public async ValueTask<int> GetCurrentCountAsync() | ||
| { | ||
| var database = _databases[0]; |
There was a problem hiding this comment.
this._databases, per the codebase style
| /// <returns></returns> | ||
| internal async ValueTask<int> GetCurrentCountAsync(IDatabaseAsync database) | ||
| { | ||
| const string script = @"return redis.call('zcard', KEYS[1])"; |
There was a problem hiding this comment.
Using a script here adds unnecessary overhead since we're just executing a single command. Also, we want to support sync-via-async execution.
Let's just do database.SortedSetLengthAsync() with CommandFlags.DemandMaster.
To support sync-via-async execution, we should actually do:
var length = SyncViaAsync.IsSynchronous
? database.SortedSetLength(...)
: await database.SortedSetLengthAsync(...).ConfigureAwait(false);
| /// </summary> | ||
| /// <param name="database"></param> | ||
| /// <returns></returns> | ||
| internal async ValueTask<int> GetCurrentCountAsync(IDatabaseAsync database) |
| /// </summary> | ||
| /// <param name="database"></param> | ||
| /// <returns></returns> | ||
| internal async ValueTask<int> GetCurrentCountAsync(IDatabaseAsync database) |
There was a problem hiding this comment.
Let's call this GetAcquiredCountAsync since it doesn't return the remaining count.
| /// Get Current Count | ||
| /// </summary> | ||
| /// <param name="database"></param> | ||
| /// <returns></returns> |
There was a problem hiding this comment.
Let's remove this doc comment; it isn't adding anything
| var database = _databases[0]; | ||
| var primitive = new RedisSemaphorePrimitive(this.Key, this.MaxCount, this._options.RedLockTimeouts); | ||
| var acquiredCount = await primitive.GetCurrentCountAsync(database).ConfigureAwait(false); | ||
| return this.MaxCount - acquiredCount; |
There was a problem hiding this comment.
Let's wrap this with Math.Max(0, ...). Because semaphores can be declared with different counts on different machines (not a good thing, but it could happen), it's possible that acquiredCount could be greater than MaxCount. However, CurrentCount shouldn't return a negative number in that case.
|
|
||
| expectedAvailable.ShouldEqual(available); | ||
| databaseMock.VerifyAll(); | ||
| } |
There was a problem hiding this comment.
Let's add one additional test case that demonstrates this working end-to-end.
See RedisDistributedLockTest.TestCanAcquireLockWhenCurrentCultureIsTurkishTurkey() as an example of how you can get a database inside the test case.
This end-to-end test would not have the "CI" tag.
|
@madelson Have addressed those comments. Can you take a look at it? |
|
@madelson the build still failed. Checking the logs history, seems it has been failing before |
|
@madelson the build still failed. Checking the log history, seems it has been failing before |
Ok; I'll take a look after merging. |
#234