tokio-boring: Add additional accessors to `HandshakeError` #57

hawkw · 2021-11-04T17:35:37Z

Currently, the HandshakeError type in tokio-boring provides a
std::error::Error implementation and an as_io_error method that
returns an Option<&std::io::Error>, as the only ways to access the
potentially underlying error. There are a couple of cases where this is
somewhat insufficient:

If a user has an owned HandshakeError, and needs an owned
io::Error, they have to create a new one, using the
HandshakeError's cause and message, like this:
```
let err: HandshakeError<_> = // ...;
if let Some(io_err) = err.as_io_error() {
    return Err(io::Error::new(io_err.cause(), io_err.to_string()));
}
// ...
```
This seems like kind of a shame, since it allocates two additional
times (for the String and then again for the io::Error itself),
and deallocates the original io::Error at the end of the scope. None
of this should be necessary, since the HandshakeError in this case
is owned and the original io::Error could be returned.
HandshakeError only implements fmt::Debug and fmt::Display when
the wrapped I/O stream it's generic over implements Debug. This
means that bounding the S type with Debug is necessary for
HandshakeError to implement the Error trait. In some cases,
introducing a Debug bound on a generic I/O type is kind of a
heavyweight requirement. Therefore, it would be nice to have a way to
extract a type that always implements Error from the
HandshakeError, in cases where the returned I/O stream is just going
to be discarded if the handshake failed.

This branch introduces new functions on HandshakeError:

HandshakeError::as_ssl_error, which is analogous to
HandshakeError::as_io_error but returning an
Option<&boring::error::ErrorStack> instead of an I/O error.
HandshakeError::into_io_error and HandshakeError::into_ssl_error,
which consume the error and return an Option<io::Error> or an
Option<ErrorStack>, respectively.

I noticed that some of the into_$TYPE methods on errors in the
boring crate return Results rather than Options so that the
original error can be reused if it couldn't be converted into the
requested type. However, this can't easily be done in
HandshakeError's into_io_error and into_ssl_error, because these
methods call MidHandshakeSslStream::into_error, and (since
MidHandshakeSslStream can only be constructed by the boring crate,
not in tokio-boring), we can't ever get the MidHandshakeSslStream
back...so we can't return the original Self type in this case.

Hopefully this should make it much easier to convert HandshakeErrors
into other error types as required in user code!

Signed-off-by: Eliza Weisman eliza@buoyant.io

Currently, the `HandshakeError` type in `tokio-boring` provides a `std::error::Error` implementation and an `as_io_error` method that returns an `Option<&std::io::Error>`, as the only ways to access the potentially underlying error. There are a couple of cases where this is somewhat insufficient: * If a user has an owned `HandshakeError`, and needs an owned `io::Error`, they have to create a *new* one, using the `HandshakeError`'s cause and message, like this: ```rust let err: HandshakeError<_> = // ...; if let Some(io_err) = err.as_io_error() { return Err(io::Error::new(io_err.cause(), io_err.to_string())); } // ... ``` This seems like kind of a shame, since it allocates two additional times (for the `String` and then again for the `io::Error` itself), and deallocates the original `io::Error` at the end of the scope. None of this *should* be necessary, since the `HandshakeError` in this case is owned and the original `io::Error` could be returned. * `HandshakeError` only implements `fmt::Debug` and `fmt::Display` when the wrapped I/O stream it's generic over implements `Debug`. This means that bounding the `S` type with `Debug` is necessary for `HandshakeError` to implement the `Error` trait. In some cases, introducing a `Debug` bound on a generic I/O type is kind of a heavyweight requirement. Therefore, it would be nice to have a way to extract a type that *always* implements `Error` from the `HandshakeError`, in cases where the returned I/O stream is just going to be discarded if the handshake failed. This branch introduces new functions on `HandshakeError`: * `HandshakeError::as_ssl_error`, which is analogous to `HandshakeError::as_io_error` but returning an `Option<&boring::error::ErrorStack>` instead of an I/O error. * `HandshakeError::into_io_error` and `HandshakeError::into_ssl_error`, which consume the error and return an `Option<io::Error>` or an `Option<ErrorStack>`, respectively. I noticed that some of the `into_$TYPE` methods on errors in the `boring` crate return `Result`s rather than `Option`s so that the original error can be reused if it couldn't be converted into the requested type. However, this can't easily be done in `HandshakeError`'s `into_io_error` and `into_ssl_error`, because these methods call `MidHandshakeSslStream::into_error`, and (since `MidHandshakeSslStream` can only be constructed by the `boring` crate, not in `tokio-boring`), we can't ever get the `MidHandshakeSslStream` _back_...so we can't return the original `Self` type in this case. Hopefully this should make it much easier to convert `HandshakeError`s into other error types as required in user code! Signed-off-by: Eliza Weisman <eliza@buoyant.io>

olix0r · 2021-11-04T18:22:28Z

tokio-boring/src/lib.rs

+
+    /// Consumes `self` and returns the inner I/O error by value, if this error
+    /// was caused by an I/O error.
+    pub fn into_io_error(self) -> Option<io::Error> {


FWIW I'm not sure that this is valuable for us... If it's not an Io error we lose the original error?

I guess we'd have to use something like if e.as_io_error().is_some() { e.into_io_error.unwrap() { ? that's pretty cumbersome

I thought about adding an is_io/is_ssl, but I wasn't sure if it was necessary given that it would just be as_io_error().is_some(). I could add that as well, though. IMO it's still less cumbersome than the current code we have, which calls as_io_error and creates a new io::Error using values from the one that's passed by reference.

Unfortunately, we can't return Results rather than Options so that the original error can be reused if it couldn't be converted into an io::Error, because we have to call MidHandshakeSslStream::into_error, and (since MidHandshakeSslStream can only be constructed by the boring crate, not in tokio-boring), we can't ever get the MidHandshakeSslStream back...so we can't return the original Self type in this case.

@olix0r okay, I've modified these to return Result<io::Error, Self> and Result<ErrorStack, Self> so you can now write code like

error .into_ssl_error() .map(Into::into) .or_else(|error| error.into_io_error().map(Into::into))

or similar

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

jyn514

Sorry for the long delay. It makes sense to me that the existing API is insufficient, and I think the type signatures for the new functions are fine. Do be aware though that we're considering larger changes to the error types soon: #20

jyn514 · 2022-02-01T19:19:56Z

tokio-boring/src/lib.rs

+            ssl::HandshakeError::Failure(s) => s.error().ssl_error(),
+            ssl::HandshakeError::SetupFailure(ref s) => Some(s),
+            _ => None,


Shouldn't this also handle HandshakeError::WouldBlock? Or is it impossible somehow for WouldBlock to be an SSL error?

Suggested change

ssl::HandshakeError::Failure(s) => s.error().ssl_error(),

ssl::HandshakeError::SetupFailure(ref s) => Some(s),

_ => None,

ssl::HandshakeError::Failure(s) | ssl::HandshakeError::WouldBlock(s) => s.error().ssl_error(),

ssl::HandshakeError::SetupFailure(ref s) => Some(s),

jyn514 · 2022-02-01T19:20:23Z

tokio-boring/src/lib.rs

+    /// that the error value can be reused.
+    pub fn into_io_error(self) -> Result<io::Error, Self> {
+        match self.0 {
+            ssl::HandshakeError::Failure(s) => {


Suggested change

ssl::HandshakeError::Failure(s) => {

ssl::HandshakeError::Failure(s) | ssl::HandshakeError::WouldBlock(s) => {

jyn514 · 2022-02-01T19:20:52Z

tokio-boring/src/lib.rs

+                    Err(Self(ssl::HandshakeError::Failure(s)))
+                }
+            }
+            _ => Err(self),


Suggested change

_ => Err(self),

ssl::HandshakeError::SetupFailure(_) => Err(self),

jyn514 · 2022-02-01T19:22:38Z

tokio-boring/src/lib.rs

+            // TODO(eliza): it's not great that we have to clone in this case,
+            // but `boring::ssl::Error` doesn't currently expose an
+            // `into_ssl_error` the way it does for `io::Error`s. We could add
+            // that in a separate PR, but adding that here would make
+            // `tokio-boring` depend on a new release of `boring`...
+            ssl::HandshakeError::Failure(s) => s
+                .error()
+                .ssl_error()
+                .cloned()
+                .ok_or_else(|| Self(ssl::HandshakeError::Failure(s))),


I'd rather add into_ssl_error for boring too, if you don't mind - you can change them both in the same PR.

Sure! I was trying to limit the API additions because I wasn't sure what would be preferred by the maintainers, but I'm happy to change that.

jyn514 · 2022-02-01T19:24:05Z

tokio-boring/src/lib.rs

+                    Ok(s.into_error().into_io_error().expect(
+                        "if `s.error().io_error().is_some()`, `into_io_error()` must succeed",
+                    ))


I think you could avoid the expect() here by adding a new MidHandshakeSslStream::from_parts function and using into_parts() above.

hawkw · 2022-02-08T17:38:01Z

Do be aware though that we're considering larger changes to the error types soon: #20

Hmm, if the intent is to make larger changes in the near future, is it worth moving forwards with the changes in this branch, or should I just try to make the changes proposed in #20?

jyn514 · 2022-02-08T17:53:05Z

should I just try to make the changes proposed in #20?

That would be really helpful! I'd love to hear if that new design fixes your issue.

nox · 2023-10-09T11:38:29Z

Btw #142 completely removes HandshakeError.

hawkw mentioned this pull request Nov 4, 2021

meshtls: Add a boring backend linkerd/linkerd2-proxy#1351

Merged

olix0r reviewed Nov 4, 2021

View reviewed changes

return Results from into_$ERROR methods

986a8ca

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

jyn514 reviewed Feb 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tokio-boring: Add additional accessors to `HandshakeError` #57

tokio-boring: Add additional accessors to `HandshakeError` #57

hawkw commented Nov 4, 2021

olix0r Nov 4, 2021

hawkw Nov 4, 2021 •

edited

Loading

hawkw Nov 5, 2021

olix0r Nov 5, 2021

jyn514 left a comment

jyn514 Feb 1, 2022

jyn514 Feb 1, 2022

jyn514 Feb 1, 2022

jyn514 Feb 1, 2022

hawkw Feb 1, 2022

jyn514 Feb 1, 2022

hawkw commented Feb 8, 2022

jyn514 commented Feb 8, 2022

nox commented Oct 9, 2023

	ssl::HandshakeError::Failure(s) => {
	ssl::HandshakeError::Failure(s) \| ssl::HandshakeError::WouldBlock(s) => {

	_ => Err(self),
	ssl::HandshakeError::SetupFailure(_) => Err(self),

tokio-boring: Add additional accessors to HandshakeError #57

Are you sure you want to change the base?

tokio-boring: Add additional accessors to HandshakeError #57

Conversation

hawkw commented Nov 4, 2021

Choose a reason for hiding this comment

hawkw Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Feb 8, 2022

jyn514 commented Feb 8, 2022

nox commented Oct 9, 2023

tokio-boring: Add additional accessors to `HandshakeError` #57

tokio-boring: Add additional accessors to `HandshakeError` #57

hawkw Nov 4, 2021 •

edited

Loading