Add API to get RST_STREAM value from HTTP exceptions

[JamesNK]

Part of User Story: dotnet/core#5493

Background and Motivation

gRPC specifies that RST_STREAM error codes are mapped to gRPC error codes - https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#errors

I believe that today the RST_STREAM value is only exposed in exception message descriptions. The only way to get this value is to parse exception messages, which is pretty awful for the usual reasons.

There should be an exception property with the RST_STREAM error code so it can be used programmatically.

Proposed API

Make this value publicly accessible:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2ProtocolException.cs

Line 28 in faf91e0

internal Http2ProtocolErrorCode ProtocolError { get; }

I don't feel strongly that it needs to be an enum. public int ProtocolError { get; } would also be fine.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[geoffkizer]

I agree that we should expose this value.

There's a similar issue with error codes from GOAWAY -- this is similar to RST_STREAM, except it affects the whole connection and kills all streams. As part of exposing the RST_STREAM error code, we should also consider exposing the GOAWAY error code if/when appropriate.

We should also try to do this in such a way that it works for HTTP3 as well, since there is a similar error code capability in HTTP3. Not sure to what extent the error codes are aligned, though...

[JamesNK]

HTTP/3 error codes are distinct https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-11.2.3

[geoffkizer]

HTTP/3 error codes are distinct

Of course they are :(

And they can be 62 bits long, too.

I guess this argues for having something like long ProtocolError, with values that are version specific. But that raises the issue of how you know which protocol version was actually used...

[JamesNK]

It looks like distinct ranges for each protocol.

Http2: <255
Http3: >256

[karelz]

As discussed earlier, we won't be able to make this in 6.0. Moving to Future.

[JamesNK]

Behold the hack I added to gRPC: https://github.com/grpc/grpc-dotnet/blob/ac3b3f4309b29b0c4fb9048da961e8c054ee955a/src/Grpc.Net.Client/Internal/GrpcProtocolHelpers.cs#L422-L561 😄

It's not good but it works. Obviously, it would be great to improve with a real API.

[antonfirsov]

I also agree that we should create a unified proposal that covers RST_STREAM, GOAWAY, and the HTTP/3 cases. What makes this tricky is the way the exception/error information is nested today:

• Http2: HttpRequestException --> IOException --> Http2StreamException/Http2ConnectionException --> ProtocolError
• Http3 : HttpRequestException --> QuicStreamAbortedException --> ErrorCode (At least in cases I was able to figure out.)

One possible solution is to expose the exception types Http2StreamException, Http2ConnectionException and QuicStreamAbortedException and leave it for the user to dispatch the nested error information. If we go down this way, I wonder if the way we nest the exceptions today is an intentional behavioral contract, or is it something we can change it in order to simplify things and make sure HttpRequestException.InnerException is an instance of the 3 possible "protocol exceptions" in case they are present?

If simplification is not possible, we can create a shortcut as a separate property on HttpRequestException:

public class HttpRequestException {
    // Http2StreamException, Http2ConnectionException or QuicStreamAbortedException:
    public Exception? ProtocolException { get; } 
}

A different approach would be to expose protocol error code right on HttpRequestException, however the gRPC workaround in #43239 (comment) indicates that we can't get away with a single long ErrorCode property, because gRPC needs to distinguish between stream and connection errors, only mapping the stream errors. @JamesNK correct me if I'm wrong on this.

A possible API:

public class HttpRequestException {
    long? ProtocolErrorCode { get; }
    ProtocolErrorSource? ProtocolErrorSource { get; }
}

public enum ProtocolErrorSource {
    Http2Stream,
    Http2Connection,
    QuicStream
}

@dotnet/ncl thoughts?

[JamesNK]

however the gRPC workaround in #43239 (comment) indicates that we can't get away with a single long ErrorCode property, because gRPC needs to distinguish between stream and connection errors, only mapping the stream errors. @JamesNK correct me if I'm wrong on this.

I don't think we need a way to distinguish.

When I experimented with how to get error codes, which resulted in the exception message parsing code I linked to, all the thrown errors I found were either Http2StreamException or QuicStreamAbortedException. I didn't find any examples where I cared about Http2ConnectionException. I'm guessing because I cared about catching errors when a request is in progress

When is Http2ConnectionException thrown?

tldr long? ProtocolErrorCode { get; } by itself is probably fine.

[antonfirsov]

When is Http2ConnectionException thrown?

1. Whenever we receive a GOAWAY frame, with the protocol error code in the received frame
2. Whenever we encounter a protocol violation that requires us to terminate the connection. (Reporting a protocol error code here is quite an arbitrary behavior IMHO since we don't send back GOAWAY/RST_STREAM to the server in this case.)

@JamesNK since your ResolveRpcExceptionStatusCode method is currently filtering on Http2StreamException only, I assumed you want to avoid mapping protocol error codes when these events occur, but maybe I was wrong?

[JamesNK]

I care about those as well. Those scenarios never happened in my tests so I never needed to check for Http2ConnectionException.

10 points to SocketsHttpHandler and ASP.NET Core for not violating the HTTP/2 protocol 😄

[JamesNK]

@karelz This was moved from 6.0 to future last year, but it was never added to the 7.0 milestone. Should it be?

[antonfirsov]

@JamesNK here it is in 7.0 😄

@dotnet/ncl can we think of cases when a user would need to see which of the many possible scenarios has resulted in HttpRequestException reporting a protocol error, or are we fine simply adding a field with the error code?

[karelz]

@JamesNK sorry, that was an oversight on our side. Thanks @antonfirsov for fixing it!

[JamesNK]

@JamesNK here it is in 7.0 😄

@dotnet/ncl can we think of cases when a user would need to see which of the many possible scenarios has resulted in HttpRequestException reporting a protocol error, or are we fine simply adding a field with the error code?

Are there places where a protocol error occurred but HttpRequestExveption isn’t the exception thrown?

[JamesNK]

Here is at least one case where IOException is thrown because a protocol error: #62228

In this situation, HttpResponseMessage has successfully returned from SendAsync and the error happens when reading from the response body.

[antonfirsov]

Well, makes it trickier, opening up up a bunch of options:

• View HttpClient should report protocol error code when the server closes the connection #62228 as an entirely separate issue (maybe it's enough to report a more verbose exception message?)
• Subclass IOException adding an error code, which we should always embed as HttpRequestException.InnerException
• Subclass IOException adding an error code, but also repeat the error code property on HttpRequestException

Edit:

• +1 option from team discussion: Embed HttpResponseMessage as IOException's InnerException to solve HttpClient should report protocol error code when the server closes the connection #62228. (This is not unprecedental)

A lot depends on the answer on my question in #62228 (comment)

[antonfirsov]

Long story short, it looks like we can get away with a simple API addition for this particular issue:

public class HttpRequestException {
    long? ProtocolErrorCode { get; }
}

We can extend this with more error details later if we want. We can utilize it to solve #62228.

[antonfirsov]

Team discussion:
It might be actually better to carry this information in an IOException subclass, maybe the same one for both HTTP2 and QUIC.
Passing this over to @rzikm who is responsible for the QUIC exception API design. I can help with the implementation when API-s are approved.

[ManickaP]

This is also related to decision whether to derive QuicException from IOException: #56954
In such case, we would have HttpRequestException with IOException as the inner exception in both H/2 and H/3 cases. From that point of view, it might make more sense to reuse the similarity and design a common vehicle for the protocol error code. Moreover, this would also be useful for #62228 without sticking HttpRequestException inside IOException (reverse order to what HttpClient does).

This line of thinking depends on the decision we'll make in #56954. @rzikm, could you please take this into account when you're thinking about QUIC exceptions?

Either way, as a fallback we can always do what #43239 (comment) above ^ suggests.

[antonfirsov]

Closing this in favor of #70684