Add non-throwing parser path for bad request handling #65256
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? |
| } | ||
| } | ||
|
|
||
| #pragma warning disable CS0618 // Type or member is obsolete |
There was a problem hiding this comment.
| #pragma warning disable CS0618 // Type or member is obsolete |
There was a problem hiding this comment.
This is needed because BadHttpRequestException is obsolete.
| #pragma warning restore CS0618 // Type or member is obsolete | ||
|
|
||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| private void OnBadRequest(ReadOnlySequence<byte> requestData, BadHttpRequestException ex) |
There was a problem hiding this comment.
looking into code more, I spotted that BadHttpRequestException is mostly used for flow-control, and we probably can:
-
eliminate
_requestRejectedExceptioncompletely (I did not see any usage, where we needException-> we only use field to determine if request is already rejected, or to get theex.StatusCode. -
dont even allocate the badRequestException at all, and rely on
ErrorReason
BadHttpRequestException being obsoleted is another hint to explore this
| /// <summary> | ||
| /// Non-throwing version of ParseRequest. Returns HttpParseResult instead of throwing. | ||
| /// </summary> | ||
| private HttpParseResult TryParseRequestNoThrow(ref SequenceReader<byte> reader) |
There was a problem hiding this comment.
nit: not sure we need to name it NoThrow. You are simply improving the code and once it is all non-throwing it will not add more context to the code
| // In this case, there is an ongoing request but the start line/header parsing has timed out, so send | ||
| // a 408 response. | ||
| KestrelBadHttpRequestException.Throw(RequestRejectionReason.RequestHeadersTimeout); | ||
| var ex = KestrelBadHttpRequestException.GetException(RequestRejectionReason.RequestHeadersTimeout); |
There was a problem hiding this comment.
We're losing the stack trace. Might want to use ExceptionDispatchInfo.SetCurrentStackTrace(ex) to preserve that without throwing.
There was a problem hiding this comment.
I don't think there's much value in that, given that the reason is already included. Using SetCurrentStackTrace would add stack walk overhead on each bad request for no benefit.
There's no impact on valid requests. Updated original comment to clarify that as well. |
4 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.