Skip to content

Commit

Permalink
Fix queue count in rate limiters (dotnet#90810) (dotnet#97041)
Browse files Browse the repository at this point in the history
  • Loading branch information
@BrennanConroy @matouskozak
BrennanConroy authored and matouskozak committed Apr 30, 2024
1 parent aa2d20d commit f93c954
Show file tree
Hide file tree
Showing 4 changed files with 136 additions and 22 deletions.
40 changes: 34 additions & 6 deletions ...ies/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,17 @@ protected override ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, C
Debug.Assert(_queueCount >= 0);
if (!oldestRequest.TrySetResult(FailedLease))
{
// Updating queue count is handled by the cancellation code
_queueCount += oldestRequest.Count;
if (!oldestRequest.QueueCountModified)
{
// We already updated the queue count, the Cancel code is about to run or running and waiting on our lock,
// tell Cancel not to do anything
oldestRequest.QueueCountModified = true;
}
else
{
// Updating queue count was handled by the cancellation code, don't double count
_queueCount += oldestRequest.Count;
}
}
else
{
Expand Down Expand Up @@ -277,10 +286,19 @@ private void Release(int releaseCount)
// Check if request was canceled
if (!nextPendingRequest.TrySetResult(lease))
{
// Queued item was canceled so add count back
// Queued item was canceled so add count back, permits weren't acquired
_permitCount += nextPendingRequest.Count;
// Updating queue count is handled by the cancellation code
_queueCount += nextPendingRequest.Count;
if (!nextPendingRequest.QueueCountModified)
{
// We already updated the queue count, the Cancel code is about to run or running and waiting on our lock,
// tell Cancel not to do anything
nextPendingRequest.QueueCountModified = true;
}
else
{
// Updating queue count was handled by the cancellation code, don't double count
_queueCount += nextPendingRequest.Count;
}
}
else
{
Expand Down Expand Up @@ -399,6 +417,9 @@ private sealed class RequestRegistration : TaskCompletionSource<RateLimitLease>
private readonly CancellationToken _cancellationToken;
private CancellationTokenRegistration _cancellationTokenRegistration;

// Update under the limiter lock and only if the queue count was updated by the calling code
public bool QueueCountModified { get; set; }

// this field is used only by the disposal mechanics and never shared between threads
private RequestRegistration? _next;

Expand Down Expand Up @@ -429,7 +450,14 @@ private static void Cancel(object? state)
var limiter = (ConcurrencyLimiter)registration.Task.AsyncState!;
lock (limiter.Lock)
{
limiter._queueCount -= registration.Count;
// Queuing and replenishing code might modify the _queueCount, since there is no guarantee of when the cancellation
// code runs and we only want to update the _queueCount once, we set a bool (under a lock) so either method
// can update the count and not double count.
if (!registration.QueueCountModified)
{
limiter._queueCount -= registration.Count;
registration.QueueCountModified = true;
}
}
}
}
Expand Down
39 changes: 34 additions & 5 deletions ...System.Threading.RateLimiting/src/System/Threading/RateLimiting/FixedWindowRateLimiter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,17 @@ protected override ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, C
Debug.Assert(_queueCount >= 0);
if (!oldestRequest.TrySetResult(FailedLease))
{
_queueCount += oldestRequest.Count;
if (!oldestRequest.QueueCountModified)
{
// We already updated the queue count, the Cancel code is about to run or running and waiting on our lock,
// tell Cancel not to do anything
oldestRequest.QueueCountModified = true;
}
else
{
// Updating queue count was handled by the cancellation code, don't double count
_queueCount += oldestRequest.Count;
}
}
else
{
Expand Down Expand Up @@ -330,10 +340,19 @@ private void ReplenishInternal(long nowTicks)

if (!nextPendingRequest.TrySetResult(SuccessfulLease))
{
// Queued item was canceled so add count back
// Queued item was canceled so add count back, permits weren't acquired
_permitCount += nextPendingRequest.Count;
// Updating queue count is handled by the cancellation code
_queueCount += nextPendingRequest.Count;
if (!nextPendingRequest.QueueCountModified)
{
// We already updated the queue count, the Cancel code is about to run or running and waiting on our lock,
// tell Cancel not to do anything
nextPendingRequest.QueueCountModified = true;
}
else
{
// Updating queue count was handled by the cancellation code, don't double count
_queueCount += nextPendingRequest.Count;
}
}
else
{
Expand Down Expand Up @@ -435,6 +454,9 @@ private sealed class RequestRegistration : TaskCompletionSource<RateLimitLease>
private readonly CancellationToken _cancellationToken;
private CancellationTokenRegistration _cancellationTokenRegistration;

// Update under the limiter lock and only if the queue count was updated by the calling code
public bool QueueCountModified { get; set; }

// this field is used only by the disposal mechanics and never shared between threads
private RequestRegistration? _next;

Expand Down Expand Up @@ -465,7 +487,14 @@ private static void Cancel(object? state)
var limiter = (FixedWindowRateLimiter)registration.Task.AsyncState!;
lock (limiter.Lock)
{
limiter._queueCount -= registration.Count;
// Queuing and replenishing code might modify the _queueCount, since there is no guarantee of when the cancellation
// code runs and we only want to update the _queueCount once, we set a bool (under a lock) so either method
// can update the count and not double count.
if (!registration.QueueCountModified)
{
limiter._queueCount -= registration.Count;
registration.QueueCountModified = true;
}
}
}
}
Expand Down
39 changes: 34 additions & 5 deletions ...stem.Threading.RateLimiting/src/System/Threading/RateLimiting/SlidingWindowRateLimiter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,17 @@ protected override ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, C
Debug.Assert(_queueCount >= 0);
if (!oldestRequest.TrySetResult(FailedLease))
{
_queueCount += oldestRequest.Count;
if (!oldestRequest.QueueCountModified)
{
// We already updated the queue count, the Cancel code is about to run or running and waiting on our lock,
// tell Cancel not to do anything
oldestRequest.QueueCountModified = true;
}
else
{
// Updating queue count was handled by the cancellation code, don't double count
_queueCount += oldestRequest.Count;
}
}
else
{
Expand Down Expand Up @@ -342,11 +352,20 @@ private void ReplenishInternal(long nowTicks)

if (!nextPendingRequest.TrySetResult(SuccessfulLease))
{
// Queued item was canceled so add count back
// Queued item was canceled so add count back, permits weren't acquired
_permitCount += nextPendingRequest.Count;
_requestsPerSegment[_currentSegmentIndex] -= nextPendingRequest.Count;
// Updating queue count is handled by the cancellation code
_queueCount += nextPendingRequest.Count;
if (!nextPendingRequest.QueueCountModified)
{
// We already updated the queue count, the Cancel code is about to run or running and waiting on our lock,
// tell Cancel not to do anything
nextPendingRequest.QueueCountModified = true;
}
else
{
// Updating queue count was handled by the cancellation code, don't double count
_queueCount += nextPendingRequest.Count;
}
}
else
{
Expand Down Expand Up @@ -448,6 +467,9 @@ private sealed class RequestRegistration : TaskCompletionSource<RateLimitLease>
private readonly CancellationToken _cancellationToken;
private CancellationTokenRegistration _cancellationTokenRegistration;

// Update under the limiter lock and only if the queue count was updated by the calling code
public bool QueueCountModified { get; set; }

// this field is used only by the disposal mechanics and never shared between threads
private RequestRegistration? _next;

Expand Down Expand Up @@ -478,7 +500,14 @@ private static void Cancel(object? state)
var limiter = (SlidingWindowRateLimiter)registration.Task.AsyncState!;
lock (limiter.Lock)
{
limiter._queueCount -= registration.Count;
// Queuing and replenishing code might modify the _queueCount, since there is no guarantee of when the cancellation
// code runs and we only want to update the _queueCount once, we set a bool (under a lock) so either method
// can update the count and not double count.
if (!registration.QueueCountModified)
{
limiter._queueCount -= registration.Count;
registration.QueueCountModified = true;
}
}
}
}
Expand Down
40 changes: 34 additions & 6 deletions ...System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,17 @@ protected override ValueTask<RateLimitLease> AcquireAsyncCore(int tokenCount, Ca
Debug.Assert(_queueCount >= 0);
if (!oldestRequest.TrySetResult(FailedLease))
{
// Updating queue count is handled by the cancellation code
_queueCount += oldestRequest.Count;
if (!oldestRequest.QueueCountModified)
{
// We already updated the queue count, the Cancel code is about to run or running and waiting on our lock,
// tell Cancel not to do anything
oldestRequest.QueueCountModified = true;
}
else
{
// Updating queue count was handled by the cancellation code, don't double count
_queueCount += oldestRequest.Count;
}
}
else
{
Expand Down Expand Up @@ -345,10 +354,19 @@ private void ReplenishInternal(long nowTicks)

if (!nextPendingRequest.TrySetResult(SuccessfulLease))
{
// Queued item was canceled so add count back
// Queued item was canceled so add count back, permits weren't acquired
_tokenCount += nextPendingRequest.Count;
// Updating queue count is handled by the cancellation code
_queueCount += nextPendingRequest.Count;
if (!nextPendingRequest.QueueCountModified)
{
// We already updated the queue count, the Cancel code is about to run or running and waiting on our lock,
// tell Cancel not to do anything
nextPendingRequest.QueueCountModified = true;
}
else
{
// Updating queue count was handled by the cancellation code, don't double count
_queueCount += nextPendingRequest.Count;
}
}
else
{
Expand Down Expand Up @@ -450,6 +468,9 @@ private sealed class RequestRegistration : TaskCompletionSource<RateLimitLease>
private readonly CancellationToken _cancellationToken;
private CancellationTokenRegistration _cancellationTokenRegistration;

// Update under the limiter lock and only if the queue count was updated by the calling code
public bool QueueCountModified { get; set; }

// this field is used only by the disposal mechanics and never shared between threads
private RequestRegistration? _next;

Expand Down Expand Up @@ -480,7 +501,14 @@ private static void Cancel(object? state)
var limiter = (TokenBucketRateLimiter)registration.Task.AsyncState!;
lock (limiter.Lock)
{
limiter._queueCount -= registration.Count;
// Queuing and replenishing code might modify the _queueCount, since there is no guarantee of when the cancellation
// code runs and we only want to update the _queueCount once, we set a bool (under a lock) so either method
// can update the count and not double count.
if (!registration.QueueCountModified)
{
limiter._queueCount -= registration.Count;
registration.QueueCountModified = true;
}
}
}
}
Expand Down

0 comments on commit f93c954

Please sign in to comment.