From b6b0fb1e0b135c774b797d7c04de7729bee99e91 Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Wed, 18 Sep 2024 14:15:13 +0100 Subject: [PATCH] WebSocket Feedback Follow-up (#107662) * Fixes * State validation update * Roll back dispose changes, fix mutex logging * Roll back observe changes * Add internal flags enum for states * Update src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs Co-authored-by: Miha Zupan * Feedback --------- Co-authored-by: Miha Zupan --- .../Net/WebSockets/WebSocketValidate.cs | 23 --------- .../src/System.Net.WebSockets.csproj | 2 + .../src/System/Net/WebSockets/AsyncMutex.cs | 33 +++++++----- .../WebSockets/ManagedWebSocket.KeepAlive.cs | 31 ++++-------- .../System/Net/WebSockets/ManagedWebSocket.cs | 25 +++------- .../Net/WebSockets/ManagedWebSocketStates.cs | 21 ++++++++ .../WebSockets/NetEventSource.WebSockets.cs | 12 ----- .../Net/WebSockets/WebSocketStateHelper.cs | 50 +++++++++++++++++++ 8 files changed, 112 insertions(+), 85 deletions(-) create mode 100644 src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocketStates.cs 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 b60a0bf8cc463..c02ed3fa73d65 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 @@ + @@ -31,6 +32,7 @@ + = 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.MutexContended(this, _gate); - 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.MutexContended(this, _gate); - 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..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 @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Buffers; using System.Buffers.Binary; using System.Diagnostics; using System.Runtime.ExceptionServices; @@ -13,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) => Array.IndexOf(s_validSendStates, state) != -1; - private static bool IsValidReceiveState(WebSocketState state) => Array.IndexOf(s_validReceiveStates, state) != -1; private void HeartBeat() { @@ -36,11 +33,11 @@ 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)) + if (!WebSocketStateHelper.IsValidSendState(_state)) { if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Cannot send keep-alive frame in {nameof(_state)}={_state}"); @@ -48,9 +45,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 +71,7 @@ private void KeepAlivePingHeartBeat() if (_keepAlivePingState.PingSent) { - if (Environment.TickCount64 > _keepAlivePingState.PingTimeoutTimestamp) + if (now > _keepAlivePingState.PingTimeoutTimestamp) { if (NetEventSource.Log.IsEnabled()) { @@ -92,7 +87,7 @@ private void KeepAlivePingHeartBeat() } else { - if (Environment.TickCount64 > _keepAlivePingState.NextPingRequestTimestamp) + if (now > _keepAlivePingState.NextPingRequestTimestamp) { _keepAlivePingState.OnNextPingRequestCore(); // we are holding the lock shouldSendPing = true; @@ -119,18 +114,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 8a26a4c29e2eb..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 @@ -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 WebSocketState[] s_validSendStates = { WebSocketState.Open, WebSocketState.CloseReceived }; - /// Valid states to be in when calling ReceiveAsync. - private static readonly WebSocketState[] s_validReceiveStates = { WebSocketState.Open, WebSocketState.CloseSent }; - /// Valid states to be in when calling CloseOutputAsync. - private static readonly WebSocketState[] s_validCloseOutputStates = { WebSocketState.Open, WebSocketState.CloseReceived }; - /// Valid states to be in when calling CloseAsync. - private static readonly WebSocketState[] s_validCloseStates = { 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); @@ -1737,9 +1728,9 @@ private void ThrowIfOperationInProgress(bool operationCompleted, [CallerMemberNa cancellationToken); } - private void ThrowIfDisposed() => ThrowIfInvalidState(); + private void ThrowIfDisposed() => ThrowIfInvalidState(validStates: ManagedWebSocketStates.All); - private void ThrowIfInvalidState(WebSocketState[]? validStates = null) + private void ThrowIfInvalidState(ManagedWebSocketStates validStates) { bool disposed = _disposed; WebSocketState state = _state; @@ -1758,7 +1749,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/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/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 // 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..1de3e3333ab31 --- /dev/null +++ b/src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs @@ -0,0 +1,50 @@ +// 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; + +namespace System.Net.WebSockets +{ + internal static class WebSocketStateHelper + { + /// 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) + { + ManagedWebSocketStates state = ToFlag(currentState); + + if ((state & validStates) == 0) + { + string invalidStateMessage = SR.Format( + SR.net_WebSockets_InvalidState, currentState, validStates); + + throw new WebSocketException(WebSocketError.InvalidState, invalidStateMessage, innerException); + } + + if (innerException is not null) + { + Debug.Assert(state == ManagedWebSocketStates.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)); + } + + private static ManagedWebSocketStates ToFlag(WebSocketState value) + { + ManagedWebSocketStates flag = (ManagedWebSocketStates)(1 << (int)value); + Debug.Assert(Enum.IsDefined(flag)); + return flag; + } + } +}