-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add initial impl of PartitionedRateLimiter.Create #67677
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @mangod9 Issue DetailsAdds the Some more work will need to be done once #67020 is merged so we can properly handle replenishing rate limiters. Additionally, fixed a bug (in concurrency and token bucket) with cancellation when disposing the limiter and when adding new requests to the queue. Found while writing a cancellation test for the
|
a559972
to
7c9b984
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since #67020 is merged, can we do something? Or are we going to leave that as follow up for preview5? I guess we can announce it with the caveat that it only works for a statically bounded number of partitions. Partitioning by username for example would not be advisable.
_partitioner = partitioner; | ||
|
||
// TODO: Only create timer once there is an active replenishing limiter | ||
// TODO: Figure out what interval we should use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we file issues for the any TODOs we think are worth following up in the .NET 7 timeframe? I think these probably qualify. I think we might want to add public API to make this interval configurable.
...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/RateLimitPartition.cs
Show resolved
Hide resolved
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs
Show resolved
Hide resolved
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Show resolved
Hide resolved
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Show resolved
Hide resolved
9ab4643
to
e49b23f
Compare
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
_timer.Start(); | ||
while (await _timer) | ||
{ | ||
Replenish(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're going to have to catch any exceptions now that we rely on the loop continuing. It's unfortunate that we cannot log it, and this stops any exception from showing up as an UnhandledException
. It's tempting to use ExceptionDispatchInfo.Capture
and dispatch the throwing of the exception to a threadpool thread.
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Show resolved
Hide resolved
b5914bf
to
f993c4b
Compare
Adds the
PartitionedRateLimiter.Create
method and the other types necessary to implement it.#65400
Some more work will need to be done once #67020 is merged so we can properly handle replenishing rate limiters.
Additionally, fixed a bug (in concurrency and token bucket) with cancellation when disposing the limiter and when adding new requests to the queue. Found while writing a cancellation test for the
PartitionedRateLimiter
.CreateChained
andTranslateKey
are not included in this change.