[API Proposal]: udate QuicError values

[wfurt]

Background and motivation

This is primarily driven by #75115 and #82262.

As noted in #75115, AcceptConnectionAsync can throw various exceptions and it is not obvious how they should be handled. For one, Quic also surfaces exceptions from user callback directly and that may make then indistinguishable from exceptions thrown by Quic itself - ODE may be particularly trick as leaked ODE exception from the callback may stop the server as it may look like whole listener was disposed. To make that more obvious, this issue is proposing to add new enum value and wrap anything from user callbacks in separate QuicException. #75184 originally proposed new enum specific to accept. However, after some discussion the user callback remains only one problematic and it is also conceptually valid to 'ConnectAsync`.

The second part of this proposal is based on API review board feedback on #76435. We wanted to add QuicError.AddressNotAvailable but the feedback was that is looks just like SocketError. As part of #82262 we agreed to throw SocketException for conditions that are not specific to Quic protocol and are caused by network conditions or environment configuration. That make several existing QuicError values obsolete and they would be never used. The proposal is to remove them - this is breaking change but we feel it is best long term option given that Quic still has preview flag.

API Proposal

namespace System.Net.Quic
{

    public enum QuicError
    {
        // existing members omitted

+       /// <summary>
+       /// An error occurred in user provided callback.
+       /// </summary>
+      UserCallbackError,
    }
}

    public enum QuicError
    {
        // existing members omitted

-      AddressInUse,
-      InvalidAddress,
-      HostUnreachable, // ICMP error
    }

API Usage

from #75115 Kestrel fragment

while (true)
{
    try
    {
        var connection = await _listener.AcceptConnectionAsync();
        return connection;
    }
    catch (ObjectDisposedException)
    {
        return null;
    }
    catch (QuicException ex)
    {
        // retry 
        log($"Quic failure {ex.Message} {ex.ErrorCode}");
    }
    catch (ArgumentException)
    {
        // retry
    }
}

Alternative Designs

#75184 originally proposed to add QuicError.AcceptConnectionFailed.

For the removal, we may mark the extra values as obsolete or we can keep them unused.

Risks

The proposed removal is breaking change. I tried to search but I did not find code that would explicitly handle the transport error codes.

[ghost]

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

Issue Details

---

Background and motivation

This is primarily driven by #75115 and #82262.

As noted in #75115, AcceptConnectionAsync can throw various exceptions and it is not obvious how they should be handled. For one, Quic also surfaces exceptions from user callback directly and that may make then indistinguishable from exceptions thrown by Quic itself - ODE may be particularly trick as leaked ODE exception from the callback may stop the server as it may look like whole listener was disposed. To make that more obvious, this issue is proposing to add new enum value and wrap anything from user callbacks in separate QuicException. #75184 originally proposed new enum specific to accept. However, after some discussion the user callback remains only one problematic and it is also conceptually valid to 'ConnectAsync`.

The second part of this proposal is based on API review board feedback on #76435. We wanted to add QuicError.AddressNotAvailable but the feedback was that is looks just like SocketError. As part of #82262 we agreed to throw SocketException for conditions that are not specific to Quic protocol and are caused by network conditions or environment configuration. That make several existing QuicError values obsolete and they would be never used. The proposal is to remove them - this is breaking change but we feel it is best long term option given that Quic still has preview flag.

API Proposal

    public enum QuicError
    {
        // existing members omitted

+       /// <summary>
+       /// An error occurred in user provided callback.
+       /// </summary>
+      UserCallbackError,
    }

    public enum QuicError
    {
        // existing members omitted

-      AddressInUse,
-      InvalidAddress,
-      HostUnreachable, // ICM error
    }

API Usage

from #75115 Kestrel fragment

while (true)
{
    try
    {
        var connection = await _listener.AcceptConnectionAsync();
        return connection;
    }
    catch (ObjectDisposedException)
    {
        return null;
    }
    catch (QuicException ex)
    {
        // retry 
    }
    catch (ArgumentException)
    {
        // retry
    }
}

Alternative Designs

#75184 originally proposed to add QuicError.AcceptConnectionFailed.

For the removal, we may mark the extra values as obsolete or we can keep them unused.

Risks

The proposed removal is breaking change. I tried to search but I did not find code that would explicitly handle the transport error codes.

Author:	wfurt
Assignees:	wfurt
Labels:	api-suggestion, area-System.Net.Quic
Milestone:	8.0.0

[rzikm]

It seems slightly confusing that the new enum value is not used in the API usage fragment.

[wfurt]

It seems slightly confusing that the new enum value is not used in the API usage fragment.

That is partially by design. Back to discussion in #75115 all QuicExceptions will be retryable e.g. there is no need to handle specific error codes. I updated the example to log the ErrorCode for diagnostic reasons.

[ghost]

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

[bartonjs]

Video

• The "User" prefix on UserCallbackError doesn't seem significant, so changed to just "CallbackError"
• The removed items should be documented as a preview breaking change.

namespace System.Net.Quic
{
    public enum QuicError
    {
        // existing members omitted

+       /// <summary>
+       /// An error occurred in user provided callback.
+       /// </summary>
+       CallbackError,

-       AddressInUse,
-       InvalidAddress,
-       HostUnreachable, // ICMP error
    }
}

[halter73]

Since we said that only the Quic implementation has reason to construct a QuicException when comparing this to #75184, should we remove the existing public constructor on QuicException?

@rzikm Did you have a particular reason for wanting the new constructor to be public?

[rzikm]

The idea was to enable unit-testing scenarios (i.e. enable mocking code construct QuicExceptions with the same innerExceptions as are thrown from System.Net.Quic).