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.
| } | ||
| } | ||
| try await serverChannel.executeThenClose { inbound in | ||
| let inboundConnectionIterationError = await withDiscardingTaskGroup { connectionGroup -> (any Error)? in |
There was a problem hiding this comment.
Similarly, I don't see why this is clearer than just using a throwing task group and throwing where appropriate?
2 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.