Mask errors in Ember server runConnection
#7157
Conversation
|
Cancellation is weird and makes reasoning even trickier. Prefer masking to that. |
|
I understand that you are not sold on this. In general I am against error masking myself. I think one of the problems with error handling in http servers in general is that there exist error states that can happen due to clients behaving in a bad way. In this case interrupting connections, but I remember error logging for invalid user agent headers from the akka-http days. For me as someone who runs services which get lots of traffic from the internet: error logging in these cases is painful because it is very noisy and I can do nothing about it other than filtering them myself in some hacky way or another. In my perfect world it would be easy to distinguish between "errors that are of interest" and "noisy stuff because http clients are weird" but I don't think that is really achievable or maintainable. I would believe that this change makes my world better even if it is not perfect. |
| .onError { case err => | ||
| onWriteFailure(request, resp, err) |
There was a problem hiding this comment.
I think the new code is more concise and easier to read and I prefer it over the old code.
But is't it functionally identical to the old code? Exceptions/errors were caught (attempt) and then re-thrown (raiseError) while the good path (Right) was just forwarded.
There was a problem hiding this comment.
Yes, good question. This is just a refactoring, the semantics should be identical.
The only semantic change in this PR is the .mask I added below.
| @@ -507,6 +505,7 @@ private[server] object ServerHelpers extends ServerHelpersPlatform { | |||
| } | |||
| .takeWhile(_.headers.get[Connection].exists(_.hasKeepAlive)) | |||
| .drain | |||
| .mask | |||
There was a problem hiding this comment.
So masking the Stream suppresses the errors and allows the Stream to successfully end without letting errors bubble up. I think this is a good idea. It makes the Stream more robust.
Fixes #7149.
Ok, so I'm not entirely sold on this change. Masking errors seems like a world of pain when things go wrong in unexpected ways. Here's my thinking:
all errors should already be reported through side-channels. So letting them leak to the unhandled error reporting just results in duplicate logging.
Random unhandled errors popping up is very unpopular. This issue is not the first time we've gotten complaints.
Mysterious error on post request: java.io.IOException: Broken pipe #7149
I did have one other implementation idea: after reporting an error through a side-channel, we could self-cancel the current fiber. Then, there would be no need to mask errors, and any errors that did appear would be legit. However, cancelation is rather drastic (unrecoverable) and may have unintended effects. Also
ErrorandCanceledare distinct pathways e.g. inonFinalizeCaseso it could be confusing for that as well.