Flatten exception handling #2313
Conversation
benaadams
force-pushed
the
exception-handling
branch
from
12e9375 to
1ae14d2
Compare
|
@benaadams is this more of an experiment/wip? |
Not really; its a proper PR 😄 Currently It also generates a chunky statemachine struct to lift the values across the exception handling and extra stuff in the method; each object reference requiring a GC memory barrier assign.
This PR extracts the main loop into a different method, that only deals with the loop; away from the extra processing that only happens once per connection; also simplifies the exception handling in the loop. After this change the statemachine for the main loop is only gets two And the statemachine looses 6 wrap fields (3 object refs); though gains one Exception field; so the state machine struct is reduced by 28 bytes per await and 2 GC memory barriers.
Also the main process requests loop is has more clarity of focus and is easier to understand (and 28 lines smaller) as it is only dealing with processing requests in a loop; rather than additionally handling connection cleanup and failure (which is handled in the outer method) |
|
@benaadams thanks that clarifies quite a bit! |
|
I wouldn't merge this unless we verified that adding another state machine doesn't regress the performance. We had big issues with the original Http2 refactoring doing that. |
Second statemachine is created once and executed once per connection, was paying close attention 😄 |
|
I see, it's ProcessRequests(), I thought this was per request but it's still per connection. We'll do a sanity test run. |
|
@benaadams can you rebase? |
3 nested try blocks with 3 finallies in same function O_o
benaadams
force-pushed
the
exception-handling
branch
from
26a3da7 to
ef9ed2c
Compare
|
Done |
| try | ||
| { | ||
| // Finish reading the request body in case the app did not. | ||
| await messageBody.ConsumeAsync(); |
There was a problem hiding this comment.
How confident are we that this is the only thing that can throw after invoking the application and verifying the response body length?
Personally, I'm fairly confident this is the case, but I want to make extra sure that we dispose the HttpContext in all the common scenarios.
There was a problem hiding this comment.
Was thinking FireOnStarting and FireOnCompleted if the user code they executed threw; but they don't let exceptions escape; so should be ok?
There was a problem hiding this comment.
Yep. I think this change is safe. I just wanted an extra pair of eyes.
There was a problem hiding this comment.
LGTM. I'd like @davidfowl and @Tratcher to also look at this carefully before merging though.
|
Thanks @halter73! |
| SetBadRequestState(badRequestException); | ||
| } | ||
|
|
||
| application.DisposeContext(httpContext, _applicationException); |
There was a problem hiding this comment.
This not being in a finally makes me nervous, @halter73 I'm hoping our tests are complete here.
There was a problem hiding this comment.
Only thing would be if await ProduceEnd could throw a BadRequestException
There was a problem hiding this comment.
Seems like it could? What happens if you set a utf8 response header before the response starts then you throw from the application?
There was a problem hiding this comment.
They are checked when they are set and would throw then in the application. When they are written out they use PipelineExtensions.WriteAsciiNoValidation
There was a problem hiding this comment.
Taking of which EncodeAsciiCharsToBytes looks like it could go faster :)
There was a problem hiding this comment.
I don't think ProduceEnd should ever throw.
| } | ||
| } | ||
| } | ||
| await ProcessRequests(application); | ||
| } | ||
| catch (BadHttpRequestException ex) |
There was a problem hiding this comment.
Can this still happen? At this level all error are connection oriented right?
There was a problem hiding this comment.
Parsing headers and request line can throw a BadHttpRequestException which gets caught here
There was a problem hiding this comment.
Is before any request has started so it catches here and tears down the connection
3 nested try blocks with 3 finallys (4 total) and 7 catches in same function O_o
Simplifies the request processing loop function by separating it from the connection cleanup, and reducing the blocks to a single try around the user code call.