Update http proxy to handle client disconnect scenario #10688
Conversation
|
For the scenarios you tested, can you compare how this would behave in a regular AspNetCore app? I think we should align with that behavior as best we can. If client disconnects, is an exception thrown? Is the request considered failed or successful? |
Here's what I found using a plain aspnet app: RequestAborted
CancellationTokenSource cancelled
Here's the app I used, or do you think I need to try and replicate what we're doing with the YARP as well? var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();
app.MapGet("/process", async (HttpContext context) =>
{
var cancellationToken = context.RequestAborted; // Client disconnect
// var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(5)).Token; // Cancel after 5 seconds
try
{
// Simulate some work that can be cancelled
Console.WriteLine("Starting work...");
for (int i = 0; i < 10; i++)
{
// Simulate work
await Task.Delay(1000, cancellationToken);
Console.WriteLine($"Work iteration {i + 1} completed.");
}
Console.WriteLine("Work completed.");
context.Response.StatusCode = StatusCodes.Status200OK;
await context.Response.WriteAsync("Processing completed successfully.");
}
catch (OperationCanceledException)
{
Console.WriteLine("Request was cancelled.");
context.Response.StatusCode = StatusCodes.Status499ClientClosedRequest; // Status code for client closed request
await context.Response.WriteAsync("Request was cancelled.");
}
});
app.Run(); |
|
Don't need to test with YARP directly, but I would like us to compare with your found results for AspNetCore with what happens in the functions YARP scenario today. Also: |
@jviau In comparison, here is the result to the exact same code as above but in a function app with what we have today in dev: RequestAborted
CancellationTokenSource cancelled
And with my changes in this PR:RequestAborted
CancellationTokenSource cancelled
|
| @@ -36,6 +37,11 @@ public async Task Invoke(HttpContext context) | |||
|
|
|||
| await _next.Invoke(context); | |||
|
|
|||
| if (context.RequestAborted.IsCancellationRequested) | |||
There was a problem hiding this comment.
| if (context.RequestAborted.IsCancellationRequested) | |
| if (context.RequestAborted.IsCancellationRequested && !context.Response.HasStarted) |
Got this from the PR which added this handling to AspNetCore: dotnet/aspnetcore#46330
May be useful to skim this PR to see if there is anything else we should be handling/doing
There was a problem hiding this comment.
I wonder if the changes here might be greater and it's worth its own PR?
| context.Response.StatusCode = StatusCodes.Status499ClientClosedRequest; | ||
| } | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Can you use a when statement?
| LoggerMessage.Define<string>( | ||
| LogLevel.Debug, | ||
| new EventId(531, nameof(RequestAborted)), | ||
| "The request was aborted by the client (requestId: '{mS_ActivityId}')."); |
There was a problem hiding this comment.
Is the capitalization of mS intentional here?
There was a problem hiding this comment.
Yeah I was copying what ExecutedHttpRequest already had, but I don't know if this was intentional by the original author. I change my reference to it, but I think for ExecutedHttpRequest, something in Properties.Resources.ExecutingHttpRequest) is expecting that format
azure-functions-host/src/WebJobs.Script.WebHost/Properties/Resources.resx
Lines 126 to 139 in c84ce0d
There was a problem hiding this comment.
But I think we either change them all, or none. I'd rather not have the inconsistency in naming
There was a problem hiding this comment.
If it is an existing way we case it, that works for me.
Issue describing the changes in this PR
resolves #10600
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-procbranch to be included in Core Tools and non-Flex deployments.in-procbranch is not requiredrelease_notes.mdAdditional information
SystemTraceMiddlewareto returnStatus499ClientClosedRequeston client disconnectRetryProxyHandlerto stop retries when the request is cancelled and to not throw an exceptionQuestions
Testing
See comments below for test scenarios completed. Behaviour change from this PR have been compared to a) what is in dev today and b) what aspnet does