From 0373fa5a136bdafdd47aad682a6cf214795e6ade Mon Sep 17 00:00:00 2001 From: ManickaP Date: Fri, 9 Jun 2023 14:40:23 +0200 Subject: [PATCH 1/7] Added tests --- .../tests/FunctionalTests/MsQuicTests.cs | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index ae10560992461..b950187dc90b0 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -19,6 +19,7 @@ using Microsoft.DotNet.XUnitExtensions; using Xunit; using Xunit.Abstractions; +using TestUtilities; namespace System.Net.Quic.Tests { @@ -57,6 +58,123 @@ public MsQuicTests(ITestOutputHelper output, CertificateSetup setup) : base(outp _certificates = setup; } + [Fact] + public async Task QuicRootedObjectGetReleased() + { + async Task<(WeakReference, WeakReference, WeakReference, WeakReference, WeakReference)> GetWeakReferencesAsync() + { + // Set up all objects, keep their weak reference. + QuicListener listener = await CreateQuicListener(); + WeakReference wrListener = new WeakReference(listener); + + var (clientConnection, serverConnection) = await CreateConnectedQuicConnection(listener); + WeakReference wrClientConnection = new WeakReference(clientConnection); + WeakReference wrServerConnection = new WeakReference(serverConnection); + + QuicStream clientStream = await clientConnection.OpenOutboundStreamAsync(QuicStreamType.Bidirectional); + await clientStream.WriteAsync(new byte[5], completeWrites: true); + + QuicStream serverStream = await serverConnection.AcceptInboundStreamAsync(); + await serverStream.WriteAsync(new byte[10], completeWrites: true); + + WeakReference wrClientStream = new WeakReference(clientStream); + WeakReference wrServerStream = new WeakReference(serverStream); + + while (!clientStream.ReadsClosed.IsCompleted) + { + int bytes = await clientStream.ReadAsync(new byte[10]); + if (bytes == 0) + { + break; + } + } + while (!serverStream.ReadsClosed.IsCompleted) + { + int bytes = await serverStream.ReadAsync(new byte[10]); + if (bytes == 0) + { + break; + } + } + + // Dispose everything and check if all weak references are dead. + await clientStream.DisposeAsync(); + await serverStream.DisposeAsync(); + await clientConnection.DisposeAsync(); + await serverConnection.DisposeAsync(); + await listener.DisposeAsync(); + + return (wrListener, wrClientConnection, wrServerConnection, wrClientStream, wrServerStream); + } + + var (wrListener, wrClientConnection, wrServerConnection, wrClientStream, wrServerStream) = await GetWeakReferencesAsync(); + + for (int i = 0; i < 20; ++i) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + + await Task.Delay(100 * i); + + if (wrListener.TryGetTarget(out _) || + wrClientConnection.TryGetTarget(out _) || + wrServerConnection.TryGetTarget(out _) || + wrClientStream.TryGetTarget(out _) || + wrServerStream.TryGetTarget(out _)) + { + continue; + } + + break; + } + + Assert.False(wrListener.TryGetTarget(out _)); + Assert.False(wrClientConnection.TryGetTarget(out _)); + Assert.False(wrServerConnection.TryGetTarget(out _)); + Assert.False(wrClientStream.TryGetTarget(out _)); + Assert.False(wrServerStream.TryGetTarget(out _)); + } + + [Fact] + public async Task QuicRootedConnectionGetsReleased_ConnectFails() + { + using var x = new TestEventListener(_output, "Private.InternalDiagnostics.System.Net.Quic"); + + WeakReference wrServerConnection = default; + // Set up all objects, keep their weak reference. + var listenerOptions = CreateQuicListenerOptions(); + listenerOptions.ConnectionOptionsCallback = (connection, _, _) => + { + wrServerConnection = new WeakReference(connection); + var serverConnectionOptions = CreateQuicServerOptions(); + serverConnectionOptions.ServerAuthenticationOptions = new SslServerAuthenticationOptions(); + return ValueTask.FromResult(serverConnectionOptions); + }; + QuicListener listener = await CreateQuicListener(listenerOptions); + + await Assert.ThrowsAsync(async () => await CreateConnectedQuicConnection(listener)); + + // Dispose everything and check if all weak references are dead. + await listener.DisposeAsync(); + + for (int i = 0; i < 20; ++i) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + + await Task.Delay(100 * i); + + if (wrServerConnection.TryGetTarget(out _)) + { + continue; + } + + break; + } + + Assert.False(wrServerConnection.TryGetTarget(out _)); + } + [Fact] public async Task ConnectWithCertificateChain() { From 4b16a3692563cae6af3994d63a2d7d1c393985dc Mon Sep 17 00:00:00 2001 From: ManickaP Date: Fri, 9 Jun 2023 15:40:25 +0200 Subject: [PATCH 2/7] Fixed logging and minor typos --- .../Net/Quic/Internal/MsQuicExtensions.cs | 71 +++++++++++++++ .../System/Net/Quic/Internal/MsQuicHelpers.cs | 4 +- .../src/System/Net/Quic/QuicConnection.cs | 57 ++---------- .../src/System/Net/Quic/QuicListener.cs | 2 +- .../src/System/Net/Quic/QuicStream.cs | 90 +++++-------------- 5 files changed, 103 insertions(+), 121 deletions(-) create mode 100644 src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicExtensions.cs diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicExtensions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicExtensions.cs new file mode 100644 index 0000000000000..fbade293cfda3 --- /dev/null +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicExtensions.cs @@ -0,0 +1,71 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Net.Quic; + +namespace Microsoft.Quic; + +internal unsafe partial struct QUIC_NEW_CONNECTION_INFO +{ + public override string ToString() + => $"{{ {nameof(QuicVersion)} = {QuicVersion}, {nameof(LocalAddress)} = {LocalAddress->ToIPEndPoint()}, {nameof(RemoteAddress)} = {RemoteAddress->ToIPEndPoint()} }}"; +} + +internal unsafe partial struct QUIC_LISTENER_EVENT +{ + public override string ToString() + => Type switch + { + QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION + => $"{{ {nameof(NEW_CONNECTION.Info)} = {{ {nameof(QUIC_NEW_CONNECTION_INFO.QuicVersion)} = {NEW_CONNECTION.Info->QuicVersion}, {nameof(QUIC_NEW_CONNECTION_INFO.LocalAddress)} = {NEW_CONNECTION.Info->LocalAddress->ToIPEndPoint()}, {nameof(QUIC_NEW_CONNECTION_INFO.RemoteAddress)} = {NEW_CONNECTION.Info->RemoteAddress->ToIPEndPoint()} }} }}", + _ => string.Empty + }; +} + +internal unsafe partial struct QUIC_CONNECTION_EVENT +{ + public override string ToString() + => Type switch + { + QUIC_CONNECTION_EVENT_TYPE.CONNECTED + => $"{{ {nameof(CONNECTED.SessionResumed)} = {CONNECTED.SessionResumed} }}", + QUIC_CONNECTION_EVENT_TYPE.SHUTDOWN_INITIATED_BY_TRANSPORT + => $"{{ {nameof(SHUTDOWN_INITIATED_BY_TRANSPORT.Status)} = {SHUTDOWN_INITIATED_BY_TRANSPORT.Status}, {nameof(SHUTDOWN_INITIATED_BY_TRANSPORT.ErrorCode)} = {SHUTDOWN_INITIATED_BY_TRANSPORT.ErrorCode} }}", + QUIC_CONNECTION_EVENT_TYPE.SHUTDOWN_INITIATED_BY_PEER + => $"{{ {nameof(SHUTDOWN_INITIATED_BY_PEER.ErrorCode)} = {SHUTDOWN_INITIATED_BY_PEER.ErrorCode} }}", + QUIC_CONNECTION_EVENT_TYPE.SHUTDOWN_COMPLETE + => $"{{ {nameof(SHUTDOWN_COMPLETE.HandshakeCompleted)} = {SHUTDOWN_COMPLETE.HandshakeCompleted}, {nameof(SHUTDOWN_COMPLETE.PeerAcknowledgedShutdown)} = {SHUTDOWN_COMPLETE.PeerAcknowledgedShutdown}, {nameof(SHUTDOWN_COMPLETE.AppCloseInProgress)} = {SHUTDOWN_COMPLETE.AppCloseInProgress} }}", + QUIC_CONNECTION_EVENT_TYPE.LOCAL_ADDRESS_CHANGED + => $"{{ {nameof(LOCAL_ADDRESS_CHANGED.Address)} = {LOCAL_ADDRESS_CHANGED.Address->ToIPEndPoint()} }}", + QUIC_CONNECTION_EVENT_TYPE.PEER_ADDRESS_CHANGED + => $"{{ {nameof(PEER_ADDRESS_CHANGED.Address)} = {PEER_ADDRESS_CHANGED.Address->ToIPEndPoint()} }}", + QUIC_CONNECTION_EVENT_TYPE.PEER_STREAM_STARTED + => $"{{ {nameof(PEER_STREAM_STARTED.Flags)} = {PEER_STREAM_STARTED.Flags} }}", + QUIC_CONNECTION_EVENT_TYPE.PEER_CERTIFICATE_RECEIVED + => $"{{ {nameof(PEER_CERTIFICATE_RECEIVED.DeferredStatus)} = {PEER_CERTIFICATE_RECEIVED.DeferredStatus}, {nameof(PEER_CERTIFICATE_RECEIVED.DeferredErrorFlags)} = {PEER_CERTIFICATE_RECEIVED.DeferredErrorFlags} }}", + _ => string.Empty + }; +} + +internal unsafe partial struct QUIC_STREAM_EVENT +{ + public override string ToString() + => Type switch + { + QUIC_STREAM_EVENT_TYPE.START_COMPLETE + => $"{{ {nameof(START_COMPLETE.Status)} = {START_COMPLETE.Status}, {nameof(START_COMPLETE.ID)} = {START_COMPLETE.ID}, {nameof(START_COMPLETE.PeerAccepted)} = {START_COMPLETE.PeerAccepted} }}", + QUIC_STREAM_EVENT_TYPE.RECEIVE + => $"{{ {nameof(RECEIVE.AbsoluteOffset)} = {RECEIVE.AbsoluteOffset}, {nameof(RECEIVE.TotalBufferLength)} = {RECEIVE.TotalBufferLength}, {nameof(RECEIVE.Flags)} = {RECEIVE.Flags} }}", + QUIC_STREAM_EVENT_TYPE.SEND_COMPLETE + => $"{{ {nameof(SEND_COMPLETE.Canceled)} = {SEND_COMPLETE.Canceled} }}", + QUIC_STREAM_EVENT_TYPE.PEER_SEND_ABORTED + => $"{{ {nameof(PEER_SEND_ABORTED.ErrorCode)} = {PEER_SEND_ABORTED.ErrorCode} }}", + QUIC_STREAM_EVENT_TYPE.PEER_RECEIVE_ABORTED + => $"{{ {nameof(PEER_RECEIVE_ABORTED.ErrorCode)} = {PEER_RECEIVE_ABORTED.ErrorCode} }}", + QUIC_STREAM_EVENT_TYPE.SEND_SHUTDOWN_COMPLETE + => $"{{ {nameof(SEND_SHUTDOWN_COMPLETE.Graceful)} = {SEND_SHUTDOWN_COMPLETE.Graceful} }}", + QUIC_STREAM_EVENT_TYPE.SHUTDOWN_COMPLETE + => $"{{ {nameof(SHUTDOWN_COMPLETE.ConnectionShutdown)} = {SHUTDOWN_COMPLETE.ConnectionShutdown}, {nameof(SHUTDOWN_COMPLETE.ConnectionShutdownByApp)} = {SHUTDOWN_COMPLETE.ConnectionShutdownByApp}, {nameof(SHUTDOWN_COMPLETE.ConnectionClosedRemotely)} = {SHUTDOWN_COMPLETE.ConnectionClosedRemotely}, {nameof(SHUTDOWN_COMPLETE.ConnectionErrorCode)} = {SHUTDOWN_COMPLETE.ConnectionErrorCode}, {nameof(SHUTDOWN_COMPLETE.ConnectionCloseStatus)} = {SHUTDOWN_COMPLETE.ConnectionCloseStatus} }}", + _ => string.Empty + }; +} diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicHelpers.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicHelpers.cs index 683e8bb62473e..bf454c047c3cd 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicHelpers.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicHelpers.cs @@ -42,13 +42,13 @@ internal static unsafe IPEndPoint ToIPEndPoint(this ref QuicAddr quicAddress, Ad return new Internals.SocketAddress(addressFamilyOverride ?? SocketAddressPal.GetAddressFamily(addressBytes), addressBytes).GetIPEndPoint(); } - internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint iPEndPoint) + internal static unsafe QuicAddr ToQuicAddr(this IPEndPoint ipEndPoint) { // TODO: is the layout same for SocketAddress.Buffer and QuicAddr on all platforms? QuicAddr result = default; Span rawAddress = MemoryMarshal.AsBytes(MemoryMarshal.CreateSpan(ref result, 1)); - Internals.SocketAddress address = IPEndPointExtensions.Serialize(iPEndPoint); + Internals.SocketAddress address = IPEndPointExtensions.Serialize(ipEndPoint); Debug.Assert(address.Size <= rawAddress.Length); address.Buffer.AsSpan(0, address.Size).CopyTo(rawAddress); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index ccb0f93c89862..9671def267b60 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -234,6 +234,8 @@ internal unsafe QuicConnection(QUIC_HANDLE* handle, QUIC_NEW_CONNECTION_INFO* in private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, CancellationToken cancellationToken = default) { + ObjectDisposedException.ThrowIf(_disposed == 1, this); + if (_connectedTcs.TryInitialize(out ValueTask valueTask, this, cancellationToken)) { _canAccept = options.MaxInboundBidirectionalStreams > 0 || options.MaxInboundUnidirectionalStreams > 0; @@ -473,19 +475,13 @@ private unsafe int HandleEventConnected(ref CONNECTED_DATA data) if (NetEventSource.Log.IsEnabled()) { - NetEventSource.Info(this, $"{this} Received event CONNECTED {LocalEndPoint} -> {RemoteEndPoint}"); + NetEventSource.Info(this, $"{this} Connection connected {LocalEndPoint} -> {RemoteEndPoint} for {_negotiatedApplicationProtocol} protocol"); } - _connectedTcs.TrySetResult(); return QUIC_STATUS_SUCCESS; } private unsafe int HandleEventShutdownInitiatedByTransport(ref SHUTDOWN_INITIATED_BY_TRANSPORT_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event SHUTDOWN_INITIATED_BY_TRANSPORT with {nameof(data.Status)}={data.Status}"); - } - // TODO: we should propagate transport error code. // https://github.com/dotnet/runtime/issues/72666 Exception exception = ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetExceptionForMsQuicStatus(data.Status)); @@ -495,21 +491,11 @@ private unsafe int HandleEventShutdownInitiatedByTransport(ref SHUTDOWN_INITIATE } private unsafe int HandleEventShutdownInitiatedByPeer(ref SHUTDOWN_INITIATED_BY_PEER_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event SHUTDOWN_INITIATED_BY_PEER_DATA with {nameof(data.ErrorCode)}={data.ErrorCode}"); - } - _acceptQueue.Writer.TryComplete(ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetConnectionAbortedException((long)data.ErrorCode))); return QUIC_STATUS_SUCCESS; } private unsafe int HandleEventShutdownComplete() { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event SHUTDOWN_INITIATED_BY_PEER_DATA"); - } - _acceptQueue.Writer.TryComplete(ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetOperationAbortedException())); _shutdownTcs.TrySetResult(); return QUIC_STATUS_SUCCESS; @@ -517,30 +503,15 @@ private unsafe int HandleEventShutdownComplete() private unsafe int HandleEventLocalAddressChanged(ref LOCAL_ADDRESS_CHANGED_DATA data) { _localEndPoint = data.Address->ToIPEndPoint(); - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event LOCAL_ADDRESS_CHANGED with {nameof(data.Address)}={_localEndPoint}"); - } - return QUIC_STATUS_SUCCESS; } private unsafe int HandleEventPeerAddressChanged(ref PEER_ADDRESS_CHANGED_DATA data) { _remoteEndPoint = data.Address->ToIPEndPoint(); - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event LOCAL_ADDRESS_CHANGED with {nameof(data.Address)}={_remoteEndPoint}"); - } - return QUIC_STATUS_SUCCESS; } private unsafe int HandleEventPeerStreamStarted(ref PEER_STREAM_STARTED_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event PEER_STREAM_STARTED"); - } - QuicStream stream = new QuicStream(_handle, data.Stream, data.Flags, _defaultStreamErrorCode); if (!_acceptQueue.Writer.TryWrite(stream)) { @@ -557,11 +528,6 @@ private unsafe int HandleEventPeerStreamStarted(ref PEER_STREAM_STARTED_DATA dat } private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEIVED_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event PEER_CERTIFICATE_RECEIVED_DATA"); - } - try { return _sslConnectionOptions.ValidateCertificate((QUIC_BUFFER*)data.Certificate, (QUIC_BUFFER*)data.Chain, out _remoteCertificate); @@ -573,16 +539,6 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI } } - private int HandleConnectionEvent(QUIC_CONNECTION_EVENT_TYPE type) - { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event {type}"); - } - - return QUIC_STATUS_SUCCESS; - } - private unsafe int HandleConnectionEvent(ref QUIC_CONNECTION_EVENT connectionEvent) => connectionEvent.Type switch { @@ -594,7 +550,7 @@ private unsafe int HandleConnectionEvent(ref QUIC_CONNECTION_EVENT connectionEve QUIC_CONNECTION_EVENT_TYPE.PEER_ADDRESS_CHANGED => HandleEventPeerAddressChanged(ref connectionEvent.PEER_ADDRESS_CHANGED), QUIC_CONNECTION_EVENT_TYPE.PEER_STREAM_STARTED => HandleEventPeerStreamStarted(ref connectionEvent.PEER_STREAM_STARTED), QUIC_CONNECTION_EVENT_TYPE.PEER_CERTIFICATE_RECEIVED => HandleEventPeerCertificateReceived(ref connectionEvent.PEER_CERTIFICATE_RECEIVED), - _ => HandleConnectionEvent(connectionEvent.Type), + _ => QUIC_STATUS_SUCCESS, }; #pragma warning disable CS3016 @@ -616,6 +572,11 @@ private static unsafe int NativeCallback(QUIC_HANDLE* connection, void* context, try { + // Process the event. + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(instance, $"{instance} Received event {connectionEvent->Type} {connectionEvent->ToString()}"); + } return instance.HandleConnectionEvent(ref *connectionEvent); } catch (Exception ex) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs index 841ca9b73c913..7b4c6613afabf 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs @@ -333,7 +333,7 @@ private static unsafe int NativeCallback(QUIC_HANDLE* listener, void* context, Q // Process the event. if (NetEventSource.Log.IsEnabled()) { - NetEventSource.Info(instance, $"{instance} Received event {listenerEvent->Type}"); + NetEventSource.Info(instance, $"{instance} Received event {listenerEvent->Type} {listenerEvent->ToString()}"); } return instance.HandleListenerEvent(ref *listenerEvent); } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index 701a1edbbf314..6e353249ec856 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -10,13 +10,13 @@ using static System.Net.Quic.MsQuicHelpers; using static Microsoft.Quic.MsQuic; -using START_COMPLETE = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._START_COMPLETE_e__Struct; -using RECEIVE = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._RECEIVE_e__Struct; -using SEND_COMPLETE = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._SEND_COMPLETE_e__Struct; -using PEER_SEND_ABORTED = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._PEER_SEND_ABORTED_e__Struct; -using PEER_RECEIVE_ABORTED = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._PEER_RECEIVE_ABORTED_e__Struct; -using SEND_SHUTDOWN_COMPLETE = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._SEND_SHUTDOWN_COMPLETE_e__Struct; -using SHUTDOWN_COMPLETE = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._SHUTDOWN_COMPLETE_e__Struct; +using START_COMPLETE_DATA = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._START_COMPLETE_e__Struct; +using RECEIVE_DATA = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._RECEIVE_e__Struct; +using SEND_COMPLETE_DATA = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._SEND_COMPLETE_e__Struct; +using PEER_SEND_ABORTED_DATA = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._PEER_SEND_ABORTED_e__Struct; +using PEER_RECEIVE_ABORTED_DATA = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._PEER_RECEIVE_ABORTED_e__Struct; +using SEND_SHUTDOWN_COMPLETE_DATA = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._SEND_SHUTDOWN_COMPLETE_e__Struct; +using SHUTDOWN_COMPLETE_DATA = Microsoft.Quic.QUIC_STREAM_EVENT._Anonymous_e__Union._SHUTDOWN_COMPLETE_e__Struct; namespace System.Net.Quic; @@ -489,13 +489,8 @@ public void CompleteWrites() } } - private unsafe int HandleEventStartComplete(ref START_COMPLETE data) + private unsafe int HandleEventStartComplete(ref START_COMPLETE_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event START_COMPLETE with {nameof(data.ID)}={data.ID}, {nameof(data.Status)}={data.Status} and {nameof(data.PeerAccepted)}={data.PeerAccepted}"); - } - _id = unchecked((long)data.ID); if (StatusSucceeded(data.Status)) { @@ -515,18 +510,13 @@ private unsafe int HandleEventStartComplete(ref START_COMPLETE data) return QUIC_STATUS_SUCCESS; } - private unsafe int HandleEventReceive(ref RECEIVE data) + private unsafe int HandleEventReceive(ref RECEIVE_DATA data) { ulong totalCopied = (ulong)_receiveBuffers.CopyFrom( new ReadOnlySpan(data.Buffers, (int)data.BufferCount), (int)data.TotalBufferLength, data.Flags.HasFlag(QUIC_RECEIVE_FLAGS.FIN)); - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event RECEIVE with {nameof(data.BufferCount)}={data.BufferCount}, {nameof(data.TotalBufferLength)}={data.TotalBufferLength} and {nameof(totalCopied)}={totalCopied}"); - } - if (totalCopied < data.TotalBufferLength) { Volatile.Write(ref _receivedNeedsEnable, 1); @@ -537,13 +527,8 @@ private unsafe int HandleEventReceive(ref RECEIVE data) data.TotalBufferLength = totalCopied; return (_receiveBuffers.HasCapacity() && Interlocked.CompareExchange(ref _receivedNeedsEnable, 0, 1) == 1) ? QUIC_STATUS_CONTINUE : QUIC_STATUS_SUCCESS; } - private unsafe int HandleEventSendComplete(ref SEND_COMPLETE data) + private unsafe int HandleEventSendComplete(ref SEND_COMPLETE_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event SEND_COMPLETE with {nameof(data.Canceled)}={data.Canceled}"); - } - // In case of cancellation, the task from _sendTcs is finished before the aborting. It is technically possible for subsequent WriteAsync to grab the next task // from _sendTcs and start executing before SendComplete event occurs for the previous (canceled) write lock (_sendBuffersLock) @@ -559,44 +544,24 @@ private unsafe int HandleEventSendComplete(ref SEND_COMPLETE data) } private unsafe int HandleEventPeerSendShutdown() { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event PEER_SEND_SHUTDOWN."); - } - // Same as RECEIVE with FIN flag. Remember that no more RECEIVE events will come. // Don't set the task to its final state yet, but wait for all the buffered data to get consumed first. _receiveBuffers.SetFinal(); _receiveTcs.TrySetResult(); return QUIC_STATUS_SUCCESS; } - private unsafe int HandleEventPeerSendAborted(ref PEER_SEND_ABORTED data) + private unsafe int HandleEventPeerSendAborted(ref PEER_SEND_ABORTED_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event PEER_SEND_ABORTED with {nameof(data.ErrorCode)}={data.ErrorCode}"); - } - _receiveTcs.TrySetException(ThrowHelper.GetStreamAbortedException((long)data.ErrorCode), final: true); return QUIC_STATUS_SUCCESS; } - private unsafe int HandleEventPeerReceiveAborted(ref PEER_RECEIVE_ABORTED data) + private unsafe int HandleEventPeerReceiveAborted(ref PEER_RECEIVE_ABORTED_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event PEER_RECEIVE_ABORTED with {nameof(data.ErrorCode)}={data.ErrorCode}"); - } - _sendTcs.TrySetException(ThrowHelper.GetStreamAbortedException((long)data.ErrorCode), final: true); return QUIC_STATUS_SUCCESS; } - private unsafe int HandleEventSendShutdownComplete(ref SEND_SHUTDOWN_COMPLETE data) + private unsafe int HandleEventSendShutdownComplete(ref SEND_SHUTDOWN_COMPLETE_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event PEER_RECEIVE_ABORTED with {nameof(data.Graceful)}={data.Graceful}"); - } - if (data.Graceful != 0) { _sendTcs.TrySetResult(final: true); @@ -604,13 +569,8 @@ private unsafe int HandleEventSendShutdownComplete(ref SEND_SHUTDOWN_COMPLETE da // If Graceful == 0, we either aborted write, received PEER_RECEIVE_ABORTED or will receive SHUTDOWN_COMPLETE(ConnectionClose) later, all of which completes the _sendTcs. return QUIC_STATUS_SUCCESS; } - private unsafe int HandleEventShutdownComplete(ref SHUTDOWN_COMPLETE data) + private unsafe int HandleEventShutdownComplete(ref SHUTDOWN_COMPLETE_DATA data) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event SHUTDOWN_COMPLETE with {nameof(data.ConnectionShutdown)}={data.ConnectionShutdown}"); - } - if (data.ConnectionShutdown != 0) { bool shutdownByApp = data.ConnectionShutdownByApp != 0; @@ -639,25 +599,10 @@ private unsafe int HandleEventShutdownComplete(ref SHUTDOWN_COMPLETE data) } private unsafe int HandleEventPeerAccepted() { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event PEER_ACCEPTED"); - } - _startedTcs.TrySetResult(); return QUIC_STATUS_SUCCESS; } - private int HandleStreamEvent(QUIC_STREAM_EVENT_TYPE type) - { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Received event {type}"); - } - - return QUIC_STATUS_SUCCESS; - } - private unsafe int HandleStreamEvent(ref QUIC_STREAM_EVENT streamEvent) => streamEvent.Type switch { @@ -670,7 +615,7 @@ private unsafe int HandleStreamEvent(ref QUIC_STREAM_EVENT streamEvent) QUIC_STREAM_EVENT_TYPE.SEND_SHUTDOWN_COMPLETE => HandleEventSendShutdownComplete(ref streamEvent.SEND_SHUTDOWN_COMPLETE), QUIC_STREAM_EVENT_TYPE.SHUTDOWN_COMPLETE => HandleEventShutdownComplete(ref streamEvent.SHUTDOWN_COMPLETE), QUIC_STREAM_EVENT_TYPE.PEER_ACCEPTED => HandleEventPeerAccepted(), - _ => HandleStreamEvent(streamEvent.Type) + _ => QUIC_STATUS_SUCCESS }; #pragma warning disable CS3016 @@ -692,6 +637,11 @@ private static unsafe int NativeCallback(QUIC_HANDLE* connection, void* context, try { + // Process the event. + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(instance, $"{instance} Received event {streamEvent->Type} {streamEvent->ToString()}"); + } return instance.HandleStreamEvent(ref *streamEvent); } catch (Exception ex) From 2f394e0209c58ee8dbdd1c41066b9d76300d200d Mon Sep 17 00:00:00 2001 From: ManickaP Date: Fri, 9 Jun 2023 16:11:23 +0200 Subject: [PATCH 3/7] Fix --- .../System.Net.Quic/src/System/Net/Quic/QuicConnection.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 9671def267b60..d72c55c77e838 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -496,7 +496,9 @@ private unsafe int HandleEventShutdownInitiatedByPeer(ref SHUTDOWN_INITIATED_BY_ } private unsafe int HandleEventShutdownComplete() { - _acceptQueue.Writer.TryComplete(ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetOperationAbortedException())); + Exception exception = ExceptionDispatchInfo.SetCurrentStackTrace(ThrowHelper.GetOperationAbortedException()); + _acceptQueue.Writer.TryComplete(exception); + _connectedTcs.TrySetException(exception); _shutdownTcs.TrySetResult(); return QUIC_STATUS_SUCCESS; } From 2e296cb3c8418f2821453d7ea1f6fe7952eec34f Mon Sep 17 00:00:00 2001 From: ManickaP Date: Fri, 9 Jun 2023 18:32:57 +0200 Subject: [PATCH 4/7] Fix test --- .../tests/FunctionalTests/MsQuicTests.cs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index b950187dc90b0..105a612a89bca 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -116,16 +116,18 @@ public async Task QuicRootedObjectGetReleased() await Task.Delay(100 * i); - if (wrListener.TryGetTarget(out _) || - wrClientConnection.TryGetTarget(out _) || - wrServerConnection.TryGetTarget(out _) || - wrClientStream.TryGetTarget(out _) || - wrServerStream.TryGetTarget(out _)) + if (TestWeakReferences()) { continue; } - break; + + bool TestWeakReferences() + => wrListener.TryGetTarget(out _) || + wrClientConnection.TryGetTarget(out _) || + wrServerConnection.TryGetTarget(out _) || + wrClientStream.TryGetTarget(out _) || + wrServerStream.TryGetTarget(out _); } Assert.False(wrListener.TryGetTarget(out _)); @@ -138,8 +140,6 @@ public async Task QuicRootedObjectGetReleased() [Fact] public async Task QuicRootedConnectionGetsReleased_ConnectFails() { - using var x = new TestEventListener(_output, "Private.InternalDiagnostics.System.Net.Quic"); - WeakReference wrServerConnection = default; // Set up all objects, keep their weak reference. var listenerOptions = CreateQuicListenerOptions(); @@ -164,10 +164,12 @@ public async Task QuicRootedConnectionGetsReleased_ConnectFails() await Task.Delay(100 * i); - if (wrServerConnection.TryGetTarget(out _)) + if (TestWeakReferences()) { continue; } + bool TestWeakReferences() + => wrServerConnection.TryGetTarget(out _); break; } From 8a80680496e08c2a36bbf143771deb36b0b597e4 Mon Sep 17 00:00:00 2001 From: ManickaP Date: Mon, 12 Jun 2023 16:23:37 +0200 Subject: [PATCH 5/7] "Added asserts checking GCHandle state" --- .../Net/Quic/Internal/ResettableValueTaskSource.cs | 14 +++++++++++++- .../src/System/Net/Quic/QuicConnection.cs | 2 ++ .../src/System/Net/Quic/QuicStream.cs | 5 +++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs index 76dd9eae5a640..e7c0cf87bfd5d 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs @@ -56,6 +56,18 @@ public ResettableValueTaskSource(bool runContinuationsAsynchronously = true) /// public bool IsCompleted => (State)Volatile.Read(ref Unsafe.As(ref _state)) == State.Completed; + // TODO: Revisit this with https://github.com/dotnet/runtime/issues/79818 and https://github.com/dotnet/runtime/issues/79911 + public bool KeepAliveReleased + { + get + { + lock (this) + { + return !_keepAlive.IsAllocated; + } + } + } + /// /// Tries to get a value task representing this task source. If this task source is , it'll also transition it into state. /// It prevents concurrent operations from being invoked since it'll return false if the task source was already in state. @@ -153,7 +165,7 @@ private bool TryComplete(Exception? exception, bool final) // Unblock the current task source and in case of a final also the final task source. if (exception is not null) { - // Set up the exception stack strace for the caller. + // Set up the exception stack trace for the caller. exception = exception.StackTrace is null ? ExceptionDispatchInfo.SetCurrentStackTrace(exception) : exception; if (state == State.None || state == State.Awaiting) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index d72c55c77e838..8eea92f7b1380 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Net.Security; using System.Net.Sockets; using System.Runtime.CompilerServices; @@ -617,6 +618,7 @@ public async ValueTask DisposeAsync() // Wait for SHUTDOWN_COMPLETE, the last event, so that all resources can be safely released. await valueTask.ConfigureAwait(false); + Debug.Assert(_connectedTcs.IsCompleted); _handle.Dispose(); _configuration?.Dispose(); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index 6e353249ec856..aa654644231f1 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.IO; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; @@ -695,6 +696,10 @@ public override async ValueTask DisposeAsync() // Wait for SHUTDOWN_COMPLETE, the last event, so that all resources can be safely released. await valueTask.ConfigureAwait(false); + Debug.Assert(_startedTcs.IsCompleted); + // TODO: Revisit this with https://github.com/dotnet/runtime/issues/79818 and https://github.com/dotnet/runtime/issues/79911 + Debug.Assert(_receiveTcs.KeepAliveReleased); + Debug.Assert(_sendTcs.KeepAliveReleased); _handle.Dispose(); lock (_sendBuffersLock) From efa983972504dc3bc3c1780427a6d284b1adcee4 Mon Sep 17 00:00:00 2001 From: ManickaP Date: Tue, 13 Jun 2023 10:31:15 +0200 Subject: [PATCH 6/7] Exclude test from mono --- .../System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index 105a612a89bca..1a15e3f373f0c 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -58,7 +58,7 @@ public MsQuicTests(ITestOutputHelper output, CertificateSetup setup) : base(outp _certificates = setup; } - [Fact] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime))] public async Task QuicRootedObjectGetReleased() { async Task<(WeakReference, WeakReference, WeakReference, WeakReference, WeakReference)> GetWeakReferencesAsync() @@ -137,7 +137,7 @@ bool TestWeakReferences() Assert.False(wrServerStream.TryGetTarget(out _)); } - [Fact] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime))] public async Task QuicRootedConnectionGetsReleased_ConnectFails() { WeakReference wrServerConnection = default; From d2c218a9423d91ba801c2e983b1bfad4fbedf341 Mon Sep 17 00:00:00 2001 From: ManickaP Date: Wed, 14 Jun 2023 09:59:38 +0200 Subject: [PATCH 7/7] Removed unnecessary _disposed checks --- .../System.Net.Quic/src/System/Net/Quic/QuicConnection.cs | 4 ---- .../System.Net.Quic/src/System/Net/Quic/QuicStream.cs | 2 -- 2 files changed, 6 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 8eea92f7b1380..4422277733425 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -235,8 +235,6 @@ internal unsafe QuicConnection(QUIC_HANDLE* handle, QUIC_NEW_CONNECTION_INFO* in private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); - if (_connectedTcs.TryInitialize(out ValueTask valueTask, this, cancellationToken)) { _canAccept = options.MaxInboundBidirectionalStreams > 0 || options.MaxInboundUnidirectionalStreams > 0; @@ -332,8 +330,6 @@ private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, internal ValueTask FinishHandshakeAsync(QuicServerConnectionOptions options, string targetHost, CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); - if (_connectedTcs.TryInitialize(out ValueTask valueTask, this, cancellationToken)) { _canAccept = options.MaxInboundBidirectionalStreams > 0 || options.MaxInboundUnidirectionalStreams > 0; diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index aa654644231f1..ac45fbb312fd3 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -234,8 +234,6 @@ internal unsafe QuicStream(MsQuicContextSafeHandle connectionHandle, QUIC_HANDLE /// An asynchronous task that completes with the opened . internal ValueTask StartAsync(CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); - _startedTcs.TryInitialize(out ValueTask valueTask, this, cancellationToken); { unsafe