Add CreateChained rate limiter API #70739
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. |
ghost
assigned 
|
Tagging subscribers to this area: @mangod9 Issue DetailsPart of #65400
|
...em.Threading.RateLimiting/src/System/Threading/RateLimiting/ChainedPartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
...em.Threading.RateLimiting/src/System/Threading/RateLimiting/ChainedPartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
| return new CombinedRateLimitLease(leases!); | ||
| } | ||
|
|
||
| // REVIEW: Do we dispose the inner limiters? Or just mark the object as disposed and throw in all the methods |
There was a problem hiding this comment.
I don't see why ChainedPartitionedRateLimiter should assume ownership of the inner limiters. It didn't create them. Throwing an ODE from all the methods after disposal seems fine.
The only thing I don't like about this is that the inner limiters might clean up outstanding leases when they're disposed, and this wouldn't do that. I don't want to be in the business of tracking outstanding leases and cleaning up ourselves at this layer though. We might ultimately have to do that here, but not right now.
For now, let's make sure we have clear documentation telling consumers to cancel all outstanding waits and dispose all outstanding leases before disposing the limiter returned from CreateChained. Or perhaps make that the guidance for RateLimters and PartitionedRateLimiter<TResource>s in general.
That should be the case for our rate limiting middleware with graceful shutdown at least, but then there's always ungraceful shutdown where the host and the server timeout waiting for requests and dispose everything anyway 😞.
...em.Threading.RateLimiting/src/System/Threading/RateLimiting/ChainedPartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
...em.Threading.RateLimiting/src/System/Threading/RateLimiting/ChainedPartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
...em.Threading.RateLimiting/src/System/Threading/RateLimiting/ChainedPartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public override IEnumerable<string> MetadataNames => throw new NotImplementedException(); | ||
|
|
||
| public override bool TryGetMetadata(string metadataName, out object? metadata) => throw new NotImplementedException(); |
There was a problem hiding this comment.
Same question here. I get that things can get hairy when multiple leases have metadata with the same name and that might be really common, but I don't see the harm in making a best effort.
Maybe when only one lease has metadata defined for a give name, return the object directly, and otherwise return the object[] of all the metadata. Obviously the object[] would break anything not expecting it, but things could expect it if they know they are using CreateChained.
...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/PartitionedRateLimiter.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace System.Threading.RateLimiting | ||
| { | ||
| internal sealed class ChainedPartitionedRateLimiter<TResource> : PartitionedRateLimiter<TResource> |
There was a problem hiding this comment.
Could you add some top-level comment(s) describing what the strategy is (acquire all in order, reject if any reject)
| if (_metadataNames is null) | ||
| { | ||
| _metadataNames = new HashSet<string>(); | ||
| foreach (RateLimitLease lease in _leases) |
There was a problem hiding this comment.
What if 2 leases have metadata with the same name?
There was a problem hiding this comment.
Using a HashSet to avoid duplicates and returning the metadata object from the first lease with that name.
@halter73 mentioned possibly returning an object[] if there are multiple leases with the same metadata name, but I decided not to do that because it makes the consumption pattern really messy.
if (lease.TryGetMetadata("something", out object? metadata))
{
if (metadata is object[] objects)
{
// loop over items and do something
}
else if (metadata is not null)
{
// do something
}
}
// or
if (lease.TryGetMetadata(MetadataName.RetryAfter, out TimeSpan timeSpan))
{
// only works if a single lease returns a "RETRY_AFTER" metadata item, breaks if we used object[] for multiple leases
}| private readonly Func<TResource, RateLimitPartition<TKey>> _partitioner; | ||
| private static TimeSpan _idleTimeLimit = TimeSpan.FromSeconds(10); | ||
|
|
||
| // TODO: Look at ConcurrentDictionary to try and avoid a global lock |
There was a problem hiding this comment.
TODO for this PR or a follow-up?
There was a problem hiding this comment.
Follow-ups, this code isn't new, the first commit contained a bunch of code organization.
| _limiters = new Dictionary<TKey, Lazy<RateLimiter>>(equalityComparer); | ||
| _partitioner = partitioner; | ||
|
|
||
| // TODO: Figure out what interval we should use |
| { | ||
| await Heartbeat().ConfigureAwait(false); | ||
| } | ||
| // TODO: Can we log to EventSource or somewhere? Maybe dispatch throwing the exception so it is at least an unhandled exception? |
| { | ||
| try | ||
| { | ||
| await limiter.Value.Value.DisposeAsync().ConfigureAwait(false); |
| } | ||
| } | ||
| } | ||
| else if (rateLimiter.Value.Value is ReplenishingRateLimiter replenishingRateLimiter) |
There was a problem hiding this comment.
Add a comment for this special case
Part of #65400