Improve error handling#76
Conversation
Motivation: The `serveInsecureHTTP1_1` and `serveSecureUpgrade` methods listen for incoming activity on the server channel and dispatch connections/requests to child tasks in order to process concurrently. However, these methods use a `withThrowingDiscardingTaskGroup`, and call many `throw`ing functions from within the task group. Since a `discarding` task group implicitly cancels *all* child tasks as soon as an error is encountered in *any* child task, we should carefully reason about and handle all errors that could arise within the task group. Modifications: - Removed the redundant `throw` keyword from the `handleRequestChannel` method declaration. - Refactored `serveInsecureHTTP1_1` and `serveSecureUpgrade` to handle errors more explicitly without propagating them up to the parent task group. As a result, it was then possible to replace `withThrowingDiscardingTaskGroup` with `withDiscardingTaskGroup`. Result: Better error handling.
| handler: handler | ||
| ) | ||
| try await serverChannel.executeThenClose { inbound in | ||
| let inboundConnectionIterationError = await withDiscardingTaskGroup { group -> (any Error)? in |
There was a problem hiding this comment.
Why can't we just use withThrowingDiscardingTaskGroup here? The only error we're throwing is the one coming from iterating inbound.
There was a problem hiding this comment.
This is primarily to make it explicit that if you call a throwing function from within the body of the task group, the error must be handled directly rather than letting it propagate upwards and resulting in active tasks in the group being cancelled.
Currently iterating on inbound is the only source of a potential error, but if this was a withThrowingDiscardingTaskGroup then in the future we could inadvertently introduce a call to a throwing function without the compiler warning us.
There was a problem hiding this comment.
Could we add some comment here (and in the other func) explaining this rationale? I am not convinced just doing things this way would prevent a future change from changing the behaviour unless it's more documented.
fabianfett
left a comment
fabianfett
left a comment
There was a problem hiding this comment.
This looks much better. We might want to improve logging a bit, but we can do that in a follow up.
| case .head(let request): | ||
| httpRequest = request | ||
| case .body: | ||
| self.logger.debug("Unexpectedly received body on connection. Closing now") |
There was a problem hiding this comment.
Can we have something like a connectionID here? Can easily be done in a follow up...
| outbound.finish() | ||
| return | ||
| case .end: | ||
| self.logger.debug("Unexpectedly received end on connection. Closing now") |
There was a problem hiding this comment.
Can we have something like a connectionID here? Can easily be done in a follow up...
| } catch { | ||
| self.logger.debug("Error thrown while handling connection: \(error)") | ||
| // TODO: We need to send a response head here potentially | ||
| self.logger.error("Error thrown while handling connection", metadata: ["error": "\(error)"]) |
|
Thanks. There's currently no mechanism to extract connection / stream information out of the underlying HTTP/1.1 or HTTP/2 connection channel. We'd need to add some methods to do that. I can address that in a follow-up PR. |
3 participants
Motivation:
The
serveInsecureHTTP1_1andserveSecureUpgrademethods listen for incoming activity on the server channel and dispatch connections/requests to child tasks in order to process concurrently. However, these methods use awithThrowingDiscardingTaskGroup, and call many throwing functions from within the task group.Since a
discardingtask group implicitly cancels all child tasks as soon as an error is encountered in any child task, we should carefully reason about and handle all errors that could arise within the task group.Modifications:
Addressed comments raised in Fix request error handling #71. This included removing the redundant
throwkeyword from thehandleRequestChanneldeclaration.Refactored
serveInsecureHTTP1_1andserveSecureUpgrade(and associated methods) to handle errors more explicitly without propagating them up to the parent task group. As a result, it was then possible to replacewithThrowingDiscardingTaskGroupwithwithDiscardingTaskGroup.Result:
Better error handling.