-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Missing disposed cancellation token registration after setting result on Rate Limiting #76783
Comments
Tagging subscribers to this area: @mangod9 Issue Details
private void ReplenishInternal(long nowTicks)
{
...
nextPendingRequest = _options.QueueProcessingOrder == QueueProcessingOrder.OldestFirst
? _queue.DequeueHead()
: _queue.DequeueTail();
if (!nextPendingRequest.Tcs.TrySetResult(SuccessfulLease)){ ... }
else { ... }
nextPendingRequest.CancellationTokenRegistration.Dispose();
} The limiters should follow the same behavior in the other places, such as when they try to dequeue old requests in order to free some space for pushing new requests on the queue with the newest first processing order policy. protected override ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, CancellationToken cancellationToken = default)
{
...
if (_options.QueueProcessingOrder == QueueProcessingOrder.NewestFirst && permitCount <= _options.QueueLimit){
RequestRegistration oldestRequest = _queue.DequeueHead();
if (!oldestRequest.Tcs.TrySetResult(FailedLease)){ ... }
else { ... }
// Mising dispose
// oldestRequest.CancellationTokenRegistration.Dispose();
}
}
|
@BrennanConroy, Could you please take a look at this issue and its PR? |
Fixed by #76784 |
System.Threading.RateLimiting
limiter's algorithms are using an internal queue, for queueing async acquire permit requests.There are some missing calls to disposed of queued requests cancellation token registration after setting their result.
In general, limiters try to dequeue one request from the queue and set the result of the request with a failed or successful lease.
For example, in the replenishments phase(
ReplenishInternal
), the limiter dequeues one request and, after setting its result, disposes of its cancellation token registration object(nextPendingRequest.CancellationTokenRegistration.Dispose();
). also limiter followe same bahvior onDispose
method(next.CancellationTokenRegistration.Dispose();
).The limiters should follow the same behavior in the other places, such as when they try to dequeue old requests in order to free some space for pushing new requests on the queue with the newest first processing order policy.
The text was updated successfully, but these errors were encountered: