-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix Lease release/acquire operation logic #1289
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,11 @@ | |
// ----------------------------------------------------------------------- | ||
|
||
using System; | ||
using System.Linq; | ||
using System.Runtime.ExceptionServices; | ||
using System.Text; | ||
using System.Text.RegularExpressions; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Akka.Actor; | ||
using Akka.Configuration; | ||
|
@@ -45,6 +48,8 @@ private static string MakeDns1039Compatible(string name) | |
private readonly TimeSpan _timeout; | ||
private readonly string _leaseName; | ||
private readonly IActorRef _leaseActor; | ||
private readonly object _acquireLock = new (); | ||
private Task<bool>? _acquireTask; | ||
|
||
public AzureLease(LeaseSettings settings, ExtendedActorSystem system) : | ||
this(system, new AtomicBoolean(), settings) | ||
|
@@ -92,12 +97,18 @@ public override async Task<bool> Release() | |
if(_log.IsDebugEnabled) | ||
_log.Debug("Releasing lease"); | ||
var result = await _leaseActor.Ask(LeaseActor.Release.Instance, _timeout); | ||
return result switch | ||
switch (result) | ||
{ | ||
LeaseActor.LeaseReleased _ => true, | ||
LeaseActor.InvalidRequest req => throw new LeaseException(req.Reason), | ||
_ => throw new LeaseException($"Unexpected response type: {result.GetType()}") | ||
}; | ||
case LeaseActor.LeaseReleased: | ||
return true; | ||
case LeaseActor.InvalidReleaseRequest: | ||
_log.Info("Tried to release a lease that is not acquired"); | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logically, since we didn't have the lease, we should return true. |
||
case Status.Failure f: | ||
throw new LeaseException($"Failure while releasing lease: {f.Cause.Message}", f.Cause); | ||
default: | ||
throw new LeaseException($"Unexpected response type: {result.GetType()}"); | ||
} | ||
} | ||
catch (AskTimeoutException) | ||
{ | ||
|
@@ -109,25 +120,59 @@ public override async Task<bool> Release() | |
public override Task<bool> Acquire() | ||
=> Acquire(null); | ||
|
||
public override async Task<bool> Acquire(Action<Exception?>? leaseLostCallback) | ||
public override Task<bool> Acquire(Action<Exception?>? leaseLostCallback) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To make this idempotent, we could not use async...await pattern |
||
{ | ||
try | ||
lock (_acquireLock) | ||
{ | ||
if (_acquireTask is not null) | ||
{ | ||
if(_log.IsDebugEnabled) | ||
_log.Debug("Lease is already being acquired"); | ||
return _acquireTask; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM |
||
} | ||
|
||
if(_log.IsDebugEnabled) | ||
_log.Debug("Acquiring lease"); | ||
var result = await _leaseActor.Ask(new LeaseActor.Acquire(leaseLostCallback), _timeout); | ||
return result switch | ||
{ | ||
LeaseActor.LeaseAcquired _ => true, | ||
LeaseActor.LeaseTaken _ => false, | ||
LeaseActor.InvalidRequest req => throw new LeaseException(req.Reason), | ||
_ => throw new LeaseException($"Unexpected response type: {result.GetType()}") | ||
}; | ||
} | ||
catch (AskTimeoutException) | ||
{ | ||
throw new LeaseTimeoutException( | ||
$"Timed out trying to acquire lease [{_leaseName}, {_settings.OwnerName}]. It may still be taken."); | ||
_acquireTask = _leaseActor.Ask(new LeaseActor.Acquire(leaseLostCallback), _timeout) | ||
.ContinueWith(t => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM |
||
{ | ||
if (t.IsFaulted) | ||
{ | ||
if (t.Exception is { }) | ||
{ | ||
var flattened = t.Exception.Flatten(); | ||
if (flattened.InnerExceptions.Count > 0 | ||
&& flattened.InnerExceptions.Any(e => e is AskTimeoutException)) | ||
{ | ||
throw new LeaseTimeoutException( | ||
$"Timed out trying to acquire lease [{_leaseName}, {_settings.OwnerName}]. It may still be taken.", | ||
t.Exception); | ||
} | ||
} | ||
|
||
throw new LeaseException( | ||
$"Faulted trying to acquire lease [{_leaseName}, {_settings.OwnerName}]. It may still be taken.", | ||
t.Exception); | ||
} | ||
|
||
// For completeness, we're not using cancellation token | ||
if (t.IsCanceled) | ||
{ | ||
throw new LeaseException( | ||
$"Canceled while trying to acquire lease [{_leaseName}, {_settings.OwnerName}]. It may still be taken.", | ||
t.Exception); | ||
} | ||
|
||
return t.Result switch | ||
{ | ||
LeaseActor.LeaseAcquired => true, | ||
LeaseActor.LeaseTaken => false, | ||
Status.Failure f => throw new LeaseException($"Failure while acquiring lease: {f.Cause.Message}", f.Cause), | ||
_ => throw new LeaseException($"Unexpected response type: {t.Result.GetType()}") | ||
}; | ||
}); | ||
|
||
return _acquireTask; | ||
} | ||
} | ||
|
||
|
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'd mark this as
DEBUG
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.
it needs to be
INFO
because if its not, we could not debug if a lease was "released" because it was actually released or because we didn't have it in the first place