From 671972debd71e72e7aa6e4075bdb77241157d50d Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Mon, 9 Sep 2024 21:50:31 +0100 Subject: [PATCH 1/7] Fixes --- .../src/System/Net/WebSockets/AsyncMutex.cs | 4 +- .../WebSockets/ManagedWebSocket.KeepAlive.cs | 20 ++++---- .../System/Net/WebSockets/ManagedWebSocket.cs | 51 ++++++++++--------- .../WebSockets/NetEventSource.WebSockets.cs | 12 ----- 4 files changed, 39 insertions(+), 48 deletions(-) diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs index abf7e5e56a276..6e8d7c600bb9a 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs @@ -66,7 +66,7 @@ public Task EnterAsync(CancellationToken cancellationToken) Task Contended(CancellationToken cancellationToken) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexContended(this, _gate); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Mutex queue length {-_gate}", nameof(EnterAsync)); var w = new Waiter(this); @@ -188,7 +188,7 @@ public void Exit() void Contended() { - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexContended(this, _gate); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Mutex queue length {-_gate}", nameof(Exit)); Waiter? w; lock (SyncObj) diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs index c9ff393cb7180..c877894fdae65 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs @@ -36,9 +36,9 @@ private void UnsolicitedPongHeartBeat() TrySendKeepAliveFrameAsync(MessageOpcode.Pong)); } - private ValueTask TrySendKeepAliveFrameAsync(MessageOpcode opcode, ReadOnlyMemory? payload = null) + private ValueTask TrySendKeepAliveFrameAsync(MessageOpcode opcode, ReadOnlyMemory payload = default) { - Debug.Assert(opcode is MessageOpcode.Pong || !IsUnsolicitedPongKeepAlive && opcode is MessageOpcode.Ping); + Debug.Assert((opcode is MessageOpcode.Pong) || (!IsUnsolicitedPongKeepAlive && opcode is MessageOpcode.Ping)); if (!IsValidSendState(_state)) { @@ -48,9 +48,7 @@ private ValueTask TrySendKeepAliveFrameAsync(MessageOpcode opcode, ReadOnlyMemor return ValueTask.CompletedTask; } - payload ??= ReadOnlyMemory.Empty; - - return SendFrameAsync(opcode, endOfMessage: true, disableCompression: true, payload.Value, CancellationToken.None); + return SendFrameAsync(opcode, endOfMessage: true, disableCompression: true, payload, CancellationToken.None); } private void KeepAlivePingHeartBeat() @@ -76,7 +74,7 @@ private void KeepAlivePingHeartBeat() if (_keepAlivePingState.PingSent) { - if (Environment.TickCount64 > _keepAlivePingState.PingTimeoutTimestamp) + if (now > _keepAlivePingState.PingTimeoutTimestamp) { if (NetEventSource.Log.IsEnabled()) { @@ -92,7 +90,7 @@ private void KeepAlivePingHeartBeat() } else { - if (Environment.TickCount64 > _keepAlivePingState.NextPingRequestTimestamp) + if (now > _keepAlivePingState.NextPingRequestTimestamp) { _keepAlivePingState.OnNextPingRequestCore(); // we are holding the lock shouldSendPing = true; @@ -147,22 +145,24 @@ private void Observe(ValueTask t) } } + private void Observe(Task t) => Observe(t, this); + // "Observe" any exception, ignoring it to prevent the unobserved task // exception event from being raised. - private void Observe(Task t) + private static void Observe(Task t, object thisObj) { if (t.IsCompleted) { if (t.IsFaulted) { - LogFaulted(t, this); + LogFaulted(t, thisObj); } } else { t.ContinueWith( LogFaulted, - this, + thisObj, CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs index 8a26a4c29e2eb..b75092e12bc82 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs @@ -267,31 +267,36 @@ private void DisposeCore() private static void DisposeSafe(IDisposable? resource, AsyncMutex mutex) { - if (resource is not null) + if (resource is null) { - Task lockTask = mutex.EnterAsync(CancellationToken.None); - - if (lockTask.IsCompleted) - { - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(mutex); - - resource.Dispose(); - mutex.Exit(); + return; + } - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexExited(mutex); - } - else - { - lockTask.GetAwaiter().UnsafeOnCompleted(() => - { - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(mutex); + Task lockTask = mutex.EnterAsync(CancellationToken.None); + if (lockTask.IsCompleted) + { + if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(mutex); + DisposeSafeCore(resource, mutex); + } + else + { + Observe( + DisposeSafeAsync(resource, mutex, lockTask), + thisObj: resource); + } - resource.Dispose(); - mutex.Exit(); + static async Task DisposeSafeAsync(IDisposable resource, AsyncMutex mutex, Task lockTask) + { + await lockTask.ConfigureAwait(false); + if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(mutex); + DisposeSafeCore(resource, mutex); + } - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexExited(mutex); - }); - } + static void DisposeSafeCore(IDisposable resource, AsyncMutex mutex) + { + resource.Dispose(); + mutex.Exit(); + if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexExited(mutex); } } @@ -797,11 +802,9 @@ private async ValueTask ReceiveAsyncPrivate(Memory paylo if (NetEventSource.Log.IsEnabled()) NetEventSource.ReceiveAsyncPrivateStarted(this, payloadBuffer.Length); - CancellationTokenRegistration registration = default; + CancellationTokenRegistration registration = cancellationToken.Register(static s => ((ManagedWebSocket)s!).Abort(), this); try { - registration = cancellationToken.Register(static s => ((ManagedWebSocket)s!).Abort(), this); - await _receiveMutex.EnterAsync(cancellationToken).ConfigureAwait(false); if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(_receiveMutex); diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/NetEventSource.WebSockets.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/NetEventSource.WebSockets.cs index d0977fb767060..fceeadd862d3c 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/NetEventSource.WebSockets.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/NetEventSource.WebSockets.cs @@ -34,7 +34,6 @@ internal sealed partial class NetEventSource private const int MutexEnterId = SendStopId + 1; private const int MutexExitId = MutexEnterId + 1; - private const int MutexContendedId = MutexExitId + 1; // // Keep-Alive @@ -185,10 +184,6 @@ private void MutexEnter(string objName, string memberName) => private void MutexExit(string objName, string memberName) => WriteEvent(MutexExitId, objName, memberName); - [Event(MutexContendedId, Keywords = Keywords.Debug, Level = EventLevel.Verbose)] - private void MutexContended(string objName, string memberName, int queueLength) => - WriteEvent(MutexContendedId, objName, memberName, queueLength); - [NonEvent] public static void MutexEntered(object? obj, [CallerMemberName] string? memberName = null) { @@ -203,13 +198,6 @@ public static void MutexExited(object? obj, [CallerMemberName] string? memberNam Log.MutexExit(IdOf(obj), memberName ?? MissingMember); } - [NonEvent] - public static void MutexContended(object? obj, int gateValue, [CallerMemberName] string? memberName = null) - { - Debug.Assert(Log.IsEnabled()); - Log.MutexContended(IdOf(obj), memberName ?? MissingMember, -gateValue); - } - // // WriteEvent overloads // From 995776975387a4b0772799ed5a2801c524506e46 Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Tue, 10 Sep 2024 13:55:55 +0100 Subject: [PATCH 2/7] State validation update --- .../Net/WebSockets/WebSocketValidate.cs | 23 -------- .../src/System.Net.WebSockets.csproj | 1 + .../WebSockets/ManagedWebSocket.KeepAlive.cs | 19 ++----- .../System/Net/WebSockets/ManagedWebSocket.cs | 14 ++--- .../Net/WebSockets/WebSocketStateHelper.cs | 56 +++++++++++++++++++ 5 files changed, 70 insertions(+), 43 deletions(-) create mode 100644 src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs diff --git a/src/libraries/Common/src/System/Net/WebSockets/WebSocketValidate.cs b/src/libraries/Common/src/System/Net/WebSockets/WebSocketValidate.cs index c11524bdeef9b..ed23c9bcbccda 100644 --- a/src/libraries/Common/src/System/Net/WebSockets/WebSocketValidate.cs +++ b/src/libraries/Common/src/System/Net/WebSockets/WebSocketValidate.cs @@ -37,29 +37,6 @@ internal static partial class WebSocketValidate private static readonly SearchValues s_validSubprotocolChars = SearchValues.Create("!#$%&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~"); - internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDisposed, WebSocketState[] validStates) - => ThrowIfInvalidState(currentState, isDisposed, innerException: null, validStates ?? []); - - internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDisposed, Exception? innerException, WebSocketState[]? validStates = null) - { - if (validStates is not null && Array.IndexOf(validStates, currentState) == -1) - { - string invalidStateMessage = SR.Format( - SR.net_WebSockets_InvalidState, currentState, string.Join(", ", validStates)); - - throw new WebSocketException(WebSocketError.InvalidState, invalidStateMessage, innerException); - } - - if (innerException is not null) - { - Debug.Assert(currentState == WebSocketState.Aborted); - throw new OperationCanceledException(nameof(WebSocketState.Aborted), innerException); - } - - // Ordering is important to maintain .NET 4.5 WebSocket implementation exception behavior. - ObjectDisposedException.ThrowIf(isDisposed, typeof(WebSocket)); - } - internal static void ValidateSubprotocol(string subProtocol) { ArgumentException.ThrowIfNullOrWhiteSpace(subProtocol); diff --git a/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj b/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj index 177e95dacee0a..1f1a8046a4cf1 100644 --- a/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj +++ b/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj @@ -31,6 +31,7 @@ + _keepAlivePingState is null; - private static bool IsValidSendState(WebSocketState state) => Array.IndexOf(s_validSendStates, state) != -1; - private static bool IsValidReceiveState(WebSocketState state) => Array.IndexOf(s_validReceiveStates, state) != -1; + private static bool IsValidSendState(WebSocketState state) => WebSocketStateHelper.HasFlag(s_validSendStates, state); + private static bool IsValidReceiveState(WebSocketState state) => WebSocketStateHelper.HasFlag(s_validReceiveStates, state); private void HeartBeat() { @@ -117,18 +116,12 @@ private async ValueTask SendPingAsync(long pingPayload) { Debug.Assert(_keepAlivePingState != null); - byte[] pingPayloadBuffer = ArrayPool.Shared.Rent(sizeof(long)); + byte[] pingPayloadBuffer = new byte[sizeof(long)]; BinaryPrimitives.WriteInt64BigEndian(pingPayloadBuffer, pingPayload); - try - { - await TrySendKeepAliveFrameAsync(MessageOpcode.Ping, pingPayloadBuffer.AsMemory(0, sizeof(long))).ConfigureAwait(false); - if (NetEventSource.Log.IsEnabled()) NetEventSource.KeepAlivePingSent(this, pingPayload); - } - finally - { - ArrayPool.Shared.Return(pingPayloadBuffer); - } + await TrySendKeepAliveFrameAsync(MessageOpcode.Ping, pingPayloadBuffer).ConfigureAwait(false); + + if (NetEventSource.Log.IsEnabled()) NetEventSource.KeepAlivePingSent(this, pingPayload); } // "Observe" either a ValueTask result, or any exception, ignoring it diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs index b75092e12bc82..b02f7ea783239 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs @@ -33,13 +33,13 @@ internal sealed partial class ManagedWebSocket : WebSocket private static readonly UTF8Encoding s_textEncoding = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true); /// Valid states to be in when calling SendAsync. - private static readonly WebSocketState[] s_validSendStates = { WebSocketState.Open, WebSocketState.CloseReceived }; + private static readonly int s_validSendStates = WebSocketStateHelper.ToFlags(WebSocketState.Open, WebSocketState.CloseReceived); /// Valid states to be in when calling ReceiveAsync. - private static readonly WebSocketState[] s_validReceiveStates = { WebSocketState.Open, WebSocketState.CloseSent }; + private static readonly int s_validReceiveStates = WebSocketStateHelper.ToFlags(WebSocketState.Open, WebSocketState.CloseSent); /// Valid states to be in when calling CloseOutputAsync. - private static readonly WebSocketState[] s_validCloseOutputStates = { WebSocketState.Open, WebSocketState.CloseReceived }; + private static readonly int s_validCloseOutputStates = WebSocketStateHelper.ToFlags(WebSocketState.Open, WebSocketState.CloseReceived); /// Valid states to be in when calling CloseAsync. - private static readonly WebSocketState[] s_validCloseStates = { WebSocketState.Open, WebSocketState.CloseReceived, WebSocketState.CloseSent }; + private static readonly int s_validCloseStates = WebSocketStateHelper.ToFlags(WebSocketState.Open, WebSocketState.CloseReceived, WebSocketState.CloseSent); /// The maximum size in bytes of a message frame header that includes mask bytes. internal const int MaxMessageHeaderLength = 14; @@ -1740,9 +1740,9 @@ private void ThrowIfOperationInProgress(bool operationCompleted, [CallerMemberNa cancellationToken); } - private void ThrowIfDisposed() => ThrowIfInvalidState(); + private void ThrowIfDisposed() => ThrowIfInvalidState(validStates: WebSocketStateHelper.All); - private void ThrowIfInvalidState(WebSocketState[]? validStates = null) + private void ThrowIfInvalidState(int validStates) { bool disposed = _disposed; WebSocketState state = _state; @@ -1761,7 +1761,7 @@ private void ThrowIfInvalidState(WebSocketState[]? validStates = null) if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"_state={state}, _disposed={disposed}, _keepAlivePingState.Exception={keepAliveException}"); - WebSocketValidate.ThrowIfInvalidState(state, disposed, keepAliveException, validStates); + WebSocketStateHelper.ThrowIfInvalidState(state, disposed, keepAliveException, validStates); } // From https://github.com/aspnet/WebSockets/blob/aa63e27fce2e9202698053620679a9a1059b501e/src/Microsoft.AspNetCore.WebSockets.Protocol/Utilities.cs#L75 diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs new file mode 100644 index 0000000000000..ac4da3075db17 --- /dev/null +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs @@ -0,0 +1,56 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Diagnostics; + +namespace System.Net.WebSockets +{ + internal static class WebSocketStateHelper + { + internal const int All = (1 << ((int)WebSocketState.Aborted + 1)) - 1; + + internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDisposed, Exception? innerException, int validStates) + { + if (!HasFlag(validStates, currentState)) + { + string invalidStateMessage = SR.Format( + SR.net_WebSockets_InvalidState, currentState, string.Join(", ", FromFlags(validStates))); + + throw new WebSocketException(WebSocketError.InvalidState, invalidStateMessage, innerException); + } + + if (innerException is not null) + { + Debug.Assert(currentState == WebSocketState.Aborted); + throw new OperationCanceledException(nameof(WebSocketState.Aborted), innerException); + } + + // Ordering is important to maintain .NET 4.5 WebSocket implementation exception behavior. + ObjectDisposedException.ThrowIf(isDisposed, typeof(WebSocket)); + } + + internal static bool HasFlag(int states, WebSocketState value) => (states & 1 << (int)value) != 0; + + internal static int ToFlags(params WebSocketState[] values) + { + int states = 0; + foreach (WebSocketState value in values) + { + states |= 1 << (int)value; + } + return states; + } + + private static IEnumerable FromFlags(int states) + { + foreach (WebSocketState value in Enum.GetValues()) + { + if (HasFlag(states, value)) + { + yield return value; + } + } + } + } +} From ce11844282e28a287fd7a21de0da333701fc2713 Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Tue, 10 Sep 2024 23:41:04 +0100 Subject: [PATCH 3/7] Roll back dispose changes, fix mutex logging --- .../src/System/Net/WebSockets/AsyncMutex.cs | 33 ++++++++----- .../System/Net/WebSockets/ManagedWebSocket.cs | 47 +++++++++---------- 2 files changed, 42 insertions(+), 38 deletions(-) diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs index 6e8d7c600bb9a..7389451fa5798 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs @@ -55,10 +55,21 @@ public Task EnterAsync(CancellationToken cancellationToken) // If cancellation was requested, bail immediately. // If the mutex is not currently held nor contended, enter immediately. // Otherwise, fall back to a more expensive likely-asynchronous wait. - return - cancellationToken.IsCancellationRequested ? Task.FromCanceled(cancellationToken) : - Interlocked.Decrement(ref _gate) >= 0 ? Task.CompletedTask : - Contended(cancellationToken); + + if (cancellationToken.IsCancellationRequested) + { + return Task.FromCanceled(cancellationToken); + } + + int gate = Interlocked.Decrement(ref _gate); + if (gate >= 0) + { + return Task.CompletedTask; + } + + if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Waiting to enter, queue length {-gate}"); + + return Contended(cancellationToken); // Everything that follows is the equivalent of: // return _sem.WaitAsync(cancellationToken); @@ -66,8 +77,6 @@ public Task EnterAsync(CancellationToken cancellationToken) Task Contended(CancellationToken cancellationToken) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Mutex queue length {-_gate}", nameof(EnterAsync)); - var w = new Waiter(this); // We need to register for cancellation before storing the waiter into the list. @@ -178,18 +187,18 @@ static void OnCancellation(object? state, CancellationToken cancellationToken) /// The caller must logically own the mutex. This is not validated. public void Exit() { - if (Interlocked.Increment(ref _gate) < 1) + // This is the equivalent of: + // _sem.Release(); + // if _sem were to be constructed as `new SemaphoreSlim(0)`. + int gate = Interlocked.Increment(ref _gate); + if (gate < 1) { - // This is the equivalent of: - // _sem.Release(); - // if _sem were to be constructed as `new SemaphoreSlim(0)`. + if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Unblocking next waiter on exit, remaining queue length {-_gate}", nameof(Exit)); Contended(); } void Contended() { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Mutex queue length {-_gate}", nameof(Exit)); - Waiter? w; lock (SyncObj) { diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs index b02f7ea783239..384d5a71fd559 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs @@ -267,36 +267,31 @@ private void DisposeCore() private static void DisposeSafe(IDisposable? resource, AsyncMutex mutex) { - if (resource is null) + if (resource is not null) { - return; - } + Task lockTask = mutex.EnterAsync(CancellationToken.None); - Task lockTask = mutex.EnterAsync(CancellationToken.None); - if (lockTask.IsCompleted) - { - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(mutex); - DisposeSafeCore(resource, mutex); - } - else - { - Observe( - DisposeSafeAsync(resource, mutex, lockTask), - thisObj: resource); - } + if (lockTask.IsCompleted) + { + if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(mutex); - static async Task DisposeSafeAsync(IDisposable resource, AsyncMutex mutex, Task lockTask) - { - await lockTask.ConfigureAwait(false); - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(mutex); - DisposeSafeCore(resource, mutex); - } + resource.Dispose(); + mutex.Exit(); - static void DisposeSafeCore(IDisposable resource, AsyncMutex mutex) - { - resource.Dispose(); - mutex.Exit(); - if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexExited(mutex); + if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexExited(mutex); + } + else + { + lockTask.GetAwaiter().UnsafeOnCompleted(() => + { + if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(mutex); + + resource.Dispose(); + mutex.Exit(); + + if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexExited(mutex); + }); + } } } From 290a0a066c42dbde4cd5b1b7a28c4e644fb3da6f Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Wed, 11 Sep 2024 00:00:20 +0100 Subject: [PATCH 4/7] Roll back observe changes --- .../System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs index 1b4d036fb0213..acc1ccd243000 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs @@ -138,24 +138,22 @@ private void Observe(ValueTask t) } } - private void Observe(Task t) => Observe(t, this); - // "Observe" any exception, ignoring it to prevent the unobserved task // exception event from being raised. - private static void Observe(Task t, object thisObj) + private void Observe(Task t) { if (t.IsCompleted) { if (t.IsFaulted) { - LogFaulted(t, thisObj); + LogFaulted(t, this); } } else { t.ContinueWith( LogFaulted, - thisObj, + this, CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); From bbda9af13c46b26fcb7ccc5d61187578ef2c16d2 Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Fri, 13 Sep 2024 16:49:02 +0100 Subject: [PATCH 5/7] Add internal flags enum for states --- .../src/System.Net.WebSockets.csproj | 1 + .../WebSockets/ManagedWebSocket.KeepAlive.cs | 4 +- .../System/Net/WebSockets/ManagedWebSocket.cs | 23 +++----- .../Net/WebSockets/ManagedWebSocketStates.cs | 21 ++++++++ .../Net/WebSockets/WebSocketStateHelper.cs | 53 +++++++++---------- 5 files changed, 55 insertions(+), 47 deletions(-) create mode 100644 src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocketStates.cs diff --git a/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj b/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj index 1f1a8046a4cf1..53547e2c89822 100644 --- a/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj +++ b/src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj @@ -18,6 +18,7 @@ + diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs index acc1ccd243000..4e38c4fc10042 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs @@ -12,8 +12,6 @@ namespace System.Net.WebSockets internal sealed partial class ManagedWebSocket : WebSocket { private bool IsUnsolicitedPongKeepAlive => _keepAlivePingState is null; - private static bool IsValidSendState(WebSocketState state) => WebSocketStateHelper.HasFlag(s_validSendStates, state); - private static bool IsValidReceiveState(WebSocketState state) => WebSocketStateHelper.HasFlag(s_validReceiveStates, state); private void HeartBeat() { @@ -39,7 +37,7 @@ private ValueTask TrySendKeepAliveFrameAsync(MessageOpcode opcode, ReadOnlyMemor { Debug.Assert((opcode is MessageOpcode.Pong) || (!IsUnsolicitedPongKeepAlive && opcode is MessageOpcode.Ping)); - if (!IsValidSendState(_state)) + if (!WebSocketStateHelper.IsValidSendState(_state)) { if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Cannot send keep-alive frame in {nameof(_state)}={_state}"); diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs index 384d5a71fd559..814f5fee82879 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs @@ -32,15 +32,6 @@ internal sealed partial class ManagedWebSocket : WebSocket /// Encoding for the payload of text messages: UTF-8 encoding that throws if invalid bytes are discovered, per the RFC. private static readonly UTF8Encoding s_textEncoding = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true); - /// Valid states to be in when calling SendAsync. - private static readonly int s_validSendStates = WebSocketStateHelper.ToFlags(WebSocketState.Open, WebSocketState.CloseReceived); - /// Valid states to be in when calling ReceiveAsync. - private static readonly int s_validReceiveStates = WebSocketStateHelper.ToFlags(WebSocketState.Open, WebSocketState.CloseSent); - /// Valid states to be in when calling CloseOutputAsync. - private static readonly int s_validCloseOutputStates = WebSocketStateHelper.ToFlags(WebSocketState.Open, WebSocketState.CloseReceived); - /// Valid states to be in when calling CloseAsync. - private static readonly int s_validCloseStates = WebSocketStateHelper.ToFlags(WebSocketState.Open, WebSocketState.CloseReceived, WebSocketState.CloseSent); - /// The maximum size in bytes of a message frame header that includes mask bytes. internal const int MaxMessageHeaderLength = 14; /// The maximum size of a control message payload. @@ -337,7 +328,7 @@ public override ValueTask SendAsync(ReadOnlyMemory buffer, WebSocketMessag try { - ThrowIfInvalidState(s_validSendStates); + ThrowIfInvalidState(WebSocketStateHelper.ValidSendStates); } catch (Exception exc) { @@ -377,7 +368,7 @@ public override Task ReceiveAsync(ArraySegment buf try { - ThrowIfInvalidState(s_validReceiveStates); + ThrowIfInvalidState(WebSocketStateHelper.ValidReceiveStates); return ReceiveAsyncPrivate(buffer, cancellationToken).AsTask(); } @@ -394,7 +385,7 @@ public override ValueTask ReceiveAsync(Memory try { - ThrowIfInvalidState(s_validReceiveStates); + ThrowIfInvalidState(WebSocketStateHelper.ValidReceiveStates); return ReceiveAsyncPrivate(buffer, cancellationToken); } @@ -413,7 +404,7 @@ public override Task CloseAsync(WebSocketCloseStatus closeStatus, string? status try { - ThrowIfInvalidState(s_validCloseStates); + ThrowIfInvalidState(WebSocketStateHelper.ValidCloseStates); } catch (Exception exc) { @@ -436,7 +427,7 @@ private async Task CloseOutputAsyncCore(WebSocketCloseStatus closeStatus, string { if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this); - ThrowIfInvalidState(s_validCloseOutputStates); + ThrowIfInvalidState(WebSocketStateHelper.ValidCloseOutputStates); await SendCloseFrameAsync(closeStatus, statusDescription, cancellationToken).ConfigureAwait(false); @@ -1735,9 +1726,9 @@ private void ThrowIfOperationInProgress(bool operationCompleted, [CallerMemberNa cancellationToken); } - private void ThrowIfDisposed() => ThrowIfInvalidState(validStates: WebSocketStateHelper.All); + private void ThrowIfDisposed() => ThrowIfInvalidState(validStates: ManagedWebSocketStates.All); - private void ThrowIfInvalidState(int validStates) + private void ThrowIfInvalidState(ManagedWebSocketStates validStates) { bool disposed = _disposed; WebSocketState state = _state; diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocketStates.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocketStates.cs new file mode 100644 index 0000000000000..1ad4cb2012969 --- /dev/null +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocketStates.cs @@ -0,0 +1,21 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace System.Net.WebSockets +{ + [Flags] + internal enum ManagedWebSocketStates + { + None = 0, + + // WebSocketState.None = 0 -- this state is invalid for the managed implementation + // WebSocketState.Connecting = 1 -- this state is invalid for the managed implementation + Open = 0x04, // WebSocketState.Open = 2 + CloseSent = 0x08, // WebSocketState.CloseSent = 3 + CloseReceived = 0x10, // WebSocketState.CloseReceived = 4 + Closed = 0x20, // WebSocketState.Closed = 5 + Aborted = 0x40, // WebSocketState.Aborted = 6 + + All = Open | CloseSent | CloseReceived | Closed | Aborted + } +} diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs index ac4da3075db17..1225d1dc8317b 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs @@ -1,28 +1,38 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections.Generic; using System.Diagnostics; namespace System.Net.WebSockets { internal static class WebSocketStateHelper { - internal const int All = (1 << ((int)WebSocketState.Aborted + 1)) - 1; - - internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDisposed, Exception? innerException, int validStates) + /// Valid states to be in when calling SendAsync. + internal const ManagedWebSocketStates ValidSendStates = ManagedWebSocketStates.Open | ManagedWebSocketStates.CloseReceived; + /// Valid states to be in when calling ReceiveAsync. + internal const ManagedWebSocketStates ValidReceiveStates = ManagedWebSocketStates.Open | ManagedWebSocketStates.CloseSent; + /// Valid states to be in when calling CloseOutputAsync. + internal const ManagedWebSocketStates ValidCloseOutputStates = ManagedWebSocketStates.Open | ManagedWebSocketStates.CloseReceived; + /// Valid states to be in when calling CloseAsync. + internal const ManagedWebSocketStates ValidCloseStates = ManagedWebSocketStates.Open | ManagedWebSocketStates.CloseReceived | ManagedWebSocketStates.CloseSent; + + internal static bool IsValidSendState(WebSocketState state) => ValidSendStates.HasFlag(ToFlag(state)); + + internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDisposed, Exception? innerException, ManagedWebSocketStates validStates) { - if (!HasFlag(validStates, currentState)) + ManagedWebSocketStates state = ToFlag(currentState); + + if (!validStates.HasFlag(state)) { string invalidStateMessage = SR.Format( - SR.net_WebSockets_InvalidState, currentState, string.Join(", ", FromFlags(validStates))); + SR.net_WebSockets_InvalidState, currentState, validStates); throw new WebSocketException(WebSocketError.InvalidState, invalidStateMessage, innerException); } if (innerException is not null) { - Debug.Assert(currentState == WebSocketState.Aborted); + Debug.Assert(state == ManagedWebSocketStates.Aborted); throw new OperationCanceledException(nameof(WebSocketState.Aborted), innerException); } @@ -30,27 +40,14 @@ internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDis ObjectDisposedException.ThrowIf(isDisposed, typeof(WebSocket)); } - internal static bool HasFlag(int states, WebSocketState value) => (states & 1 << (int)value) != 0; - - internal static int ToFlags(params WebSocketState[] values) + private static ManagedWebSocketStates ToFlag(WebSocketState value) => value switch { - int states = 0; - foreach (WebSocketState value in values) - { - states |= 1 << (int)value; - } - return states; - } - - private static IEnumerable FromFlags(int states) - { - foreach (WebSocketState value in Enum.GetValues()) - { - if (HasFlag(states, value)) - { - yield return value; - } - } - } + WebSocketState.Open => ManagedWebSocketStates.Open, + WebSocketState.CloseSent => ManagedWebSocketStates.CloseSent, + WebSocketState.CloseReceived => ManagedWebSocketStates.CloseReceived, + WebSocketState.Closed => ManagedWebSocketStates.Closed, + WebSocketState.Aborted => ManagedWebSocketStates.Aborted, + _ => throw new ArgumentOutOfRangeException(nameof(value)) + }; } } From 076b887f6a6ff4d591f6463bc144d09f31ccd131 Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Fri, 13 Sep 2024 18:02:30 +0200 Subject: [PATCH 6/7] Update src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs Co-authored-by: Miha Zupan --- .../System/Net/WebSockets/WebSocketStateHelper.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs index 1225d1dc8317b..a06026ee06229 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs @@ -40,14 +40,11 @@ internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDis ObjectDisposedException.ThrowIf(isDisposed, typeof(WebSocket)); } - private static ManagedWebSocketStates ToFlag(WebSocketState value) => value switch + private static ManagedWebSocketStates ToFlag(WebSocketState value) { - WebSocketState.Open => ManagedWebSocketStates.Open, - WebSocketState.CloseSent => ManagedWebSocketStates.CloseSent, - WebSocketState.CloseReceived => ManagedWebSocketStates.CloseReceived, - WebSocketState.Closed => ManagedWebSocketStates.Closed, - WebSocketState.Aborted => ManagedWebSocketStates.Aborted, - _ => throw new ArgumentOutOfRangeException(nameof(value)) - }; + ManagedWebSocketStates flag = (ManagedWebSocketStates)(1 << (int)value); + Debug.Assert(Enum.IsDefined(flag)); + return flag; + } } } From 07acff107ffb501a9048f3e42422e9a9d88dc262 Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Tue, 17 Sep 2024 19:14:18 +0100 Subject: [PATCH 7/7] Feedback --- .../src/System/Net/WebSockets/ManagedWebSocket.cs | 4 +++- .../src/System/Net/WebSockets/WebSocketStateHelper.cs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs index 814f5fee82879..f40da1cb06e8c 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs @@ -788,9 +788,11 @@ private async ValueTask ReceiveAsyncPrivate(Memory paylo if (NetEventSource.Log.IsEnabled()) NetEventSource.ReceiveAsyncPrivateStarted(this, payloadBuffer.Length); - CancellationTokenRegistration registration = cancellationToken.Register(static s => ((ManagedWebSocket)s!).Abort(), this); + CancellationTokenRegistration registration = default; try { + registration = cancellationToken.Register(static s => ((ManagedWebSocket)s!).Abort(), this); + await _receiveMutex.EnterAsync(cancellationToken).ConfigureAwait(false); if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexEntered(_receiveMutex); diff --git a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs index 1225d1dc8317b..a4998f2330146 100644 --- a/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs @@ -22,7 +22,7 @@ internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDis { ManagedWebSocketStates state = ToFlag(currentState); - if (!validStates.HasFlag(state)) + if ((state & validStates) == 0) { string invalidStateMessage = SR.Format( SR.net_WebSockets_InvalidState, currentState, validStates);