Repository navigation
[API Proposal]: udate QuicError values #87259
Description
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.
Activity
rzikm commentedon Jun 8, 2023
It seems slightly confusing that the new enum value is not used in the API usage fragment.
wfurt commentedon Jun 8, 2023
That is partially by design. Back to discussion in #75115 all
QuicExceptionswill be retryable e.g. there is no need to handle specific error codes. I updated the example to log theErrorCodefor diagnostic reasons.bartonjs commentedon Jun 22, 2023
Video
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 commentedon Jun 22, 2023
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 commentedon Jun 23, 2023
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).