-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement QUIC abort exceptions, fix channel usage, and implement some tests. #32051
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ internal sealed class MsQuicConnection : QuicConnectionProvider | |
private bool _disposed; | ||
private bool _connected; | ||
private MsQuicSecurityConfig _securityConfig; | ||
private long _abortErrorCode = -1; | ||
|
||
// Queue for accepted streams | ||
private readonly Channel<MsQuicStream> _acceptQueue = Channel.CreateUnbounded<MsQuicStream>(new UnboundedChannelOptions() | ||
|
@@ -200,6 +201,7 @@ private uint HandleEventShutdownInitiatedByTransport(ConnectionEvent connectionE | |
|
||
private uint HandleEventShutdownInitiatedByPeer(ConnectionEvent connectionEvent) | ||
{ | ||
_abortErrorCode = connectionEvent.Data.ShutdownBeginPeer.ErrorCode; | ||
_acceptQueue.Writer.Complete(); | ||
return MsQuicStatusCodes.Success; | ||
} | ||
|
@@ -237,18 +239,23 @@ internal override async ValueTask<QuicStreamProvider> AcceptStreamAsync(Cancella | |
|
||
ThrowIfDisposed(); | ||
|
||
if (await _acceptQueue.Reader.WaitToReadAsync(cancellationToken)) | ||
MsQuicStream stream; | ||
|
||
try | ||
{ | ||
stream = await _acceptQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(false); | ||
} | ||
catch (ChannelClosedException) | ||
{ | ||
if (_acceptQueue.Reader.TryRead(out MsQuicStream stream)) | ||
throw _abortErrorCode switch | ||
{ | ||
if (NetEventSource.IsEnabled) NetEventSource.Exit(this); | ||
return stream; | ||
} | ||
-1 => new QuicOperationAbortedException(), // Shutdown initiated by us. | ||
long err => new QuicConnectionAbortedException(err) // Shutdown initiated by peer. | ||
}; | ||
} | ||
|
||
if (NetEventSource.IsEnabled) NetEventSource.Exit(this); | ||
|
||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to continue returning null than to throw an exception from AcceptStreamAsync when the connection is closing. The connection being closed is a common enough that throwing an exception feels noisy. I know this will make the QuicConnection.AcceptStreamAsync() wrapper a little more complicated, but I think it's worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, looking back on this, I agree as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that anyone familiar with sockets will expect consistency with that, which is why I went that direction, but I also think most everyone else might expect it will return null. The question becomes who is the more likely user. It's trouble to merge Stream and Socket 🙃. I don't feel strongly about it. We can change it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep it with the "current" behavior of returning null, but fix it in QuicConnection.AcceptStreamAsync. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think this needs some more thought now. We should consider how consistent the APIs between QuicStream, QuicConnection, QuicListener need to be in how they behave on disposal. QuicStream.ReadAsync() etc. do not return a nullable type and will need to throw an exception. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to commit this for now to unblock things (the APIs are currently broken, returning null triggers a bug) but will open an issue to figure this out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: #32142 |
||
return stream; | ||
} | ||
|
||
internal override QuicStreamProvider OpenUnidirectionalStream() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ | |
|
||
namespace System.Net.Quic.Implementations.MsQuic | ||
{ | ||
internal class MsQuicListener : QuicListenerProvider, IDisposable | ||
internal sealed class MsQuicListener : QuicListenerProvider, IDisposable | ||
{ | ||
// Security configuration for MsQuic | ||
private MsQuicSession _session; | ||
|
@@ -65,21 +65,21 @@ internal override async ValueTask<QuicConnectionProvider> AcceptConnectionAsync( | |
|
||
ThrowIfDisposed(); | ||
|
||
if (await _acceptConnectionQueue.Reader.WaitToReadAsync()) | ||
{ | ||
if (_acceptConnectionQueue.Reader.TryRead(out MsQuicConnection connection)) | ||
{ | ||
// resolve security config here. | ||
await connection.SetSecurityConfigForConnection(_sslOptions.ServerCertificate); | ||
if (NetEventSource.IsEnabled) NetEventSource.Exit(this); | ||
MsQuicConnection connection; | ||
|
||
return connection; | ||
} | ||
try | ||
{ | ||
connection = await _acceptConnectionQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(false); | ||
} | ||
catch (ChannelClosedException) | ||
{ | ||
throw new QuicOperationAbortedException(); | ||
} | ||
|
||
if (NetEventSource.IsEnabled) NetEventSource.Exit(this); | ||
await connection.SetSecurityConfigForConnection(_sslOptions.ServerCertificate); | ||
|
||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should continue returning null here too. |
||
if (NetEventSource.IsEnabled) NetEventSource.Exit(this); | ||
return connection; | ||
} | ||
|
||
public override void Dispose() | ||
|
@@ -174,7 +174,7 @@ internal unsafe uint ListenerCallbackHandler( | |
} | ||
} | ||
|
||
protected void StopAcceptingConnections() | ||
private void StopAcceptingConnections() | ||
{ | ||
_acceptConnectionQueue.Writer.TryComplete(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,10 +48,12 @@ internal sealed class MsQuicStream : QuicStreamProvider | |
private StartState _started; | ||
|
||
private ReadState _readState; | ||
private long _readErrorCode = -1; | ||
|
||
private ShutdownWriteState _shutdownState; | ||
|
||
private SendState _sendState; | ||
private long _sendErrorCode = -1; | ||
|
||
// Used by the class to indicate that the stream is m_Readable. | ||
private readonly bool _canRead; | ||
|
@@ -245,7 +247,11 @@ internal override async ValueTask<int> ReadAsync(Memory<byte> destination, Cance | |
} | ||
else if (_readState == ReadState.Aborted) | ||
{ | ||
throw new IOException("Reading has been aborted by the peer."); | ||
throw _readErrorCode switch | ||
{ | ||
-1 => new QuicOperationAbortedException(), | ||
long err => new QuicStreamAbortedException(err) | ||
}; | ||
} | ||
} | ||
|
||
|
@@ -402,6 +408,7 @@ internal override void Write(ReadOnlySpan<byte> buffer) | |
{ | ||
ThrowIfDisposed(); | ||
|
||
// TODO: optimize this. | ||
WriteAsync(buffer.ToArray()).GetAwaiter().GetResult(); | ||
} | ||
|
||
|
@@ -532,14 +539,13 @@ private uint HandleEvent(ref StreamEvent evt) | |
// Peer has told us to abort the reading side of the stream. | ||
case QUIC_STREAM_EVENT.PEER_SEND_ABORTED: | ||
{ | ||
status = HandleEventPeerSendAborted(); | ||
status = HandleEventPeerSendAborted(ref evt); | ||
} | ||
break; | ||
// Peer has stopped receiving data, don't send anymore. | ||
// Potentially throw when WriteAsync/FlushAsync. | ||
case QUIC_STREAM_EVENT.PEER_RECEIVE_ABORTED: | ||
{ | ||
status = HandleEventPeerRecvAbort(); | ||
status = HandleEventPeerRecvAborted(ref evt); | ||
} | ||
break; | ||
// Occurs when shutdown is completed for the send side. | ||
|
@@ -598,9 +604,26 @@ private unsafe uint HandleEventRecv(ref MsQuicNativeMethods.StreamEvent evt) | |
return MsQuicStatusCodes.Pending; | ||
} | ||
|
||
private uint HandleEventPeerRecvAbort() | ||
private uint HandleEventPeerRecvAborted(ref StreamEvent evt) | ||
{ | ||
if (NetEventSource.IsEnabled) NetEventSource.Enter(this); | ||
|
||
bool shouldComplete = false; | ||
lock (_sync) | ||
{ | ||
if (_sendState == SendState.None) | ||
{ | ||
shouldComplete = true; | ||
} | ||
_sendState = SendState.Aborted; | ||
_sendErrorCode = evt.Data.PeerSendAbort.ErrorCode; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sendErrorCode a local variable. I don't believe it is used elsewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is used to throw a |
||
} | ||
|
||
if (shouldComplete) | ||
{ | ||
_sendResettableCompletionSource.CompleteException(new QuicStreamAbortedException(_sendErrorCode)); | ||
} | ||
|
||
if (NetEventSource.IsEnabled) NetEventSource.Exit(this); | ||
|
||
return MsQuicStatusCodes.Success; | ||
|
@@ -696,7 +719,7 @@ private uint HandleEventShutdownComplete() | |
return MsQuicStatusCodes.Success; | ||
} | ||
|
||
private uint HandleEventPeerSendAborted() | ||
private uint HandleEventPeerSendAborted(ref StreamEvent evt) | ||
{ | ||
if (NetEventSource.IsEnabled) NetEventSource.Enter(this); | ||
|
||
|
@@ -708,11 +731,12 @@ private uint HandleEventPeerSendAborted() | |
shouldComplete = true; | ||
} | ||
_readState = ReadState.Aborted; | ||
_readErrorCode = evt.Data.PeerSendAbort.ErrorCode; | ||
} | ||
|
||
if (shouldComplete) | ||
{ | ||
_receiveResettableCompletionSource.CompleteException(new IOException("Reading has been aborted by the peer.")); | ||
_receiveResettableCompletionSource.CompleteException(new QuicStreamAbortedException(_readErrorCode)); | ||
} | ||
|
||
if (NetEventSource.IsEnabled) NetEventSource.Exit(this); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
namespace System.Net.Quic | ||
{ | ||
public class QuicOperationAbortedException : QuicException | ||
{ | ||
internal QuicOperationAbortedException() | ||
: base(SR.net_quic_operationaborted) | ||
{ | ||
} | ||
|
||
public QuicOperationAbortedException(string message) : base(message) | ||
{ | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that QuicException derives directly from Exception. Would it be better to have it derive from IOException or possibly OperationCanceledException if it's always user-initiated?
I would lean towards IOException given how it's currently used. I see it's being triggered by stuff like "HandleEventShutdownInitiatedByPeer" which feels really IOExceptiony to me (if it needs to be an exception at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that seems reasonable. Open up an issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't know if QuicOperationAbortedException will be necessary now if we start returning null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at SocketException, and it doesn't derive from IOException, so I'm no longer convinced this is the way to go. Maybe it should derive from SystemException (which SocketException does indirectly), but I'm not sure what the guidance there is. Is this supposed to be the base class for all Exceptions in a System.* namespace?