feat(errors): add Error::is_shutdown #2745
Conversation
|
I want to be able to not log shutdown-related errors, any objection @seanmonstar? |
| @@ -56,7 +56,6 @@ pub(super) enum Kind { | |||
| /// The body write was aborted. | |||
| BodyWriteAborted, | |||
| /// Error calling AsyncWrite::shutdown() | |||
| #[cfg(feature = "http1")] | |||
There was a problem hiding this comment.
Why change the feature-gate here? Would it be better to clarify the helper?
| @@ -201,6 +200,11 @@ impl Error { | |||
| self.find_source::<TimedOut>().is_some() | |||
| } | |||
|
|
|||
| /// Returns true if the error was caused by a shutdown. | |||
| pub fn is_shutdown(&self) -> bool { | |||
There was a problem hiding this comment.
e.g.
| pub fn is_shutdown(&self) -> bool { | |
| #[cfg(feature = "http1")] | |
| pub fn is_http1_shutdown(&self) -> bool { |
Either that, or this should probably also handle h2 goaways?
There was a problem hiding this comment.
Hm, that does make me think the name could confuse people. It's not a graceful shutdown, it was an error that occurred when calling AsyncWrite::shutdown.
It originally was just meant to provide additional context on top of the IO error. Is that context useful on its own?
There was a problem hiding this comment.
I don't understand what you mean. There is nothing we can do about those errors in the wild (most often they are a "broken pipe" error), so we don't want to log them at all, so we need to discriminate them to ignore them.
There was a problem hiding this comment.
So, it does seem like the context is useful to you. Now the question is, is the name confusing, such that someone might think it's a "graceful shutdown" error received over HTTP/2? Is that a bad thing? Should we hijack the name to make both cases be a is_shutdown error?
There was a problem hiding this comment.
h2 errors currently are all represented as Kind::H2 right? Maybe I should rename that method to is_http1_shutdown then.
No description provided.