-
Notifications
You must be signed in to change notification settings - Fork 221
Composite locks #248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Composite locks #248
Conversation
…istributed synchronization handles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @moeen ! This is off to a great start!
Func<TProvider, string, TimeSpan, CancellationToken, ValueTask<IDistributedSynchronizationHandle?>> acquireFunc, | ||
IReadOnlyList<string> names, | ||
TimeSpan timeout = default, | ||
CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this method is fundamentally internal, does it need default values for these parameters? If not let's remove
src/DistributedLock.Core/DistributedUpgradeableReaderWriterLockProviderExtensions.cs
Show resolved
Hide resolved
src/DistributedLock.Core/DistributedUpgradeableReaderWriterLockProviderExtensions.cs
Show resolved
Hide resolved
ValidateAcquireParameters(provider, acquireFunc, names); | ||
|
||
var timeoutTracker = new TimeoutTracker(timeout); | ||
var handles = new List<IDistributedSynchronizationHandle>(names.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this flow works, I found it to be a bit hard to follow since you need to keep in mind that the handle constructor does a defensive copy of the list which makes clear ok and that the fact that we always dispose in the finally is made ok by the clear().
Let's make this small change which I think makes the intent clearer:
IDistributedSynchronizationHandle? handle = null;
try {
foreach (var name in name)
{
var handle = ...
if (handle is null) { break; }
handles.Add(handle);
...
}
}
finally {
if (handle is null) { await DisposeAsyncHandles(handles).ConfigureAwait(false); }
}
return handle;
Let's do the same for the sync codepath
await DisposeHandlesAsync(handles).ConfigureAwait(false); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra blank line
: throw new TimeoutException($"Failed to acquire lock for '{n}'"); | ||
}; | ||
|
||
private sealed class TimeoutTracker(TimeSpan timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this a struct
: throw new TimeoutException($"Failed to acquire lock for '{n}'"); | ||
}; | ||
|
||
private sealed class TimeoutTracker(TimeSpan timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take in TimeoutValue
since this will automatically validate for us that the TimeSpan is a valid timeout value. This is important because invalid values like -2 will otherwise get converted into valid values by Remaining.
|
||
public TimeSpan Remaining => this._stopwatch is null | ||
? Timeout.InfiniteTimeSpan | ||
: timeout - this._stopwatch.Elapsed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug: if remaining becomes negative then it will become an invalid timeout or even Timeout.InfiniteTimespan!
Should be:
Remaining =>
this._stopwatch is { Elapsed: var elapsed }
? elapsed >= timeout
? TimeSpan.Zero
: timeout - elapsed
: Timeout.InfiniteTimespan;
/// </summary> | ||
{{interfaceName}} {{createMethodName}}(string name{{(interfaceName.Contains("Semaphore") ? ", int maxCount" : string.Empty)}}); | ||
} | ||
"""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the use of """ strings here, but let's indent less:
var providerInterfaceCode = $$"""
...
""";
Please address throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead and add unit tests for all of these new methods in DistributedLockProviderExtensionsTest
.
In particular:
- Let's cover all input validation/edge-case handling
- Let's cover the case where you go to acquire A, B with timeout 0 and B is already acquired -> you should get back null and A should be left in an unacquired state
- Let's test the "remaining time" logic. Might be easiest to test through mocking.
This pull request introduces new composite lock methods for various lock types via code generation.
Auto Generated Codes
src/DistributedLock.Core/DistributedLockProviderExtensions.cs
src/DistributedLock.Core/DistributedReaderWriterLockProviderExtensions.cs
src/DistributedLock.Core/DistributedSemaphoreProviderExtensions.cs
src/DistributedLock.Core/DistributedUpgradeableReaderWriterLockProviderExtensions.cs