QuicListener.AcceptConnectionAsync rethrowing exceptions

[JamesNK]

QuicListener.AcceptConnectionAsync throws in the following scenarios (that I know of):

1. QuicException with QuicError.OperationAborted. Happens when the listener is disposed while waiting for a connection.
2. ObjectDisposedException. Happens when AcceptConnectionAsync is called after disposing the listener.
3. AuthenticationException. Happens when the client and server fail to negotiate a connection, e.g. server has an invalid certificate and the client rejects it.
4. ArgumentException. Happens when the options returned from the callback are invalid. e.g. a server certificate isn't configured in the callback.
5. Any exception is thrown in the callback. Happens when an exception is thrown in the callback and is then rethrown from AcceptConnectionAsync.

I think 5 is very broad. There should be a way to disambiguate a rethrown exception from important exceptions that are used to signal the health of the listener. e.g. someone could accidentally throw an ODE in the callback, which the code that calls the listener interprets as the listener is shut down and to stop listening for connections.

I think the exception from 5, and maybe also 3 and 4, should be wrapped in another exception that says an attempt to accept a connection failed. The inner exception then provides more detail.

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

Issue Details

---

QuicListener.AcceptConnectionAsync throws in the following scenarios (that I know of):

1. QuicException with QuicError.OperationAborted. Happens when the listener is disposed while waiting for a connection.
2. ObjectDisposedException. Happens when AcceptConnectionAsync is called after disposing the listener.
3. AuthenticationException. Happens when the client and server fail to negotiate a connection, e.g. server has an invalid certificate and the client rejects it.
4. ArgumentException. Happens when `options returned from the callback are invalid. e.g. a server certificate isn't configured in the callback.
5. Any exception is thrown in the callback. Happens when an exception is thrown in the callback and is then rethrown from AcceptConnectionAsync.

I think 5 is very broad. There should be a way to disambiguate a rethrown exception from important exceptions that are used to signal the health of the listener. e.g. someone could accidentally throw an ODE in the callback, which the code that calls the listener interprets as the listener is shut down and to stop listening for connections.

I think the exception from 5, and maybe also 3 and 4, should be wrapped in another exception that says an attempt to accept a connection failed. The inner exception then provides more detail.

Author:	JamesNK
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

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

Issue Details

---

QuicListener.AcceptConnectionAsync throws in the following scenarios (that I know of):

1. QuicException with QuicError.OperationAborted. Happens when the listener is disposed while waiting for a connection.
2. ObjectDisposedException. Happens when AcceptConnectionAsync is called after disposing the listener.
3. AuthenticationException. Happens when the client and server fail to negotiate a connection, e.g. server has an invalid certificate and the client rejects it.
4. ArgumentException. Happens when the options returned from the callback are invalid. e.g. a server certificate isn't configured in the callback.
5. Any exception is thrown in the callback. Happens when an exception is thrown in the callback and is then rethrown from AcceptConnectionAsync.

I think 5 is very broad. There should be a way to disambiguate a rethrown exception from important exceptions that are used to signal the health of the listener. e.g. someone could accidentally throw an ODE in the callback, which the code that calls the listener interprets as the listener is shut down and to stop listening for connections.

I think the exception from 5, and maybe also 3 and 4, should be wrapped in another exception that says an attempt to accept a connection failed. The inner exception then provides more detail.

Author:	JamesNK
Assignees:	-
Labels:	untriaged, area-System.Net.Quic
Milestone:	-

[karelz]

Split from discussion: dotnet/aspnetcore#43730 (comment)

It sounds reasonable to me. @ManickaP @rzikm @CarnaViire @wfurt any thoughts?
If we agree, we should try to do it in 7.0 to prevent breaking changes in 8.0 ... even though QUIC APIs are preview, so we will be allowed to make them.

[JamesNK]

Currently, aspnetcore has to assume any exception thrown from AcceptConnectionAsync can be retried, unless it is OperationAborted or ODE.

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

This feels a little dangerous. What happens if a bug causes AcceptConnectionAsync to break and throw for another reason? We'd be stuck in this loop, throwing errors, catching errors, and burning CPU.

It would be nice if there was a known exception type (or QuicError type) for failed accepting of connections to check for. Unexpected exceptions could then be logged and exit this loop.

[karelz]

Triage: We should look at how we deal with these things in HttpClient and SslStream callbacks.

• HttpClient wraps it with HttpRequestException (per @MihaZupan).

Note: We might still decide to wrap exceptions in QUIC APIs.

[karelz]

Duplicate of #75184