Skip to content

Commit

Permalink
Revert "Fix queue count in rate limiters (#90810)"
Browse files Browse the repository at this point in the history 
This reverts commit 1b9e4e3.
  • Loading branch information
@jkoritzinsky
jkoritzinsky committed Aug 22, 2023
1 parent b478aba commit f76a436
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 136 deletions.
40 changes: 6 additions & 34 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,17 +156,8 @@ protected override ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, C
Debug.Assert(_queueCount >= 0);
if (!oldestRequest.TrySetResult(FailedLease))
{
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;
}
// Updating queue count is handled by the cancellation code
_queueCount += oldestRequest.Count;
}
else
{
Expand Down Expand Up @@ -286,19 +277,10 @@ private void Release(int releaseCount)
// Check if request was canceled
if (!nextPendingRequest.TrySetResult(lease))
{
// Queued item was canceled so add count back, permits weren't acquired
// Queued item was canceled so add count back
_permitCount += 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;
}
// Updating queue count is handled by the cancellation code
_queueCount += nextPendingRequest.Count;
}
else
{
Expand Down Expand Up @@ -417,9 +399,6 @@ 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 @@ -450,14 +429,7 @@ private static void Cancel(object? state)
var limiter = (ConcurrencyLimiter)registration.Task.AsyncState!;
lock (limiter.Lock)
{
// 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;
}
limiter._queueCount -= registration.Count;
}
}
}
Expand Down
39 changes: 5 additions & 34 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,17 +173,7 @@ protected override ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, C
Debug.Assert(_queueCount >= 0);
if (!oldestRequest.TrySetResult(FailedLease))
{
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;
}
_queueCount += oldestRequest.Count;
}
else
{
Expand Down Expand Up @@ -340,19 +330,10 @@ private void ReplenishInternal(long nowTicks)

if (!nextPendingRequest.TrySetResult(SuccessfulLease))
{
// Queued item was canceled so add count back, permits weren't acquired
// Queued item was canceled so add count back
_permitCount += 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;
}
// Updating queue count is handled by the cancellation code
_queueCount += nextPendingRequest.Count;
}
else
{
Expand Down Expand Up @@ -454,9 +435,6 @@ 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 @@ -487,14 +465,7 @@ private static void Cancel(object? state)
var limiter = (FixedWindowRateLimiter)registration.Task.AsyncState!;
lock (limiter.Lock)
{
// 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;
}
limiter._queueCount -= registration.Count;
}
}
}
Expand Down
39 changes: 5 additions & 34 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,17 +185,7 @@ protected override ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, C
Debug.Assert(_queueCount >= 0);
if (!oldestRequest.TrySetResult(FailedLease))
{
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;
}
_queueCount += oldestRequest.Count;
}
else
{
Expand Down Expand Up @@ -352,20 +342,11 @@ private void ReplenishInternal(long nowTicks)

if (!nextPendingRequest.TrySetResult(SuccessfulLease))
{
// Queued item was canceled so add count back, permits weren't acquired
// Queued item was canceled so add count back
_permitCount += nextPendingRequest.Count;
_requestsPerSegment[_currentSegmentIndex] -= 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;
}
// Updating queue count is handled by the cancellation code
_queueCount += nextPendingRequest.Count;
}
else
{
Expand Down Expand Up @@ -467,9 +448,6 @@ 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 @@ -500,14 +478,7 @@ private static void Cancel(object? state)
var limiter = (SlidingWindowRateLimiter)registration.Task.AsyncState!;
lock (limiter.Lock)
{
// 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;
}
limiter._queueCount -= registration.Count;
}
}
}
Expand Down
40 changes: 6 additions & 34 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,17 +178,8 @@ protected override ValueTask<RateLimitLease> AcquireAsyncCore(int tokenCount, Ca
Debug.Assert(_queueCount >= 0);
if (!oldestRequest.TrySetResult(FailedLease))
{
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;
}
// Updating queue count is handled by the cancellation code
_queueCount += oldestRequest.Count;
}
else
{
Expand Down Expand Up @@ -354,19 +345,10 @@ private void ReplenishInternal(long nowTicks)

if (!nextPendingRequest.TrySetResult(SuccessfulLease))
{
// Queued item was canceled so add count back, permits weren't acquired
// Queued item was canceled so add count back
_tokenCount += 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;
}
// Updating queue count is handled by the cancellation code
_queueCount += nextPendingRequest.Count;
}
else
{
Expand Down Expand Up @@ -468,9 +450,6 @@ 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 @@ -501,14 +480,7 @@ private static void Cancel(object? state)
var limiter = (TokenBucketRateLimiter)registration.Task.AsyncState!;
lock (limiter.Lock)
{
// 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;
}
limiter._queueCount -= registration.Count;
}
}
}
Expand Down

0 comments on commit f76a436

Please sign in to comment.