Fix request error handling#71
Conversation
| @@ -289,7 +289,7 @@ | |||
| } catch { | |||
| self.logger.debug("Error thrown while handling connection: \(error)") | |||
There was a problem hiding this comment.
not a change made in this pr, but we should never use string interpolation for log messages. the correct pattern is to put the error message into metadata.
| self.logger.debug("Error thrown while handling connection: \(error)") | |
| self.logger.debug("Error thrown while handling connection", metadata: ["error": "\(error)"]) |
There was a problem hiding this comment.
I think this was already changed in a different PR - in any case there's an open issue to audit these log messages.
| self.logger.debug("Error thrown while handling connection: \(error)") | ||
| // TODO: We need to send a response head here potentially | ||
| throw error | ||
| try? await channel.channel.close() |
There was a problem hiding this comment.
what has happened to the OG error? that is just swallowed here? I think we should at least add a comment, why we swallow it here.
There was a problem hiding this comment.
also I think, we should log the error, if we can't close the connection.
There was a problem hiding this comment.
The change in this PR was a bit of a workaround to avoid the whole server coming down when an error is thrown. I will work on handling this properly - that's part of what the TODO right above is. I'll make sure we also log the error when closing.
| handler: some HTTPServerRequestHandler<RequestConcludingReader, ResponseConcludingWriter> | ||
| ) async throws { |
There was a problem hiding this comment.
this should remove the throws keyword here, which will make usage of TaskGroups much more explicit.
There was a problem hiding this comment.
Yeah, this I missed, we should remove it - mind opening a small follow up @aryan-25 ?
There was a problem hiding this comment.
Yes, I'm going to shortly create a PR to address @fabianfett's comments.
|
Feedback from @fabianfett is addressed in #76. |
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 throwing 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: - Addressed comments raised in #71. This included removing the redundant `throw` keyword from the `handleRequestChannel` declaration. - Refactored `serveInsecureHTTP1_1` and `serveSecureUpgrade` (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 replace `withThrowingDiscardingTaskGroup` with `withDiscardingTaskGroup`. Result: Better error handling.
3 participants
Motivation:
We currently propagate errors out of the
handleRequestChannelmethod.For the plaintext HTTP/1.1 transport, the
handleRequestChannelmethod is called from within awithThrowingDiscardingTaskGroupwithout any error catching.Since
withThrowingDiscardingTaskGroupcancels itself whenever any of the tasks spawned inside of it throws, an error thrown in one connection brings the entire server down. We should fix this.Modifications:
handleRequestChannelto close the channel when an error is caught instead of propagating the error upwards.Result:
An error thrown in a plaintext HTTP/1.1 connection no longer results in the parent task group being cancelled.