Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Act on TLS errors only #1652

Closed
rolftimmermans opened this issue Sep 13, 2018 · 4 comments
Closed

Act on TLS errors only #1652

rolftimmermans opened this issue Sep 13, 2018 · 4 comments

Comments

@rolftimmermans
Copy link

rolftimmermans commented Sep 13, 2018

I am using hyper & hyper-tls to connect to HTTPS endpoints.

I want to do something specific if the endpoint has an invalid TLS certificate or configuration. It seems there is no way to distinguish between all these kinds of errors right now?

I think the error will internally be represented as Kind::Connect, but I'm not entirely sure. There seems to be a TODO to expose is_connect(). This might be what I need.

However, I really wish Kind were public, or maybe a less granular public enum that allows me to distinguish various classes of errors (for example: UserError, ParseError, ConnectError, Closed, Timeout). All the is_* methods on Error are not ergonomic (quickly turns into many if/else-if statements) and are too easily incomplete when new types of errors are added. I see it in other libraries but I severely dislike it and I never manage to correctly and thoroughly handle all errors this way. Thoughts?

@seanmonstar
Copy link
Member

Hm, is_connect probably should be added, that's true! You can also inspect the underlying error using Error::cause2.

I know matching on enum variants in these cases is much easier, the Error type used to be an enum. However, it made it difficult to add new variants, and impossible to refactor the representation of an error. For those reasons, it is now an opaque struct with some ways to inspect. See #1131 for more discussion around that.

@rolftimmermans
Copy link
Author

Thanks for the clarification. I have added a comment in #1131 with my thoughts on the matter. I would love a reconsideration to add enums back in the Error, it really is so much easier and clearer for API users.

I will attempt to use cause2() for now to solve my problem, thanks for the suggestion.

@rolftimmermans
Copy link
Author

rolftimmermans commented Sep 14, 2018

OK, this is what I have now in my match arm.

The err is a hyper::Error wrapped with tokio::timer::timeout::Timeout.

Err(err) => {
    (|| {
        if let Some(inner) = err.into_inner() {
            if let Some(cause) = inner.cause2() {
                if let Some(io) = cause.downcast_ref::<io::Error>() {
                    if let Some(inner) = io.get_ref() {
                        if inner.is::<hyper_tls::Error>() {
                            return Response::invalid_certificate()
                        }
                    }
                }
            }
        }
        Response::origin_failure()
    })()
}

There is a lot to improve with Rust error handling. 😞

I hope that someday I can match like this:

Err(timer::Timeout::Inner(hyper::Error::Connect(_))) => {
    Response::invalid_certificate()
},
Err(_) => {
    Response::origin_failure()
},

@seanmonstar
Copy link
Member

I would love a reconsideration to add enums back in the Error

I expect the type itself to never make its representation public, since that was a hindrance before that prevented optimizations. Exposing a public Kind-like enum might happen, but it depends on #[non_exhaustive], and as the issue explains, building an enum that can gain new variants in a backwards-compatible way is difficult.

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

No branches or pull requests

2 participants