Add action-base concurrency limit support

[wu-yafeng]

I'm trying to limit concurrency request in my action OrdersController.Get but concurrency limiter middleware only support global.

Describe the solution you'd like

In controller

[ConcurrencyLimit(MaxConcurrentRequests=10)]
public ActionResult<Orders> Get()
{
}
[ConcurrencyLimit(Profile = "Profile")]
public ActionResult<Orders> Post()
{
}

In startup

public void ConfigureService(IServiceCollection services)
{
      services.AddControllers(options=>{
         options.ConcurrencyLimitProfile.Add("profile",new ConcurrencyLimitProfile());
      });
}

[javiercn]

@wu-yafeng thanks for contacting us.

@davidfowl do you have any thoughts here?
(Could this be endpoint routerized so that the middleware picks attributes from the endpoint metadata?)

[davidfowl]

Feels like it could be a nice feature of the concurrency limit middleware we shipped in 3.x. No plans to do this as part of 5.0 but we can review a design for this and take PRs.

The feature should be based on endpoints not MVC controllers (which use endpoints under the covers)

cc @Tratcher

[Tratcher]

This would have been trivial in the prior MVC setup because you could add middleware directly to a route/controller. Rather than doing something complex in the concurrency middleware to make it endpoint aware, why not make add a routing gesture to add middleware to specific routes/endpoints?

[davidfowl]

@Tratcher because we're basically done adding features to MVC in filters. The reason we did endpoint routing was to make features like this work beyond MVC and for arbitrary routes.

[Tratcher]

Right, so let's make a routing gesture that says to add a middleware to a route/endpoint, not to MVC. Trying rewrite the concurrency middleware to sniff endpoints and apply different config and state is going to be messy and in-efficient. It's much easier in this case to add separate instances of the middleware to specific routes.

[javiercn]

Trying rewrite the concurrency middleware to sniff endpoints and apply different config and state is going to be messy and in-efficient.

Without any idea about how the middleware works, wouldn't this be a case of checking the request to see if it has a given endpoint metadata and using those values instead of the ones provided by static config? With regards to efficiency, I don't think it's fair to make a claim without measuring first, and even if it has an impact, the improved experience makes up for it. If not we wouldn't have done it for authorization and CORS, for example.

Right, so let's make a routing gesture that says to add a middleware to a route/endpoint, not to MVC.

I don't think you can find a good way to do this. If you do something like MapControllers() how would you apply specific settings for a given action within a controller? The API would be incredibly cumbersome.

[Tratcher]

It's not just the settings, it would also need to maintain state per endpoint (locks, counters, etc.) so you'd end up with some sort of concurrent dictionary mapping.

[davidfowl]

@Tratcher do you think it would scale to have say 1000 concurrency limiter middlewares in the system vs a dictionary with a 1000 entries? That's the difference here.

[Tratcher]

You're going to end up with a similar number of long lived objects to track the state in either case, and if you attach each middleware instance directly to a route you'll save a secondary lookup.

The only scenario I can think of that middleware-per-endpoint can't handle is if you did some kind of named grouping. E.g. Endpoints marked as group A are concurrency limited together with specific limits, while other groups or unmarked endpoints are tracked separately.

[rynowak]

Rather than doing something complex in the concurrency middleware to make it endpoint aware, why not make add a routing gesture to add middleware to specific routes/endpoints?

I'll bite. Middleware are ordered. Trying to staple middleware to a thing that doesn't allow you to control order (when endpoints execute) takes away a lot of degrees of freedom. The MVC feature isn't widely used, and at best it's a workaround 😆. If we're starting fresh, we'd start from existing designs (middleware that behave differently based on endpoints), or from the optimal user experience - this seems strictly less optimal.

It doesn't make much sense why we'd optimize for "simpler to code". A lot of the complexity being dicussed here overlaps with topics that we've discussed related to YARP. There's no reason why we can't solve them all together, which should make writing this middleware pretty easy.

[Tratcher]

A lot of the complexity being dicussed here overlaps with topics that we've discussed related to YARP. There's no reason why we can't solve them all together, which should make writing this middleware pretty easy.

Where is that discussion?

[rynowak]

The need for Endpoints to have a global store for per-endpoint state, and also a per-generation state store for state that spans multiple endpoints of the same generation. This was discussed at the meeting a few weeks ago where @davidni demoed stuff for us.

We have primitives for computed state per-endpoint and there are already 3-4 places in the framework where we use it.

[Tratcher]

Ah, that would be helpful. Can you link to a few examples?

[rynowak]

https://github.com/dotnet/aspnetcore/search?q=DataSourceDependentCache&unscoped_q=DataSourceDependentCache

[Kahbazi]

I have a proposal for this feature and I can send a PR if t get approved.

public static class ServiceCollectionExtensions
{
    public static ConcurrencyLimiterBuilder AddConcurrencyLimiter(this IServiceCollection services, Action<ConcurrencyLimiterOptions> configure);
    public static ConcurrencyLimiterBuilder AddConcurrencyLimiter(this IServiceCollection services, string defaultProfileName);
    public static ConcurrencyLimiterBuilder AddConcurrencyLimiter(this IServiceCollection services);
}

public class ConcurrencyLimiterBuilder
{
    public ConcurrencyLimiterBuilder(IServiceCollection services, Action<ConcurrencyLimiterOptions> configure);
    public ConcurrencyLimiterBuilder(IServiceCollection services, string defaultProfile);

    public IServiceCollection Services { get; }

    public ConcurrencyLimiterBuilder AddProfile(string name, IQueuePolicy queuePolicy);
}

public static class ConcurrencyLimiterBuilderExtensions
{
    public static ConcurrencyLimiterBuilder AddQueuePolicyProfile(this ConcurrencyLimiterBuilder builder, string name, Action<QueuePolicyOptions> configureOption);
    public static ConcurrencyLimiterBuilder AddStackPolicyProfile(this ConcurrencyLimiterBuilder builder, string name, Action<QueuePolicyOptions> configureOption);
}

public class ConcurrencyLimiterOptions
{
    public IDictionary<string, Profile> Profiles { get; }

    public void AddProfile(string name, IQueuePolicy queuePolicy);

    public string DefaultProfile { get; set; }
        
    public string NoEndpointProfile { get; set; }


    // These properties clearly needs better names
    public bool UseDefaultProfileWhenNoMetadaDataFound { get; set; }
    public bool UseNoEndpointProfileWhenNoEndpointFound { get; set; }
}

public class Profile
{
    public Profile(string name, IQueuePolicy queuePolicy)

    public string Name { get; set; }
    public IQueuePolicy QueuePolicy { get; set; }
}

public class ConcurrencyLimitAttribute : Attribute
{
    public string Profile { get; set; }
}

public interface IProfileProvider
{
    Profile GetDefaultProfile();
    Profile GetNoEndpointProfile();
    Profile GetProfile(string name);
}

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[halter73]

Let's use #19823 for future discussion of wider, more fine-grained support for concurrency limits and request throttling in general.