Skip to content

feat(errors): add Error::is_shutdown - #2745

Closed
nox wants to merge 1 commit into
masterfrom
error-is-shutdown
Closed

feat(errors): add Error::is_shutdown#2745
nox wants to merge 1 commit into
masterfrom
error-is-shutdown

Conversation

@nox

@nox nox commented Jan 31, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@nox
nox force-pushed the error-is-shutdown branch from f97b5e8 to 1d61a5c Compare January 31, 2022 10:20
@nox

nox commented Feb 4, 2022

Copy link
Copy Markdown
Contributor Author

I want to be able to not log shutdown-related errors, any objection @seanmonstar?

@nox
nox requested a review from seanmonstar February 4, 2022 11:15
Comment thread src/error.rs
/// The body write was aborted.
BodyWriteAborted,
/// Error calling AsyncWrite::shutdown()
#[cfg(feature = "http1")]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the feature-gate here? Would it be better to clarify the helper?

Comment thread src/error.rs
}

/// Returns true if the error was caused by a shutdown.
pub fn is_shutdown(&self) -> bool {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

Suggested change
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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h2 errors currently are all represented as Kind::H2 right? Maybe I should rename that method to is_http1_shutdown then.

@seanmonstar

Copy link
Copy Markdown
Member

I'm thinking we close this?

@nox

nox commented Dec 6, 2022

Copy link
Copy Markdown
Contributor Author

I'm thinking we close this?

Is there a replacement for it?

joshka added a commit to joshka/hyper that referenced this pull request Mar 23, 2025
Benchmarking tools often cause connections to be ungracefully shutdown.
This makes it possible to filter these errors out.

Replaces hyperium#2745
@joshka

joshka commented Mar 23, 2025

Copy link
Copy Markdown
Contributor

Is there a replacement for it?

I just raised #3863 as a possible replacement.

joshka added a commit to joshka/hyper that referenced this pull request Mar 25, 2025
Benchmarking tools often cause connections to be ungracefully shutdown.
This makes it possible to filter these errors out.

Replaces hyperium#2745
joshka added a commit to joshka/hyper that referenced this pull request Mar 26, 2025
Benchmarking tools often cause connections to be ungracefully shutdown.
This makes it possible to filter these errors out.

Replaces hyperium#2745
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants