QuicListener.AcceptConnectionAsync rethrowing exceptions

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

QuicException with QuicError.OperationAborted. Happens when the listener is disposed while waiting for a connection.
ObjectDisposedException. Happens when AcceptConnectionAsync is called after disposing the listener.
AuthenticationException. Happens when the client and server fail to negotiate a connection, e.g. server has an invalid certificate and the client rejects it.
ArgumentException. Happens when the options returned from the callback are invalid. e.g. a server certificate isn't configured in the callback.
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.

@ManickaP

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.

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.

@MihaZupan

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.

Duplicate of #75184

@wfurt

@wfurt why did you reopen it?
@ManickaP thinks it should be closed with your last 2 PRs merged yesterday.

mostly. We still should look at 1. IMHO e.g. Disposing listener should throw ODE. The changes addressed $5 e.g. we wrap callback exceptions now.

It's a one-liner in here https://source.dot.net/#System.Net.Quic/System/Net/Quic/QuicListener.cs,376
And the same thing applies to QuicConnection and AcceptStream: https://source.dot.net/#System.Net.Quic/System/Net/Quic/QuicConnection.cs,645
==> 2 liner change

@ManickaP

do you want to do it and write test @ManickaP? I will not get to it this week.

Yep, that's why I assigned myself. I already have something in progress.

Navigation Menu

Repository navigation

QuicListener.AcceptConnectionAsync rethrowing exceptions #75115

2 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

QuicListener.AcceptConnectionAsync rethrowing exceptions #75115

Description

Activity

karelz commented on Sep 6, 2022

JamesNK commented on Sep 6, 2022

karelz commented on Sep 6, 2022

karelz commented on Sep 7, 2022

2 remaining items

karelz commented on Jul 18, 2023

wfurt commented on Jul 18, 2023

ManickaP commented on Jul 18, 2023

wfurt commented on Jul 19, 2023

ManickaP commented on Jul 20, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions