Add RateLimiting APIs

[BrennanConroy]

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?

[ghost]

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]

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]

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

[BrennanConroy]

Added ! to obj, to fix nullable warning

[stephentoub]

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

[BrennanConroy]

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

[BrennanConroy]

Added = default;

[BrennanConroy]

Added ! to obj

[stephentoub]

Same issue here with regards to a closure.

[BrennanConroy]

Changed to private and use reflection in tests to avoid IVT

[BrennanConroy]

Changed to reflection to call private method instead of IVT

[stephentoub]

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

[BrennanConroy]

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

[eerhardt]

The approved API called these:

ProcessOldest,
ProcessNewest

[BrennanConroy]

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]

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

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

[BrennanConroy]

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

[halter73]

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)

[eerhardt]

This description is different than ConcurrencyLimiterOptions:

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

[BrennanConroy]

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

[eerhardt]

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.

[eerhardt]

LGTM.

[eerhardt]

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

[BrennanConroy]

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

[eerhardt]

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.