Skip to content
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

IHttpForwarder and cancellation #1542

Closed
Tratcher opened this issue Feb 1, 2022 · 5 comments · Fixed by #1985
Closed

IHttpForwarder and cancellation #1542

Tratcher opened this issue Feb 1, 2022 · 5 comments · Fixed by #1985
Assignees
Labels
Type: Bug Something isn't working

Comments

@Tratcher
Copy link
Member

Tratcher commented Feb 1, 2022

IHttpForwarder.SendAsync does not directly accept a CancellationToken to abort it. It supports canceling if the ForwarderRequestConfig.ActivityTimeout expires, or if HttpContext.RequestAborted triggers (client disconnect).

ValueTask<ForwarderError> SendAsync(HttpContext context, string destinationPrefix, HttpMessageInvoker httpClient,
ForwarderRequestConfig requestConfig, HttpTransformer transformer);

A customer that wanted to add their own cancellation condition did so by replacing HttpContext.RequestAborted. However, this caused an issue where they were leaking resources because they were no longer responding to the original RequestAborted token. The fix was to chain the two tokens.

Feedback: Having a CancellationToken parameter on IHttpForwarder.SendAsync would make this less error prone.

@Tratcher Tratcher added the Type: Bug Something isn't working label Feb 1, 2022
@MihaZupan
Copy link
Member

Having a CancellationToken parameter on IHttpForwarder.SendAsync would make this less error prone

I agree with this from the perspective of runtime APIs, but this doesn't seem to be the pattern AspNet follows to my knowledge:

  • Middleware don't give you CancellationTokens explicitly - you are expected to use HttpContext.RequestAborted
  • Same thing for MVC controllers
  • You can't inject a CT into minimal APIs
app.MapGet("/delay1", async (CancellationToken ct) => // blows up at runtime
{
    while (true)
    {
        Console.WriteLine(ct.IsCancellationRequested);
        await Task.Delay(1000);
    }
});

app.MapGet("/delay2", async (HttpContext context) =>
{
    while (true)
    {
        Console.WriteLine(context.RequestAborted.IsCancellationRequested);
        await Task.Delay(1000);
    }
});

With YARP being in the middle between runtime and AspNet, do we believe YARP users are more error-prone to this than those of other web apps?

@Tratcher
Copy link
Member Author

Tratcher commented Feb 1, 2022

The main difference I see is that Middleware isn't invoked by users, it's invoked by the runtime that doesn't need to customize parameters. IHttpForwarder is an API invoked by the user more like HttpClient that does take a CT.

@karelz karelz added this to the Backlog milestone Feb 1, 2022
@davidfowl
Copy link
Contributor

@MihaZupan

You can't inject a CT into minimal APIs

You can and also into MVC controller actions.

@MihaZupan
Copy link
Member

You can and also into MVC controller actions.

Oh was that added in 7?

@davidfowl
Copy link
Contributor

davidfowl commented May 9, 2022

6.0 supports cancellation tokens but MVC always supported it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

5 participants