Add non-throwing parser path for bad request handling#65256
Add non-throwing parser path for bad request handling#65256adityamandaleeka merged 11 commits intodotnet:mainfrom
Conversation
adityamandaleeka
requested review from
BrennanConroy,
JamesNK and
halter73
as code owners
github-actions
bot
added
the
area-networking
There was a problem hiding this comment.
Pull request overview
This pull request introduces a non-throwing code path for HTTP/1.1 request parsing to improve performance when handling malformed requests. Instead of relying on exception handling for bad requests (which is expensive), the parser now returns a structured result indicating success, incomplete, or error states.
Changes:
- Added
HttpParseResultstruct to represent parsing outcomes without exceptions - Implemented non-throwing parser methods (
TryParseRequestLine,TryParseHeaders) that return parse results - Modified
Http1Connection.TryParseRequestto use the non-throwing path and only create exceptions when needed - Added tests to verify error offset/length extraction works correctly with multi-segment buffers
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Servers/Kestrel/Core/src/Internal/Http/IHttpParser.cs | Adds HttpParseResult struct for non-throwing parse operations |
| src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs | Implements TryParseRequestLine and TryParseHeaders methods with error offset tracking, refactors existing throwing methods to call non-throwing versions |
| src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs | Adds TryParseRequestNoThrow and CreateBadRequestException methods, updates TryParseRequest to use non-throwing path |
| src/Servers/Kestrel/Core/test/HttpParserTests.cs | Adds tests for multi-segment buffer error detail extraction and error offset calculation with consumed bytes |
| src/Servers/Kestrel/Core/test/Http1/Http1ConnectionTests.cs | Updates test to expect BadHttpRequestException instead of InvalidOperationException for invalid headers |
adityamandaleeka
force-pushed
the
nonthrow_parser
branch
from
914ef9e to
a099102
Compare
|
What is the impact on successful requests? |
There's no impact on valid requests. Updated original comment to clarify that as well. |
adityamandaleeka
force-pushed
the
nonthrow_parser
branch
from
de85eb2 to
e081175
Compare
BrennanConroy
left a comment
BrennanConroy
left a comment
There was a problem hiding this comment.
Lots of comments not copied to the Try* methods, could we add some/most/all of them back?
Only looked through Http1Connection, will continue on HttpParser
| { | ||
| // Record OtherError for unexpected exceptions that escape the parser | ||
| KestrelMetrics.AddConnectionEndReason(MetricsContext, ConnectionEndReason.OtherError); | ||
| throw; |
There was a problem hiding this comment.
Should this also set the bad request state, end connection, then return instead of throwing?
There was a problem hiding this comment.
Maybe... but I think we should keep this as a rethrow. This catch block is for truly unexpected exceptions (OOM, etc.) which we can't "handle" and send a bad request exception, or InvalidOperationException from invalid header characters (null/CR/LF). The outer catch in ProcessRequestsAsync will log them appropriately. We could potentially convert InvalidOperationException to a bad request, but that would be a behavioral change - the current behavior of logging and closing the connection is reasonable.
|
|
||
| // Extract error detail from buffer if available, but only when logging is enabled | ||
| // to avoid leaking internal details in production. | ||
| if (Log.IsEnabled(LogLevel.Information) && |
There was a problem hiding this comment.
Some of our exception logic uses HttpParser._showErrorDetails and some of it uses LogLevel.Information. Seems like we're losing that differentiator.
| return false; | ||
| } | ||
|
|
||
| public bool TakeStartLine(ref SequenceReader<byte> reader) |
There was a problem hiding this comment.
Delete method? Also delete TakeMessageHeaders? (will need to update Http1ChunkedEncodingMessageBody)
There was a problem hiding this comment.
Will do this as followup once this PR is in.
5 participants
Introduces a non-throwing code path for HTTP/1.1 request parsing. Instead of using try/catch to handle malformed requests, the parser now returns a result struct indicating success, incomplete, or error states.
Exception handling is expensive, and in scenarios with many malformed requests (port scanning, malicious traffic, misconfigured clients), throwing
BadHttpRequestExceptionon every parse failure added unnecessary overhead. This eliminates exceptions from the normal bad-request handling path.In my testing using a client sending bad requests, throughput is up ~20% on an Azure Linux VM. When pinning to 2 cores on my physical machine (to make it CPU bound), I can see RPS increase by over 40%, and a perf trace shows that the time spent in exception handling goes from ~10% to ~0.2%. There's no impact on valid requests.