-
Notifications
You must be signed in to change notification settings - Fork 10.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Request timeouts middleware #45732
Comments
Thanks for contacting us. We're moving this issue to the |
It feels a bit odd to have a mix of endpoint specific metadata that further filters the request to discover the timeout. The point of metadata is to bake in the decision about which endpoints have which metadata at startup so cheap decision can be made per request. |
This supports determining the timeout for any non-endpoint-aware resources like static files. It's optional for now, but I anticipate customers asking for it.
So that policy instance would get stored in the endpoint metadata and then checked for along with the attributes?
@sebastienros is working on an effort to make more of our components config aware, reloadable, etc. since this is a common requirement from real apps. They don't want to recompile to change timeouts. The API shape for that experience is negotiable. I don't think this can be removed without a proposed alternative.
I don't follow. Where is there further filtering? Endpoints store a specific timeout, a policy name for a centrally configured timeout, or an opt-out. Or are you expecting the policy name to be resolved to a timeout when building the endpoint? |
Whenever we add middleware that has predicate as part of the options, I question the need. We have routing matching and pipeline branching, why do we need middleware specific filters on top of that? This also came up when we were discussing output caching.
Right, much like the authorization policies
I think that's fine, but I'm not a fan of making one off changes like this to new middleware. This proposal needs to be all encompassing, and we shouldn't add this API to this one place. As an example, the hosting filtering middleware supports config reloading and it doesn't take IConfiguration as input (which is good, it should be using IOptionsMonitor<T>). I don't want to end up in a situation like this again.
Endpoints have already been routed to (routing is the filtering). Routing can applies policies. e.g. app.MapGet("/long", (Cancellation token) => client.GetSomethingAsync(token))
.RequireHost("*:80");
.WithTimeout(TimeSpan.FromSeconds(5)); The above endpoint is already filtered to GET requests with /long in the route and port 80. |
I'm still not sure which part of the design you're talking about? Which API is this for? |
I've updated the proposal to remove the TimeoutLocator from RequestTimeoutPolicy, but left the one on RequestTimeoutOptions for further discussion. I've commented out the IConfigurationSection API and left a note about considering the IConfiguration integration story. I've also added a WithRequestTimeout overload that directly takes a RequestTimeoutPolicy. |
|
Removed.
No?
We should consider Update/Remove as part of the reloadable configuration story.
TimeSpan isn't a supported input for attributes. I wanted to avoid string input as both error prone and it would conflict with the policy overload.
👍 |
|
Removed DefaultTimeoutLocator. Added RemovePolicy, though at this point maybe we should be using a thread safe collection instead. |
@Tratcher Is this issue up for grab? |
How about also adding + public static class RequestTimeoutsIEndpointConventionBuilderExtensions
+ {
+ public static IEndpointConventionBuilder DisableRequestTimeout(this IEndpointConventionBuilder builder) { }
+ } |
Also Why does |
Not until we finish the API review. |
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
Added |
Null would allow you to conditionally/programatically disable the policy without updating all of the endpoints to DisableRequestTimeout. Timeout.InfiniateTimeSpan is an alternative, but I'd like to avoid special values. |
API Review Notes:
namespace Microsoft.Extensions.DependencyInjection;
+ public static class RequestTimeoutsIServiceCollectionExtensions
+ {
+ public static IServiceCollection AddRequestTimeouts(this IServiceCollection services) { }
+ public static IServiceCollection AddRequestTimeouts(this IServiceCollection services, Action<RequestTimeoutOptions> configure) { }
+ }
namespace Microsoft.AspNetCore.Builder;
+ public static class RequestTimeoutsIApplicationBuilderExtensions
+ {
+ public static IApplicationBuilder UseRequestTimeouts(this IApplicationBuilder builder) { }
+ }
+ public static class RequestTimeoutsIEndpointConventionBuilderExtensions
+ {
+ public static IEndpointConventionBuilder WithRequestTimeout(this IEndpointConventionBuilder builder, TimeSpan timeout) { }
+ public static IEndpointConventionBuilder WithRequestTimeout(this IEndpointConventionBuilder builder, string policyName) { }
+ public static IEndpointConventionBuilder WithRequestTimeout(this IEndpointConventionBuilder builder, RequestTimeoutPolicy policy) { }
+ public static IEndpointConventionBuilder DisableRequestTimeout(this IEndpointConventionBuilder builder) { }
+ }
+ namespace Microsoft.AspNetCore.Http.Timeouts;
+ public sealed class RequestTimeoutOptions
+ {
+ // Applied to any request without a policy set. No value by default.
+ public TimeSpan? DefaultTimeout { get; set; }
+ public RequestTimeoutOptions AddPolicy(string policyName, TimeSpan timeout) { }
+ public RequestTimeoutOptions AddPolicy(string policyName, RequestTimeoutPolicy policy) { }
+ public IDictionary<string, RequestTimeoutPolicy> Policies { get; }
+ }
+ public sealed class RequestTimeoutPolicy
+ {
+ public TimeSpan? Timeout { get; }
+ public RequestTimeoutPolicy(TimeSpan? timeout) { }
+ }
+ // Overrides the global timeout, if any.
+ [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class)]
+ public sealed class RequestTimeoutAttribute : Attribute
+ {
+ public TimeSpan? Timeout { get; }
+ public string? PolicyName { get; }
+ public RequestTimeoutAttribute(int milliseconds) { }
+ public RequestTimeoutAttribute(string policyName) { }
+ }
+ // Disables all timeouts, even the global one.
+ [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class)]
+ public sealed class DisableRequestTimeoutAttribute : Attribute
+ {
+ public DisableRequestTimeoutAttribute() { }
+ } API Approved! |
I know we just approved |
Policies seems fine, the fact that you need a name to add/get them seems self documenting that the others are inaccessible. |
@Kahbazi, this should be ready, any questions before you get started? |
Why is there no callback API on AddPolicy? Is it because it only has the timeout? |
RequestTimeoutPolicy is currently designed to be immutable which conflicts with the callback pattern. Which would you prefer? |
Everything looks clear for now. I'll start coding! |
Bumping back to API review. There's a customer ask for making the timeout response configurable. There's already a question in the PR about what the default status code should be, so being able to configure at least that seems obvious. Allowing a RequestDelegate to generate the response seems the most flexible. + public int? TimeoutStatusCode { get; set; }
+ public RequestDelegate WriteTimeoutResponse { get; set; } It would be called here: Question: Should this be on RequestTimeoutOptions, RequestTimeoutPolicy, or both? |
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
API Review Notes:
API Approved! - @Kahbazi + public sealed class RequestTimeoutOptions
+ {
+ // Applied to any request without a policy set. No value by default.
+ public RequestTimeoutPolicy? DefaultPolicy { get; set; }
+ public RequestTimeoutOptions AddPolicy(string policyName, TimeSpan timeout) { }
+ public RequestTimeoutOptions AddPolicy(string policyName, RequestTimeoutPolicy policy) { }
+ public IDictionary<string, RequestTimeoutPolicy> Policies { get; }
+ }
+ public sealed class RequestTimeoutPolicy
+ {
+ public TimeSpan? Timeout { get; init; }
+ public int? TimeoutStatusCode { get; init; }
+ public RequestDelegate? WriteTimeoutResponse { get; init; }
+ } |
I'm on it! |
Here's an addition to address long running request scenarios that aren't endpoint aware and need to opt-out of the timeout. public interface IHttpRequestTimeoutFeature
{
void CancelTimeout();
}
We'll review this on Monday. |
Are we going to use this issue to review the proposed |
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
API Review Notes:
var timeoutFeature = context.Features.Get<IHttpRequestTimeoutFeature>();
timeoutFeature.DisableTimeout();
if (!timeoutFeature.RequestTimeoutToken.IsCancellationRequested)
{
// Timeout was successfully disabled
} API Approved! namespace Microsoft.AspNetCore.Http.Timeouts;
+ public interface IHttpRequestTimeoutFeature
+ {
+ CancellationToken RequestTimeoutToken { get; }
+ void DisableTimeout();
+ } |
Background and Motivation
A common customer request is to be able to apply timeouts to their requests. AspNetCore servers don't do this by default since request times vary widely by scenario and we don't have good ways to predict that. E.g. WebSockets, static files, expensive APIs, etc..
To provide more control we can provide timeouts via middleware that are configured per-endpoint, as well as a global timeout if desired. These timeouts would link to the RequestAborted CancellationToken and be entirely cooperative. E.g. we won't call Abort when the timeout fires. It's up to the application logic to consume RequestAborted and decide how to handle the cancellation.
Proposed API
Usage Examples
Alternative Designs
Risks
The text was updated successfully, but these errors were encountered: