Update RateLimiter queues on cancellation #64825
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @mangod9 Issue DetailsFixes #64078 Realized the issue wasn't specific to TokenBucket so updated the base test class.
|
Is the custom TCS really less expensive than the closure? I figured it'd be about the same. |
|
Without the custom tcs you have the normal TCS allocation + the closure allocation. With a custom tcs, you have the single class allocation, so basically a TCS with a couple extra fields. |
|
|
||
| private class WrappedTCS : TaskCompletionSource<RateLimitLease> |
There was a problem hiding this comment.
Can this type be moved to the base class? So it's doubled from ConcurrencyLimiter (almost).
There was a problem hiding this comment.
There is a lot of similar code between TokenBucket and ConcurrencyLimiter, planning on sharing a lot of it in a future change.
...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs
Outdated
Show resolved
Hide resolved
...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public void TryCancel() | ||
| { | ||
| if (TrySetException(new OperationCanceledException())) |
There was a problem hiding this comment.
Why this rather than TrySetCanceled()?
There was a problem hiding this comment.
Here's some previous discussion: aspnet/AspLabs#387 (comment)
Given that we're doing return new ValueTask<RateLimitLease>(Task.FromCanceled<RateLimitLease>(cancellationToken)); instead of calling ThrowIfCancellationRequested(), that's even more reason to use TrySetCanceled() now here.
We should probably capture the cancellationToken too and call the TrySetCanceled(CancellationToken) overload.
| Limiter = limiter; | ||
| } | ||
|
|
||
| public void TryCancel() |
There was a problem hiding this comment.
Nit: there's a minor (internal) usability issue here, as now there's a functional difference between TryCancel and TrySetCanceled, which sound very similar.
There was a problem hiding this comment.
Would the recommended approach be to new override TrySetCanceled?
|
Failures unrelated to the PR and tracked in #64963, merging. |
Fixes #64078
Realized the issue wasn't specific to TokenBucket so updated the base test class.
Also, avoiding the extra allocations for a closure by implementing a custom TCS.