Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement Error::source for h2::Error. #462

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Apr 30, 2020

This makes it easier to detect an IO error at the bottom of an error
cause chain (e.g. behind a hyper::Error).

Also, don't implement std::error::Error for the private error types,
to avoid confusion.

This makes it easier to detect an IO error at the bottom of an error
cause chain (e.g. behind a `hyper::Error`).

Also, don't implement `std::error::Error` for the private error types,
to avoid confusion.
@LucioFranco LucioFranco requested a review from seanmonstar May 4, 2020 16:19
@alanhe
Copy link

alanhe commented Jan 27, 2022

Can we push this forward?

In my case, when a client crashes, the server gets an error similar to this.

// dbg!(&source);

 &source = hyper::Error(
    Body,
    Error {
        kind: Io(
            Kind(
                BrokenPipe,
            ),
        ),
    },
)

I want to match the error kind to record the event, but recursively downcasting the error to std::io::Error couldn't work because h2::Error does not implement std::error::Error#source().

This is what I end up with:

fn match_broken_pipe_error(err: &(dyn std::error::Error + 'static)) -> bool {
    if let Some(e) = err.downcast_ref::<h2::Error>() {
        if let Some(e) = e.get_io() {
            if e.kind() == std::io::ErrorKind::BrokenPipe {
                return true;
            }
        }
    }

    if let Some(source) = err.source() {
        match_broken_pipe_error(source)
    } else {
        false
    }
}

The code is much more awkward.

@kaifastromai
Copy link

oof...any update on this? Ran into this myself

@mkatychev
Copy link

mkatychev commented Mar 18, 2024

This is still an issue esp while matching on a stream disconnect in tonic: hyperium/tonic#1579
@seanmonstar what is needed to move this PR forward, happy to start a separate feature branch? This will still affect grpc disonnects since tonic uses h2 0.3 (so it would have to be backported as well).

@goffrie goffrie requested a review from seanmonstar April 29, 2024 18:00
@goffrie
Copy link
Contributor Author

goffrie commented Apr 29, 2024

I've updated the PR for recent master if anyone would like to review this again :)

@mkatychev
Copy link

mkatychev commented Sep 24, 2024

@seanmonstar could we get some feedback on this PR? This would greatly simplify some tonic error handling cases (especially in disambiguating client disconnections):
https://github.com/hyperium/tonic//blob/f7090c24813041dc62052894dee119520d603d88/examples/src/streaming/server.rs#L23-L29

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants