Request limits middleware PoC #32886

JunTaoLuo · 2021-05-20T21:11:38Z

TODO:

Smoke testing
Review API
Add tests
PR (implementation) review

davidfowl · 2021-05-21T08:01:32Z

src/Middleware/RequestLimiter/sample/Controllers/HomeController.cs

+
+namespace RequestLimiterSample
+{
+    [RequestLimit(requestPerSecond: 10)]


It's unlikely this would ever be hardcoded, I wonder how useful this is as an attribute. Maybe we need it for completeness.

What do you mean hardcoded? I think it's probably useful to provide a default implementation that takes in common parameters such as "x request per second" since it's super easy to use. Otherwise, requiring the configuration of policies in ConfigureServices and specifying a policy name here may be overly complicated?

I mean read from configuration and set based on the environment

Ah yes, config driven limits. I haven't implemented that part yet.

davidfowl · 2021-05-21T08:02:27Z

src/Middleware/RequestLimiter/src/IPAggregatedRateLimiter.cs

+        private readonly long _newResourcePerSecond;
+
+        private Timer _renewTimer;
+        // This is racy


Kahbazi · 2021-05-24T18:07:59Z

src/Middleware/RequestLimiter/src/RequestLimiterMiddleware.cs

+
+            if (attributes == null)
+            {
+                // TODO: Apply default policy


suggestion : add another metadata to skip rate limit

Kahbazi · 2021-05-24T18:13:18Z

src/ResourceLimits/src/ConcurrencyLimiter.cs

+                            state: null,
+                            onDispose: resource => Release(requestedCount));


Could this code be used to remove the closure?

Suggested change

state: null,

onDispose: resource => Release(requestedCount));

state: (this, requestedCount),

onDispose: resource =>

{

(var concurrencyLimiter, var _requestedCount) = resource as (ConcurrencyLimiter, long)

concurrencyLimiter.Release(_requestedCount));

}

My concern with that approach was that State is exposed to the user. I'm not sure exposing these internal details is a good idea. But I agree that there needs to be a way to not capture the closure.

Tratcher · 2021-05-26T21:16:32Z

src/Middleware/RequestLimiter/sample/Startup.cs

+                {
+                    await Task.Delay(5000);
+                    await context.Response.WriteAsync("Default!");
+                }).EnforceLimit();


EnforceLimit gives no context here for what kind of limit.
LimitRequests?
LimitConcurrency?
LimitRequestRate? as opposed to body rate

This one enforces the default limit by adding a blank attribute. I can change the name to be more explicit.

Yeah, we want to distinguish between this and other limits like request sizes, data rates, etc..

src/Middleware/RequestLimiter/src/RequestLimiterMiddleware.cs

Tratcher · 2021-05-26T21:24:06Z

src/Middleware/RequestLimiter/src/RequestLimiterMiddleware.cs

+            {
+                while (resourceLeases.TryPop(out var resource))
+                {
+                    _logger.LogInformation("Releasing resource");


Debug, and we want some way to identity the resource being released like the policy name.

Ooops, this is meant to be placeholder logging.

Tratcher · 2021-05-26T21:27:23Z

src/Middleware/RequestLimiter/src/RequestLimiterMiddleware.cs

+                        if (_options.ResolveDefaultRequestLimit != null)
+                        {
+                            if (!await ApplyLimitAsync(_options.ResolveDefaultRequestLimit(context.RequestServices), context, resourceLeases))


The default only applies if there's an attribute but no values? There's no way to apply a default to requests without an attribute? AuthZ added a FallbackPolicy that applies to all requests without an attribute.

aspnetcore/src/Security/Authorization/Core/src/AuthorizationOptions.cs

Line 38 in af96aba

public AuthorizationPolicy? FallbackPolicy { get; set; }

Great question, I'm not sure which behaviour we want. Or maybe even both?

src/Middleware/RequestLimiter/src/RequestLimiterMiddleware.cs

Tratcher · 2021-05-26T21:35:35Z

src/Middleware/RequestLimiter/src/RequestLimiterMiddleware.cs

+                return false;
+            }
+
+            _logger.LogInformation("Resource obtained");


Debug. Identify the resource?

And never use these logging things as strings, but this is a draft.

Tratcher · 2021-05-26T21:39:07Z

src/Middleware/RequestLimiter/src/RequestLimiterOptions.cs

+            var policy = new RequestLimiterPolicy();
+            configurePolicy(policy);
+
+            PolicyMap[name] = policy;


Check for conflicts? This overwrites. Otherwise it should be SetPolicy.

What does authZ do?

The same 😢

aspnetcore/src/Security/Authorization/Core/src/AuthorizationOptions.cs

Lines 65 to 80 in af96aba

public void AddPolicy(string name, Action<AuthorizationPolicyBuilder> configurePolicy)

{

if (name == null)

{

throw new ArgumentNullException(nameof(name));

}

if (configurePolicy == null)

{

throw new ArgumentNullException(nameof(configurePolicy));

}

var policyBuilder = new AuthorizationPolicyBuilder();

configurePolicy(policyBuilder);

PolicyMap[name] = policyBuilder.Build();

}

davidfowl · 2021-05-27T07:16:53Z

src/Middleware/RequestLimiter/src/IPAggregatedRateLimiter.cs

+
+namespace Microsoft.AspNetCore.RequestLimiter
+{
+    // TODO: update implementation with WaitAsync and use MemoryCache instead of ConcurrentDictionary


It would be closer to a "remote store" based limiter example. Something like a RedisRateLimiter we envision. It's for illustration purposes.

I don't think we should think about that. We need to do what's best. I worry that we'll lose access to APIs that IMemoryCache hide.

Fair enough. Though I'm curious what you think we lose by using IMemoryCache APIs. I think it depends on how we use it.

Tratcher · 2021-05-27T20:39:03Z

src/Middleware/RequestLimiter/src/RequestLimiterMiddleware.cs

+        private Task<bool> ApplyLimitAsync(AggregatedResourceLimiter<HttpContext> limiter, HttpContext context, Stack<ResourceLease> obtainedResources)
+        {
+            _logger.LogInformation("Resource count: " + limiter.EstimatedCount(context));
+            var resourceLeaseTask = limiter.WaitAsync(context);


Pass in the RequestAborted token

Tratcher · 2021-05-27T21:08:52Z

src/Middleware/RequestLimiter/src/RequestLimiterMiddleware.cs

+            var resourceLeases = new Stack<ResourceLease>();
+            try
+            {
+                foreach (var attribute in attributes)


Discussed in review, but I'll leave my original comment here for others:

Appling multiple limits in sequence can cause some strange side effects. E.g. If the first limit hits a soft cap then I get queued until that's released. If the second hit another soft cap then you'd be stuck in line again, likely behind other requests that arrived later and didn't acquire the first limit. If the third resource hits a hard cap and I get refused immediately then why did I bother waiting for the the other two? Would it make sense to wait for all limits in parallel and abort if any are refused? It would also make multiple soft limits more fare.

Could you also get into a deadlock condition? Two requests acquiring resources in the opposite order,

JunTaoLuo · 2021-06-14T21:02:47Z

Moving to asplabs.

JunTaoLuo force-pushed the johluo/request-limits-middleware branch from 7130ee3 to 9d02c46 Compare May 20, 2021 23:23

davidfowl reviewed May 21, 2021

View reviewed changes

JunTaoLuo force-pushed the johluo/request-limits-middleware branch from 41d5bb4 to 52e0b9c Compare May 21, 2021 09:07

Pilchie added the area-runtime label May 21, 2021

Kahbazi reviewed May 26, 2021

View reviewed changes

JunTaoLuo force-pushed the johluo/request-limits-middleware branch 2 times, most recently from dc44609 to 191575d Compare May 26, 2021 20:47

Tratcher reviewed May 26, 2021

View reviewed changes

davidfowl reviewed May 27, 2021

View reviewed changes

Tratcher reviewed May 27, 2021

View reviewed changes

JunTaoLuo mentioned this pull request May 28, 2021

Rate limit APIs dotnet/runtime#52079

Closed

JunTaoLuo force-pushed the johluo/request-limits-middleware branch from b544026 to 5d30a80 Compare June 3, 2021 21:26

John Luo added 7 commits June 3, 2021 16:22

Create Resource limits feature

609f028

Update concurrency limiter

04a679b

Update API

9b6284f

Update ConcurrencyLimiter implementation

8fd834f

Wip Update TokenBucketImplementation

4cd525e

Update API based on review feedback

40a418f

Update API to use abstract class for PermitLease

baaedeb

JunTaoLuo force-pushed the johluo/request-limits-middleware branch 2 times, most recently from f04fb9f to 838149d Compare June 4, 2021 20:33

John Luo added 8 commits June 4, 2021 13:36

Make AvailablePermits a function

c53a14a

Create request limits middleware

1da3f1c

Fixup rate limiter

8ad26e8

React to API change

8f043d4

Update middleware implementation

e974b55

Some feedback

f872ff1

Wip

ecd8a26

Update API

cd71b9d

John Luo added 2 commits June 4, 2021 13:38

Update handling of simple and aggregated limiters

253ff8f

React to API refactor

e33467d

JunTaoLuo force-pushed the johluo/request-limits-middleware branch from 838149d to e33467d Compare June 4, 2021 20:42

JunTaoLuo closed this Jun 14, 2021

dougbu deleted the johluo/request-limits-middleware branch August 21, 2021 22:33

amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request limits middleware PoC #32886

Request limits middleware PoC #32886

JunTaoLuo commented May 20, 2021

davidfowl May 21, 2021

JunTaoLuo May 21, 2021

davidfowl May 21, 2021

JunTaoLuo May 21, 2021

davidfowl May 21, 2021

Kahbazi May 24, 2021

Kahbazi May 24, 2021

JunTaoLuo May 26, 2021

Tratcher May 26, 2021

JunTaoLuo May 26, 2021 •

edited

Loading

Tratcher May 26, 2021

Tratcher May 26, 2021

JunTaoLuo May 26, 2021

Tratcher May 26, 2021

JunTaoLuo May 26, 2021

Tratcher May 26, 2021

davidfowl May 26, 2021 •

edited

Loading

Tratcher May 26, 2021

davidfowl May 26, 2021

Tratcher May 26, 2021

davidfowl May 27, 2021

JunTaoLuo May 27, 2021

davidfowl May 27, 2021

JunTaoLuo May 27, 2021

Tratcher May 27, 2021

Tratcher May 27, 2021

JunTaoLuo commented Jun 14, 2021

		state: null,
		onDispose: resource => Release(requestedCount));

	public void AddPolicy(string name, Action<AuthorizationPolicyBuilder> configurePolicy)
	{
	if (name == null)
	{
	throw new ArgumentNullException(nameof(name));
	}

	if (configurePolicy == null)
	{
	throw new ArgumentNullException(nameof(configurePolicy));
	}

	var policyBuilder = new AuthorizationPolicyBuilder();
	configurePolicy(policyBuilder);
	PolicyMap[name] = policyBuilder.Build();
	}

Request limits middleware PoC #32886

Request limits middleware PoC #32886

Conversation

JunTaoLuo commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo May 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl May 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo commented Jun 14, 2021

JunTaoLuo May 26, 2021 •

edited

Loading

davidfowl May 26, 2021 •

edited

Loading