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. |
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.