Repository navigation
[API Proposal] QuicError.AcceptConnectionFailed #75184
Description
Background and motivation
Context: #75115
QuicListener.AcceptConnectionAsync can currently throw in the following cases:
QuicExceptionwithQuicError.OperationAbortedwhen the listener is disposed while there is a pendingAcceptConnectionAsynccallObjectDisposedExceptionif called after the listener was disposedAuthenticationExceptionwhen the TLS handshake fails (e.g. due to the certificate being rejected)ArgumentExceptionwhen the options returned fromQuicListenerOptions.ConnectionOptionsCallbackare 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;
}
}
Activity
karelz commentedon Sep 7, 2022
LGTM, thanks @rzikm! @CarnaViire can you please take a look? cc @ManickaP if she will have time today.
JamesNK commentedon Sep 7, 2022
Correct me if I’m wrong but I didn’t see a QuicException ctor overload with an inner exception.
JamesNK commentedon Sep 7, 2022
I don’t think it changes anything, but a 6th exception is probably OperationCanceledException if the CT passed to the method is canceled.
rzikm commentedon Sep 7, 2022
Good catch! I forgot we removed that overload in the final proposal, I have added it in the proposal
JamesNK commentedon Sep 7, 2022
The exception is thrown internally. You could use an internal ctor. Up to you on whether to make it public or not.
rzikm commentedon Sep 7, 2022
When we had API Proposal for QuicException, there was pushback against having internal constructors, so I think we should stick to that
CarnaViire commentedon Sep 7, 2022
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)
rzikm commentedon Sep 7, 2022
I could not find a better name, so I am open to suggestions :)
karelz commentedon Sep 8, 2022
Triage: Right now we have last couple of days to make these kind of changes and even that is border-line meeting the bar for 7.0.
Given that there are more concerns on usability from @JamesNK we should rather properly rethink what we want long term and do the right thing in 8.0 (as a technical breaking change of this particular API).
Moving to 8.0.
ManickaP commentedon Sep 12, 2022
I like @CarnaViire suggestion with
UserCallbackError.JamesNK commentedon Sep 12, 2022
AuthenticationException can come from the client rejecting the server certificate. Ie a non callback cause.
ManickaP commentedon Sep 12, 2022
I haven't read the description properly. I don't think we should wrap those three, just the last one. And if we decide to wrap
AuthenticationExceptionandArgumentExceptionwe should reconsider what we throw fromQuicConnection.ConnectAsyncsince those two methods throw analogous exceptions.ManickaP commentedon Jan 19, 2023
Triage: we should compare with
SslStreamand other relevant similar APIs (SslStreamdoesn't wrap) and decide whether to do anything about this or not. That should be done 8.0.rzikm commentedon Jun 23, 2023
@wfurt Can we close this proposal? Since the scenario is going to be addressed by #87259?
wfurt commentedon Jun 23, 2023
yes, closing in favor of #87259. I'll re-open #75115 to track parts that do not need API changes.