Add RateLimiting APIs #61788

BrennanConroy · 2021-11-18T17:02:52Z

Part of dotnet/aspnetcore#37385
Moves the current non-generic APIs and two rate limiter implementations from https://github.com/aspnet/AspLabs/tree/main/src/RateLimiting.

There were a couple minor code fixups needed for building in the repo because the project now targets more TFMs than just netstandard2.0. I will call out the changes in code review comments.

Couple things to note:

Move Deque to the Common/ folder as it now lives in Channels and RateLimiting
- I can do this if we want but was trying to limit the scope of this PR
Copied the TaskExtensions.DefaultTimeout() methods from asplabs/aspnetcore that we use in our tests to detect hangs/prevent hangs on the CI. Not sure how the Runtime handles test hangs as I haven't seen tests do anything special with awaiting tasks.
Does the project need <IsPackable> or any other metadata?

dotnet-issue-labeler · 2021-11-18T17:02:59Z

Note regarding the new-api-needs-documentation label:

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 · 2021-11-18T17:03:02Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

Part of dotnet/aspnetcore#37385
Moves the current non-generic APIs and two rate limiter implementations from https://github.com/aspnet/AspLabs/tree/main/src/RateLimiting.

There were a couple minor code fixups needed for building in the repo because the project now targets more TFMs than just netstandard2.0. I will call out the changes in code review comments.

Couple things to note:

Move Deque to the Common/ folder as it now lives in Channels and RateLimiting
- I can do this if we want but was trying to limit the scope of this PR
Copied the TaskExtensions.DefaultTimeout() methods from asplabs/aspnetcore that we use in our tests to detect hangs/prevent hangs on the CI. Not sure how the Runtime handles test hangs as I haven't seen tests do anything special with awaiting tasks.
Does the project need <IsPackable> or any other metadata?

Author:	BrennanConroy
Assignees:	-
Labels:	`area-System.Threading`, `new-api-needs-documentation`
Milestone:	-

BrennanConroy · 2021-11-18T17:03:34Z

...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs

+                }
+
+                TaskCompletionSource<RateLimitLease> tcs = new TaskCompletionSource<RateLimitLease>(TaskCreationOptions.RunContinuationsAsynchronously);
+                CancellationTokenRegistration ctr = default;


Added = default; since the compiler doesn't like you using non-initialized variables.

BrennanConroy · 2021-11-18T17:04:08Z

...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs

+                {
+                    ctr = cancellationToken.Register(obj =>
+                    {
+                        ((TaskCompletionSource<RateLimitLease>)obj!).TrySetException(new OperationCanceledException(cancellationToken));


Added ! to obj, to fix nullable warning

The use of cancellationToken in the delegate is causing this method to always allocate a closure, even on the fast paths.

BrennanConroy · 2021-11-18T17:06:09Z

src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/MetadataName.T.cs

+        }
+
+        /// <inheritdoc/>
+        public bool Equals(MetadataName<T>? other)


Added ? since IEquatable requires it, and added the if (other is null) check

BrennanConroy · 2021-11-18T17:06:34Z

...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs

+
+                TaskCompletionSource<RateLimitLease> tcs = new TaskCompletionSource<RateLimitLease>(TaskCreationOptions.RunContinuationsAsynchronously);
+
+                CancellationTokenRegistration ctr = default;


Added = default;

BrennanConroy · 2021-11-18T17:06:44Z

...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs

+                {
+                    ctr = cancellationToken.Register(obj =>
+                    {
+                        ((TaskCompletionSource<RateLimitLease>)obj!).TrySetException(new OperationCanceledException(cancellationToken));


Added ! to obj

Same issue here with regards to a closure.

BrennanConroy · 2021-11-18T17:07:05Z

...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs

+        }
+
+        // Used in tests that test behavior with specific time intervals
+        private void ReplenishInternal(uint nowTicks)


Changed to private and use reflection in tests to avoid IVT

BrennanConroy · 2021-11-18T17:07:51Z

src/libraries/System.Threading.RateLimiting/tests/TokenBucketRateLimiterTests.cs

+            var wait = limiter.WaitAsync(1);
+            Assert.False(wait.IsCompleted);
+
+            var replenishInternalMethod = typeof(TokenBucketRateLimiter).GetMethod("ReplenishInternal", Reflection.BindingFlags.NonPublic | Reflection.BindingFlags.Instance)!;


Changed to reflection to call private method instead of IVT

stephentoub · 2021-11-18T18:06:03Z

...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs

+            }
+
+            // Perf: Check SemaphoreSlim implementation instead of locking
+            if (_permitCount >= permitCount)


These non-synchronized fast-path checks mean we might return failure even if success is possible. That's ok?

I think it's fine, it's an implicit race, locking would still race with someone releasing permits at the same time.

...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs

src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/Internal/Deque.cs

src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/MetadataName.T.cs

src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/RateLimitLease.cs

src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/RateLimiter.cs

...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs

src/libraries/System.Threading.RateLimiting/ref/System.Threading.RateLimiting.csproj

src/libraries/System.Threading.RateLimiting/tests/System.Threading.RateLimiting.Tests.csproj

src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/MetadataName.T.cs

src/libraries/System.Threading.RateLimiting/tests/Internal/TaskExtensions.cs

eerhardt · 2021-11-18T20:49:30Z

src/libraries/System.Threading.RateLimiting/ref/System.Threading.RateLimiting.cs

+    }
+    public enum QueueProcessingOrder
+    {
+        OldestFirst = 0,


The approved API called these:

ProcessOldest, ProcessNewest

The approved API code was wrong, the comment above the code states:

We decided to rename the QueueProcessingOrder members from { ProcessOldest, ProcessNewest } to { OldestFirst, NewestFirst }

eerhardt · 2021-11-18T20:52:03Z

src/libraries/System.Threading.RateLimiting/ref/System.Threading.RateLimiting.cs

+        protected abstract void Dispose(bool disposing);
+        public virtual System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>> GetAllMetadata() { throw null; }
+        public abstract bool TryGetMetadata(string metadataName, out object? metadata);
+        public bool TryGetMetadata<T>(System.Threading.RateLimiting.MetadataName<T> metadataName, [System.Diagnostics.CodeAnalysis.MaybeNullAttribute] out T metadata) { throw null; }


Shouldn't metadata be attributed with [MaybeNullWhen(false)]?

What does it mean to return true from TryGetMetadata, but the metadata still being null?

During one of the reviews it was discussed that a null value could be valid metadata

I remember discussing wanting to distinguish between metadata that is sometimes defined but not present (returns true but gives null) vs metadata that is never present (returns false).

Previous discussion: #52079 (comment)

src/libraries/System.Threading.RateLimiting/ref/System.Threading.RateLimiting.cs

src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/RateLimitLease.cs

eerhardt · 2021-11-18T21:00:09Z

...em.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiterOptions.cs

+        public QueueProcessingOrder QueueProcessingOrder { get; }
+
+        /// <summary>
+        /// Maximum cumulative token count of queued acquisition requests.


This description is different than ConcurrencyLimiterOptions:

/// Maximum number of permits that can be queued concurrently.

This is done on purpose. The API review called out that TokenBucket should refer to permits as tokens where possible.

eerhardt · 2021-11-18T23:36:03Z

src/libraries/System.Threading.RateLimiting/ref/System.Threading.RateLimiting.cs

+
+namespace System.Threading.RateLimiting
+{
+    public sealed partial class ConcurrencyLimiter : System.Threading.RateLimiting.RateLimiter


Is it odd that we have RateLimiter, TokenBucketRateLimiter, but then ConcurrencyLimiter? Why not ConcurrencyRateLimiter?

martincostello

We've had a rate-limiting prototype sitting around in Polly for a while (App-vNext/Polly#666) so I thought I'd take a look at this - looks nice.

...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs

...ries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/QueueProcessingOrder.cs

...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs

...em.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiterOptions.cs

...es/System.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiter.cs

...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs

src/libraries/System.Threading.RateLimiting/ref/System.Threading.RateLimiting.cs

eerhardt

LGTM.

src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/RateLimiter.cs

eerhardt · 2021-12-03T18:09:10Z

...raries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ConcurrencyLimiter.cs

+            }
+
+            // Return SuccessfulLease or FailedLease to indicate limiter state
+            if (permitCount == 0 && !_disposed)


Do we want to throw ObjectDisposedException if this has been disposed?

Should an ODE throw synchronously or be wrapped in a ValueTask?

I guess the first question is:

Do we want to throw ODE or always return "failed" when a limiter is disposed?

Doing a quick check of other System.Threading disposable types, they seem to throw ODE: Barrier, ManualResetEventSlim, ReaderWriterLockSlim

For the next question, I would say it is similar to argument checking - so we would throw inline.

BrennanConroy requested review from halter73, eerhardt and rafikiassumani-msft November 18, 2021 17:02

dotnet-issue-labeler bot added area-System.Threading new-api-needs-documentation labels Nov 18, 2021

BrennanConroy commented Nov 18, 2021

View reviewed changes