-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[API Proposal]: RateLimiter should implement IDisposable and IAsyncDisposable #62370
Comments
Tagging subscribers to this area: @mangod9 Issue DetailsBackground and motivation#61788 added the Rate Limiting work from #52079 and while working on it we realized it made sense for rate limiters to be disposable. It started with the TokenBucket implementation which has a timer instance which should be disposed of. But it also made sense for the other rate limiters to release any pending acquisition requests from Additionally, we expect some 3rd party implementations to be backed by a network resource and async disposal is preferred in those cases. As well as implementing the two virtual methods on the rate limiter implementations (Concurrency, TokenBucket, FixedWindow, SlidingWindow) We went ahead and made the API disposable, filing this to make sure we properly API review it. API Proposalpublic abstract class RateLimiter
+ : IAsyncDisposable, IDisposable
{
+ protected virtual void Dispose(bool disposing) { }
+ public void Dispose() { }
+ protected virtual ValueTask DisposeAsyncCore() { }
+ public async ValueTask DisposeAsync() { }
} API Usagevar limiter = new ConcurrencyLimiter(new ConcurrencyLimiterOptions(1, QueueProcessingOrder.OldestFirst, 3));
using var lease = limiter.Acquire(1);
var task = limiter.WaitAsync(2);
await limiter.DisposeAsync();
var failedLease = await task; Alternative DesignsNo response RisksNo response
|
Looks good as proposed namespace System.Threading.RateLimiting
{
public partial class RateLimiter : IAsyncDisposable, IDisposable
{
protected virtual void Dispose(bool disposing) { }
public void Dispose() { }
protected virtual ValueTask DisposeAsyncCore() { }
public ValueTask DisposeAsync() { }
}
} |
Thanks 😃 |
Background and motivation
#61788 added the Rate Limiting work from #52079 and while working on it we realized it made sense for rate limiters to be disposable. It started with the TokenBucket implementation which has a timer instance which should be disposed of. But it also made sense for the other rate limiters to release any pending acquisition requests from
limiter.WaitAsync(..., token);
as well as clean up any cancellation token registrations.Additionally, we expect some 3rd party implementations to be backed by a network resource and async disposal is preferred in those cases.
As well as implementing the two virtual methods on the rate limiter implementations (Concurrency, TokenBucket, FixedWindow, SlidingWindow)
We went ahead and made the API disposable, filing this to make sure we properly API review it.
API Proposal
API Usage
Alternative Designs
No response
Risks
No response
The text was updated successfully, but these errors were encountered: