Redesign the h2::Error type

[nox]

Yea, I think we'd need to redesign the h2::Error type some, so that it can include if it's a stream error, or a connection error (GOAWAY). Then we'd be better equipped to answer that programmatically.

Originally posted by @seanmonstar in hyperium/hyper#2500 (comment)

AFAICT we want DynConnection::error and ConnectionInner::error to at the very least be an Option<frame::GoAway> instead of a plain Option<Reason>.

[nox]

Once those two error fields are made to store a frame::GoAway, we can patch Connection::take_error to also take the last stream id out of the frame, but that raises more questions:

• Do we ignore the last stream id if (ours, theirs) is (_, Reason::NO_ERROR)?
• How do we represent the stream id in proto::Error, i.e. do we make a new variant or do we add a field to the Proto one?
• How do we propagate that to crate::Error? Do we make new methods like get_io and take_io?

Also, instead of just passing around the last stream id from the GOAWAY frame, maybe we should just let the user be able to reach the entire frame::GoAway value in case the other peer sent interesting debug information with it.

[seanmonstar]

Yes, for the connection error variant, I've wanted to add the debug_data field as well, since some things send useful hints in that.

I've wanted to redesign the Error type internals in h2 for a while, with these thoughts in mind:

• Be able to distinguish between a stream and connection error.
• Probably include a way to determine if the error was received (via a RST_STREAM or GOAWAY) by the remote, or if h2 "triggered" it because of some violation.
• Ditch the Io variant, and just interpret that as a connection error with the io::Error in the cause.
• I'd probably propose still keeping a single h2::Error type, and have the variants be internal kinds, with accessors to check for them.
• With a connection error, while we'd want to expose the last_stream_id (I think), it's possible for someone to wrongly use that. It will contain the last stream ID that was received by the creator of the GOAWAY. That means if the client were to notice a connection error problem, and send a GOAWAY, and the server had never sent any push promises, then the last_stream_id would be StreamId(0). If you then saw that and did a simple error.last_stream_id() < req_stream_id, they could easily assume all the requests could be resent.

[nox]

Currently there is no way to distinguish between errors generated by h2 itself and remote stream/connection errors, correct?

[nox]

There are other error-related issues: #463 and #470.

[seanmonstar]

Oh yes, that's another point I forgot to include, is it an error the remote told us about, or is it an error "we" noticed about the remote doing something wrong.

[nox]

I've started working on this and there are 4 different kinds of errors we need to disambiguate:

• the remote peer sent a reset or a goaway (remote error);
• the local peer sent a reset or a goaway (local error);
• the h2 crate rejected something the local peer did (user error);
• the h2 crate rejected something the remote peer did (I want to call that a state error).

[seanmonstar]

I'd like to offer another axis to how we could categorize errors: stream, connection, and user. With a getter to determine if we received the error over the wire, or generated it locally. I'd also like to consider keeping the source for the errors (many are just logged as a tracing event). One possibility is to include the frame::Reset or frame::GoAway as the source, to indicate whether we received it or not.

[seanmonstar]

For example, here's some sample fmt::Debug outputs:

We received a GOAWAY:

h2::Error {
    kind: Connection {
        error_code: ENHANCE_YOUR_CALM,
    },
    source: frame::GoAway {
        error_code: ENHANCE_YOUR_CALM,
        last_stream_id: 87,
        debug_data: b"too_many_pings",
    },
}

We found an illegal SETTINGS frame on the wire

h2::Error {
    kind: Connection {
        error_code: PROTOCOL_ERROR,
        last_stream_id: 0,
        debug_data: b"bro do you even http?",
    },
    source: FrameDecodeError::InvalidSettingsFrame,
}

We received a DATA frame without enough window capacity

h2::Error {
    kind: Stream {
        reason: FLOW_CONTROL_ERROR,
        stream_id: 55,
    },
    source: DataFrameExceedsStreamWindow,
}

We got a RST_STREAM frame.

h2::Error {
    kind: Stream {
        reason: STREAM_CLOSED,
        stream_id: 135,
    },
    source: frame::Reset,
}

[nox]

Why is there no last_stream_id nor debug_data in the first one?

How do you represent IO errors?

[nox]

I'm also not sure how you can check whether the error came from the wire or not from those payloads.

[nox]

Another issue: what happens to SendStream::poll_reset? We probs want to change the return type right? A bare Reason doesn't tell us much about what happened when the stream got reset.

[seanmonstar]

Why is there no last_stream_id nor debug_data in the first one?

Eh, there could be! Seemed like duplicate information that was already in the source. But 🤷

How do you represent IO errors?

Probably as a kind: Connection { error_code: INTERNAL_ERROR } with the IO error as the source. It's basically a connection level error at that point...

I'm also not sure how you can check whether the error came from the wire or not from those payloads.

impl Error {
    // method naming is hard
    pub fn is_from_peer(&self) -> bool {
        self.source().map(|e| e.is::<frame::Reset>() || e.is::<frame::GoAway>()).unwrap_or(false)
    }
}

what happens to SendStream::poll_reset? We probs want to change the return type right? A bare Reason doesn't tell us much about what happened when the stream got reset.

Does it need to change? I haven't thought about it really. We already know the stream, so stream ID, doesn't help much. I guess returning an Error instead would allow a user to know that the stream is being killed because of a reset or a goaway?

[nox]

Eh, there could be! Seemed like duplicate information that was already in the source. But 🤷

My point was that you provided two samples using the same type but with different fields in both, which is quite confusing to me.

[nox]

Your plan also does not let the user check whether the error came from the h2 crate itself because the other peer didn't properly follow the protocol.

[nox]

Probably as a kind: Connection { error_code: INTERNAL_ERROR } with the IO error as the source. It's basically a connection level error at that point...

There are some I/O errors we expect, but we most probably want to know when an origin server replied back with INTERNAL_ERROR, conflating those explicit INTERNAL_ERROR responses with an I/O error seem like it would be confusing to log unexpected errors.

[seanmonstar]

Your plan also does not let the user check whether the error came from the h2 crate itself because the other peer didn't properly follow the protocol.

The is_from_peer() method I posted should allow that.