The error types should be refactored

[nox]

trafficstars

I have many issues with the various Error types we define and how HttpsConnector ultimately just uses BoxError for its Service<Uri> error type, I'll try to summarize them here.

First, the BoxError, this makes it impossible to consume any more specific error type, as downcasting with the Error trait is always by reference.

Second, I keep confusing myself with boring::Error and boring::ssl::Error.

Third, the boring::ssl::HandshakeError is not fun to use for multiple reasons:

• the underlying I/O errors can be in two separate variants (Failure and WouldBlock);
• it stores a MidHandshakeSslStream<S> even in the Failure variant, even though you are obviously not supposed to do anything anymore with that stream given the handshake failed;
• given that second bullet point, it also doesn't make much sense for the MidHandshakeSslStream<S> struct to keep around the error that interrupted the handshake, as that was expected and you just want to resume it;
• it has a variant SetupFailure which feels out of place to me, shouldn't setup errors be completely contained in builders etc?

Fourth, tokio_boring::HandshakeError is as useful as BoxError given it doesn't let us access the boring::ssl::HandshakeError it wraps directly, so that's one more layer of hoops to go through to find, say, I/O errors.

Fifth, even if tokio_boring::HandshakeError let us access its inner boring::ssl::HandshakeError, that would still be a bit of a bother to use, as we know tokio_boring would never return a WouldBlock error but we would still need an arm for that in our code.

[inikulin]

@nox let's do these improvements

[nox]

As a first step, I want to change:

pub enum HandshakeError<S> {
    /// Setup failed.
    SetupFailure(ErrorStack),
    /// The handshake failed.
    Failure(MidHandshakeSslStream<S>),
    /// The handshake encountered a `WouldBlock` error midway through.
    ///
    /// This error will never be returned for blocking streams.
    WouldBlock(MidHandshakeSslStream<S>),
}

pub struct MidHandshakeSslStream<S> {
    stream: SslStream<S>,
    error: Error,
}

to

pub enum HandshakeResult<S> {
    Done(SslStream<S>),
    WouldBlock(MidHandshakeSslStream<S>),
    Failed(StreamError<S>),
}

pub struct StreamError<S> {
    stream: S,
    error: Error,
}

pub struct MidHandshakeSslStream<S> {
    stream: SslStream<S>,
}

We would return HandshakeResult<S> and we would store setup errors just as the non-setup ones in Error.

---

Another way I thought of is:

pub enum HandshakeStream<S> {
    Done(SslStream<S>),
    Mid(MidHandshakeSslStream<S>),
}

pub struct StreamError<S> {
    stream: S,
    error: Error,
}

And we would just return Result<HandshakeStream<S>, StreamError<S>>.

[nox]

Actually the second way with HandshakeStream<S> is more pleasant to use so I'm currently doing that.

[nox]

For now, it looks like this: https://github.com/cloudflare/boring/compare/master...nox:errors

[nox]

Actually, I realise now that both ways are bad, because in the end, the implementations of Read and Write want to return an io::Error. I now want to reverse everything and make StreamError store an io::Error, and put all SSL-specific errors into an I/O error whose kind is io::ErrorKind::Other.

[nox]

I was staring at those two snippets thinking this would hinder my plan of making everything be io::Error:

https://github.com/cloudflare/boring/blob/2667b0fdee830acbfbbf4051fb6933c1f0166895/boring/src/ssl/mod.rs#L3131

https://github.com/cloudflare/boring/blob/2667b0fdee830acbfbbf4051fb6933c1f0166895/boring/src/ssl/mod.rs#L3147

(Note for some reason that both check WANT_READ even though the second one is in the write method.)

Apparently we specifically check for this error code with no explicit I/O error because SSL_read can emit SSL_ERROR_WANT_READ even if the BIO layer didn't actually return WouldBlock, so we loop to be sure we made some progress that involved doing I/O calls.

After reading the docs a bit more, this seems not necessary because we set AUTO_RETRY anyway which makes BoringSSL loop on its own.

https://github.com/cloudflare/boring/blob/0c9166dd746cad006d36c9497094fb4843511e9e/boring/src/ssl/connector.rs#L37-L38

So AFAIK there is no need to distinguish "BoringSSL returned WANT_READ" from "the BIO layer used by BoringSSL returned WANT_READ".

[inikulin]

Yeah, HandshakeStream variant looks more appealing

[jyn514]

@nox did you ever finish working on this? Or do you have a WIP branch I could adapt? Would love to improve the error API.

[nox]

#142 removes HandshakeError and its infamous SetupFailure variant.