Http.Sys: Clean up Request parsing errors #57531
Conversation
BrennanConroy
added
the
area-networking
BrennanConroy
requested review from
halter73,
JamesNK and
mgravell
as code owners
| @@ -142,6 +136,7 @@ public async Task Request_OverlongUTF8Path(string requestPath, string expectedPa | |||
| [InlineData("/", "/", "", "/")] | |||
| [InlineData("/base", "/base", "/base", "")] | |||
| [InlineData("/base", "/baSe", "/baSe", "")] | |||
| [InlineData("/base", "/baSe/", "/baSe", "/")] | |||
There was a problem hiding this comment.
There was a branch in Request.ctor that wasn't being hit by tests, this test case hits it
src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs
Show resolved
Hide resolved
| catch (Exception ex) | ||
| { | ||
| Log.RequestParsingError(Logger, ex); | ||
| Server.SendError(_requestId.Value, StatusCodes.Status400BadRequest, authChallenges: null); |
There was a problem hiding this comment.
It's not immediately obvious that this is a safe place to call SendError. What does it do? Is it synchronous? Does it schedule something? It doesn't depend on anything established by InitializeFeatures?
There was a problem hiding this comment.
It's a synchronous call, only state it needs from InitializeFeature is the request ID which we pass in. Would you like some sort of comment here mentioning it's a synchronous call to native code?
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/10816106964 |
|
@BrennanConroy backporting to release/8.0 failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Http.Sys: Clean up Request parsing errors
Using index info to reconstruct a base tree...
M src/Servers/HttpSys/src/RequestProcessing/Request.cs
M src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs
M src/Servers/HttpSys/test/FunctionalTests/Listener/RequestTests.cs
M src/Servers/HttpSys/test/FunctionalTests/RequestTests.cs
M src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs
M src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Servers/HttpSys/test/FunctionalTests/ResponseSendFileTests.cs
Auto-merging src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs
Auto-merging src/Servers/HttpSys/test/FunctionalTests/RequestTests.cs
Auto-merging src/Servers/HttpSys/test/FunctionalTests/Listener/RequestTests.cs
CONFLICT (content): Merge conflict in src/Servers/HttpSys/test/FunctionalTests/Listener/RequestTests.cs
Auto-merging src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs
CONFLICT (content): Merge conflict in src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs
Auto-merging src/Servers/HttpSys/src/RequestProcessing/Request.cs
CONFLICT (content): Merge conflict in src/Servers/HttpSys/src/RequestProcessing/Request.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Http.Sys: Clean up Request parsing errors
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@BrennanConroy an error occurred while backporting to release/8.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
|
@BrennanConroy ready to merge? |
Http.Sys: Clean up Request parsing errors
Description
There are a few errors logged in the event that request processing fails. Additionally, there isn't a clean error on the client side when this occurs.
This change catches the errors earlier, logs a Debug level log, and sends an Http 400 response.
Customer Impact
Better client-side error and removes potential Error level logs on the server.
Regression?
Regressed in 6.0
Risk
The change does not affect happy path scenarios and is limited to improved exception handling for non-happy path scenarios. Test added to verify new behavior. Lots of manual testing was done with individual parts of the change to make sure everything worked well.
Verification
Packaging changes reviewed?