runtime: [API Proposal] QuicError.AcceptConnectionFailed
Background and motivation
Context: #75115
QuicListener.AcceptConnectionAsync
can currently throw in the following cases:
QuicException
withQuicError.OperationAborted
when the listener is disposed while there is a pendingAcceptConnectionAsync
callObjectDisposedException
if called after the listener was disposedAuthenticationException
when the TLS handshake fails (e.g. due to the certificate being rejected)ArgumentException
when the options returned fromQuicListenerOptions.ConnectionOptionsCallback
are not valid (e.g. server certificate is missing)- any exception that was thrown from the
QuicListenerOptions.ConnectionOptionsCallback
The last three cases are propagated outside to make it easier for users to diagnose why they are unable to accept a connection. Without it, would be difficult for users to debug why they are unable to connect.
The very last case is especially problematic because it invokes a user-provided callback and therefore it can throw any exception. Throwing e.g. ObjectDisposedException
from the callback could confuse code that uses QuicListener
(such as ASP.NET Core).
The proposed solution is to wrap the last 3 cases listed above in a QuicException
with a new QuicError.AcceptConnectionFailed
value. This makes it easier for users to distinguish non-fatal exceptions after which AcceptConnectionAsync
can be called again.
Note: QUIC APIs are new in 7.0 and they are Preview – therefore such change is acceptable.
There is an existing precedent for wrapping exceptions from user callbacks. For example with HttpClient
, when an exception is thrown from certificate selection/validation callback, the exception is wrapped in HttpRequestException
.
API Proposal
public enum QuicError
{
// existing members omitted
+ /// <summary>
+ /// An error occurred when accepting a new connection, such as failed handshake or invalid <see cref="QuicConnectionOptions" /> was returned from <see cref="QuicListenerOptions.ConnectionOptionsCallback" />.
+ /// </summary>
+ AcceptConnectionFailed,
}
public sealed class QuicException : IOException
{
// existing members omitted
// existing ctor:
public QuicException(QuicError error, long? applicationErrorCode, string message) {...}
+ public QuicException(QuicError error, long? applicationErrorCode, string message, Exception innerException) {...}
}
}
API Usage
while (true)
{
try
{
var connection = await _listener.AcceptConnectionAsync();
Task.Run(() => ProcessConnection(connection));
}
catch (QuicException ex) when (ex.QuicError == QuicError.AcceptConnectionFailed)
{
// transient failure, retry and wait for next connection attempt.
}
catch (Exception e) when (e is ObjectDisposedException || (e is QuicException ex && ex.QuicError == QuicError.OperationAborted))
{
// listener was disposed, stop accepting connections.
return;
}
}
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Reactions: 1
- Comments: 16 (16 by maintainers)
LGTM in general. I feel like I would like it more if it was not specifically bound to AcceptConnection though, more like “UserCallbackError” or something in the same meaning, so it could be reused for the same purpose in other callbacks. But I think we only have a callback for Accept now, so maybe it’s ok.
(Also, I feel like “AcceptConnectionFailed” might be too general name, so it might not be clear for the user, that it wouldn’t include all other cases in which Accept is failing, like AuthenticationException)