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

[API Proposal]: Exposing HTTP/2 and HTTP/3 protocol error codes #70684

Closed
antonfirsov opened this issue Jun 13, 2022 · 7 comments · Fixed by #72095
Closed

[API Proposal]: Exposing HTTP/2 and HTTP/3 protocol error codes #70684

antonfirsov opened this issue Jun 13, 2022 · 7 comments · Fixed by #72095
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Jun 13, 2022

Exposing HTTP/2 and HTTP/3 protocol error codes from SocketsHttpHandler

This proposal builds on our QuicException proposal.

Background and Motivation

GRPC (and potentially other lower-level users) need a way to get the underlying HTTP/2 or HTTP/3 error codes in case a protocol error occurs.

The error code should be observable not only when an error occurs while calling an HttpClient method (#43239), but also when invoking Stream API-s on the response's content read stream (#62228).

Proposed design

  • Define a new exception type HttpProtocolException, and embed it as HttpRequestException.InnerException
  • Throw ProtocolException directly from HttpResponse content read streams
namespace System.Net.Http;

public sealed class HttpProtocolException : IOException
{
    public HttpProtocolException(HttpProtocolError errorCode, string message, Exception? innerException);

    public HttpProtocolError ErrorCode { get; }
}

// Map enum values to H2 and H3 error codes
// H3 error codes can be 62 bit long
public enum HttpProtocolError : long 
{
    // Camel-cased names taken directly from the HTTP/2 spec
    // https://datatracker.ietf.org/doc/html/rfc7540#section-7
    NoError = 0x0,
    ProtocolError,
    InternalError,
    FlowControlError,
    SettingsTimeout,
    StreamClosed,
    FrameSizeError,
    RefusedStream,
    Cancel,
    CompressionError,
    EnhanceYourCalm,
    InadequateSecurity,
    Http11Required,

    // Camel-cased names taken directly from the HTTP/3 spec
    // https://datatracker.ietf.org/doc/html/rfc9114/#section-8.1
    H3NoError = 0x100,
    H3GeneralProtocolError,
    H3InternalERror,
    H3StreamCreationError,
    H3ClosedCriticalStream,
    H3FrameUnexpected,
    H3FrameError,
    H3ExcessiveLoad,
    H3IdError,
    H3SettingsError,
    H3MissingSettings,
    H3RequestRejected,
    H3RequestCancelled,
    H3RequestIncomplete,
    H3MessageError,
    H3ConnectError,
    H3VersionFallback
}

Edit: Added a public constructor.

Notes

  • No public constructors, because we prefer to do the bare minimum to deliver a feature. This means that WinHttpHandler and user-made HttpClientHandlers won't be able to throw HttpProtocolException for now. We can consider public constructors later, if necessary.
  • We throw ProtocolError when a connection or stream is aborted, or when we detect a protocol violation ourselves
  • ErrorCode >= 256 means HTTP/3
  • In case of HTTP/3 connection or stream errors, we are embedding QuicException as ProtocolException.InnerException
  • We don't throw HttpProtocolException for transport-level errors

API Usage

Over HttpClient

using var client = new HttpClient();

try
{
    var response = await client.GetStringAsync(".");
}
catch (HttpRequestException ex) when (ex.InnerException is ProtocolException protocolException)
{
    Console.WriteLine("HTTP error code: " + protocolException.ProtocolErrorCode)
    if (protocolException.InnerException is QuicException quicException)
        Console.WriteLine("Underlying QUIC error: " + quicException.Message);
}
catch (HttpRequestException ex) when (ex.InnerException is AuthenticationException authenticationException) {
    // TLS authentication error. Can originate from QUIC, we map some errors to high-level .NET exceptions
}

Over response stream

using var client = new HttpClient();
using var response = await client.GetAsync(".", HttpCompletionOption.ResponseHeadersRead);
using var responseStream = await response.Content.ReadAsStreamAsync();
using var memoryStream = new MemoryStream();

try {
    await responseStream.CopyToAsync(memoryStream);
}
catch (ProtocolException protocolException) {
    // HTTP(2|3) protocol error
}
catch (QuicException quicException) {
    // QUIC transport error
}
catch (IOException exception) when (ex.InnerException is SocketException socketException) {
    // TCP transport error
}

Alternative designs

Parallel independent exception types for HTTP/2 and HTTP/3

public class Http2ProtocolException : IOException
{
    public int ErrorCode { get; }
}

// Use System.Net.Quic.QuicException for HTTP/3
// public class QuicException : IOException { }
Usage
using var client = new HttpClient();

try
{
    var response = await client.GetStringAsync("foo.bar");
}
// HTTP/2
catch (HttpRequestException ex) when (ex.InnerException is Http2ProtocolException protocolException)
{
    Console.WriteLine(protocolException.ProtocolErrorCode)
}
// HTTP/3
catch (HttpRequestException ex) when (ex.InnerException is QuicException quicException)
{
    Console.WriteLine(quicException.ApplicationErrorCode);
}

Do not define an enum, use untyped long error codes

public sealed class HttpProtocolException : IOException
{
    public long ErrorCode { get; }
}

Risks

  • HttpProtocolError enum: implementations may send unspecified error codes. If spec evolves in the future, we will have to introduce new error codes. In both cases field would contain a value outside the enum range, so users would have a workaround.
  • Unless I'm missing something, this is not a breaking change, since HttpProtocolException is an IOException.
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 13, 2022
@antonfirsov antonfirsov self-assigned this Jun 13, 2022
@ghost
Copy link

ghost commented Jun 13, 2022

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

Issue Details

Exposing HTTP/2 and HTTP/3 protocol error details from SocketsHttpHandler

This proposal builds on our QuicException proposal.

Background and Motivation

GRPC (and potentially other lower-level users) need a way to get the underlying HTTP/2 or HTTP/3 error codes in case a protocol error occurs.

The error code should be observable not only when an error occurs while calling an HttpClient method (#43239), but also when invoking Stream API-s on the response's content read stream (#62228).

Proposed design

  • Define a new exception type HttpProtocolException, and embed it as HttpRequestException.InnerException
  • Throw ProtocolException directly from HttpResponse content read streams
namespace System.Net.Http;

public sealed class HttpProtocolException : IOException
{
    public long ErrorCode { get; }
}

Notes

  • No public constructors, because we prefer to do the bare minimum to deliver a feature. This means that WinHttpHandler and user-made HttpClientHandlers won't be able to throw HttpProtocolException for now. We can consider public constructors later, if necessary.
  • We throw ProtocolError when a connection or stream is aborted, or when we detect a protocol violation ourselves
  • ErrorCode >= 256 means HTTP/3
  • In case of HTTP/3 connection or stream errors, we are embedding QuicException as ProtocolException.InnerException
  • We don't throw HttpProtocolException for transport-level errors

API Usage

Over HttpClient

using var client = new HttpClient();

try
{
    var response = await client.GetStringAsync(".");
}
catch (HttpRequestException ex) when (ex.InnerException is ProtocolException protocolException)
{
    Console.WriteLine("HTTP error code: " + protocolException.ProtocolErrorCode)
    if (protocolException.InnerException is QuicException quicException)
        Console.WriteLine("Underlying QUIC error: " + quicException.Message);
}
catch (HttpRequestException ex) when (ex.InnerException is AuthenticationException authenticationException) {
    // TLS authentication error. Can originate from QUIC, we map some errors to high-level .NET exceptions
}

Over response stream

using var client = new HttpClient();
using var response = await client.GetAsync(".", HttpCompletionOption.ResponseHeadersRead);
using var responseStream = await response.Content.ReadAsStreamAsync();
using var memoryStream = new MemoryStream();

try {
    await responseStream.CopyToAsync(memoryStream);
}
catch (ProtocolException protocolException) {
    // HTTP(2|3) protocol error
}
catch (QuicException quicException) {
    // QUIC transport error
}
catch (IOException exception) when (ex.InnerException is SocketException socketException) {
    // TCP transport error
}

Alternative designs

Parallel independent exception types for HTTP/2 and HTTP/3

public class Http2ProtocolException : IOException
{
    public Http2ProtocolException(long protocolErrorCode, string message, Exception innerException) { }

    public int ErrorCode { get; }
}

// Use System.Net.Quic.QuicException for HTTP/3
// public class QuicException : IOException { }
Usage
using var client = new HttpClient();

try
{
    var response = await client.GetStringAsync("foo.bar");
}
// HTTP/2
catch (HttpRequestException ex) when (ex.InnerException is Http2ProtocolException protocolException)
{
    Console.WriteLine(protocolException.ProtocolErrorCode)
}
// HTTP/3
catch (HttpRequestException ex) when (ex.InnerException is QuicException quicException)
{
    Console.WriteLine(quicException.ApplicationErrorCode);
}

Risks

Low/none. Unless I'm missing something, this is not a breaking change, since HttpProtocolException is an IOException

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@antonfirsov antonfirsov added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed untriaged New issue has not been triaged by the area owner labels Jun 13, 2022
@antonfirsov antonfirsov added this to the 7.0.0 milestone Jun 13, 2022
@antonfirsov antonfirsov changed the title [API Proposal]: Exposing HTTP/2 and HTTP/3 protocol error details [API Proposal]: Exposing HTTP/2 and HTTP/3 protocol error codes Jun 13, 2022
@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Jun 13, 2022

No public constructors, because we prefer to do the bare minimum to deliver a feature.

Feels very weird to have an exception without public constructors to be honest.

@antonfirsov
Copy link
Member Author

Might feel weird but not sure if it breaks any of our design guidelines. Without having use-cases to construct the exception externally, we couldn't decide how general we should go with a public constructor. Same applies for QuicException in #70669.

@stephentoub
Copy link
Member

Might feel weird but not sure if it breaks any of our design guidelines.

It does. @bartonjs can comment.

@chwarr
Copy link
Contributor

chwarr commented Jun 14, 2022

Unit tests would benefit from a public constructor so that code like this, lightly tweaked from the motivating example, can be tested:

try
{
    await SomeClientMethodThatHasBeenMockedOrArrangedToThrowAProtocolException();
}
catch (HttpRequestException ex) when (ex.InnerException is ProtocolException protocolException)
{
    // without a public ctor, exercising this code path in a unit test is annoying
}

@antonfirsov
Copy link
Member Author

antonfirsov commented Jun 20, 2022

I updated the proposal with a public constructor HttpProtocolException(string? message, long errorCode, Exception? innerException).

@rzikm #70684 (comment) is also relevant for #70669

@terrajobst
Copy link
Member

terrajobst commented Jun 28, 2022

Video

  • Let's start with a simpler design that just exposes the code as a long
    • We can add enums for the different protocols and derived exceptions later
namespace System.Net.Http;

public sealed class HttpProtocolException : IOException
{
    public HttpProtocolException(long errorCode, string message, Exception? innerException);
    public long ErrorCode { get; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 28, 2022
antonfirsov added a commit that referenced this issue Jun 30, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants