runtime: [API Proposal] QuicError.AcceptConnectionFailed

Background and motivation

Context: #75115

QuicListener.AcceptConnectionAsync can currently throw in the following cases:

  • QuicException with QuicError.OperationAborted when the listener is disposed while there is a pending AcceptConnectionAsync call
  • ObjectDisposedException if called after the listener was disposed
  • AuthenticationException when the TLS handshake fails (e.g. due to the certificate being rejected)
  • ArgumentException when the options returned from QuicListenerOptions.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)

Most upvoted comments

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)