Skip to content

Commit 2381707

Browse files
CarnaViireMihaZupan
authored and
pull[bot]
committed
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 <mihazupan.zupan1@gmail.com> * Feedback --------- Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
1 parent 24b16a2 commit 2381707

File tree

8 files changed

+112
-85
lines changed

8 files changed

+112
-85
lines changed

‎src/libraries/Common/src/System/Net/WebSockets/WebSocketValidate.cs‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -37,29 +37,6 @@ internal static partial class WebSocketValidate
3737
private static readonly SearchValues<char> s_validSubprotocolChars =
3838
SearchValues.Create("!#$%&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~");
3939

40-
internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDisposed, WebSocketState[] validStates)
41-
=> ThrowIfInvalidState(currentState, isDisposed, innerException: null, validStates ?? []);
42-
43-
internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDisposed, Exception? innerException, WebSocketState[]? validStates = null)
44-
{
45-
if (validStates is not null && Array.IndexOf(validStates, currentState) == -1)
46-
{
47-
string invalidStateMessage = SR.Format(
48-
SR.net_WebSockets_InvalidState, currentState, string.Join(", ", validStates));
49-
50-
throw new WebSocketException(WebSocketError.InvalidState, invalidStateMessage, innerException);
51-
}
52-
53-
if (innerException is not null)
54-
{
55-
Debug.Assert(currentState == WebSocketState.Aborted);
56-
throw new OperationCanceledException(nameof(WebSocketState.Aborted), innerException);
57-
}
58-
59-
// Ordering is important to maintain .NET 4.5 WebSocket implementation exception behavior.
60-
ObjectDisposedException.ThrowIf(isDisposed, typeof(WebSocket));
61-
}
62-
6340
internal static void ValidateSubprotocol(string subProtocol)
6441
{
6542
ArgumentException.ThrowIfNullOrWhiteSpace(subProtocol);

‎src/libraries/System.Net.WebSockets/src/System.Net.WebSockets.csproj‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
<Compile Include="System\Net\WebSockets\Compression\WebSocketInflater.cs" />
1919
<Compile Include="System\Net\WebSockets\ManagedWebSocket.cs" />
2020
<Compile Include="System\Net\WebSockets\ManagedWebSocket.KeepAlive.cs" />
21+
<Compile Include="System\Net\WebSockets\ManagedWebSocketStates.cs" />
2122
<Compile Include="System\Net\WebSockets\NetEventSource.WebSockets.cs" />
2223
<Compile Include="System\Net\WebSockets\ValueWebSocketReceiveResult.cs" />
2324
<Compile Include="System\Net\WebSockets\WebSocket.cs" />
@@ -31,6 +32,7 @@
3132
<Compile Include="System\Net\WebSockets\WebSocketMessageFlags.cs" />
3233
<Compile Include="System\Net\WebSockets\WebSocketReceiveResult.cs" />
3334
<Compile Include="System\Net\WebSockets\WebSocketState.cs" />
35+
<Compile Include="System\Net\WebSockets\WebSocketStateHelper.cs" />
3436
<Compile Include="$(CommonPath)System\Net\WebSockets\WebSocketDefaults.cs"
3537
Link="Common\System\Net\WebSockets\WebSocketDefaults.cs" />
3638
<Compile Include="$(CommonPath)System\Net\WebSockets\WebSocketValidate.cs"

‎src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/AsyncMutex.cs‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -55,19 +55,28 @@ public Task EnterAsync(CancellationToken cancellationToken)
5555
// If cancellation was requested, bail immediately.
5656
// If the mutex is not currently held nor contended, enter immediately.
5757
// Otherwise, fall back to a more expensive likely-asynchronous wait.
58-
return
59-
cancellationToken.IsCancellationRequested ? Task.FromCanceled(cancellationToken) :
60-
Interlocked.Decrement(ref _gate) >= 0 ? Task.CompletedTask :
61-
Contended(cancellationToken);
58+
59+
if (cancellationToken.IsCancellationRequested)
60+
{
61+
return Task.FromCanceled(cancellationToken);
62+
}
63+
64+
int gate = Interlocked.Decrement(ref _gate);
65+
if (gate >= 0)
66+
{
67+
return Task.CompletedTask;
68+
}
69+
70+
if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Waiting to enter, queue length {-gate}");
71+
72+
return Contended(cancellationToken);
6273

6374
// Everything that follows is the equivalent of:
6475
// return _sem.WaitAsync(cancellationToken);
6576
// if _sem were to be constructed as `new SemaphoreSlim(0)`.
6677

6778
Task Contended(CancellationToken cancellationToken)
6879
{
69-
if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexContended(this, _gate);
70-
7180
var w = new Waiter(this);
7281

7382
// We need to register for cancellation before storing the waiter into the list.
@@ -178,18 +187,18 @@ static void OnCancellation(object? state, CancellationToken cancellationToken)
178187
/// <remarks>The caller must logically own the mutex. This is not validated.</remarks>
179188
public void Exit()
180189
{
181-
if (Interlocked.Increment(ref _gate) < 1)
190+
// This is the equivalent of:
191+
// _sem.Release();
192+
// if _sem were to be constructed as `new SemaphoreSlim(0)`.
193+
int gate = Interlocked.Increment(ref _gate);
194+
if (gate < 1)
182195
{
183-
// This is the equivalent of:
184-
// _sem.Release();
185-
// if _sem were to be constructed as `new SemaphoreSlim(0)`.
196+
if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Unblocking next waiter on exit, remaining queue length {-_gate}", nameof(Exit));
186197
Contended();
187198
}
188199

189200
void Contended()
190201
{
191-
if (NetEventSource.Log.IsEnabled()) NetEventSource.MutexContended(this, _gate);
192-
193202
Waiter? w;
194203
lock (SyncObj)
195204
{

‎src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs‎

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System.Buffers;
54
using System.Buffers.Binary;
65
using System.Diagnostics;
76
using System.Runtime.ExceptionServices;
@@ -13,8 +12,6 @@ namespace System.Net.WebSockets
1312
internal sealed partial class ManagedWebSocket : WebSocket
1413
{
1514
private bool IsUnsolicitedPongKeepAlive => _keepAlivePingState is null;
16-
private static bool IsValidSendState(WebSocketState state) => Array.IndexOf(s_validSendStates, state) != -1;
17-
private static bool IsValidReceiveState(WebSocketState state) => Array.IndexOf(s_validReceiveStates, state) != -1;
1815

1916
private void HeartBeat()
2017
{
@@ -36,21 +33,19 @@ private void UnsolicitedPongHeartBeat()
3633
TrySendKeepAliveFrameAsync(MessageOpcode.Pong));
3734
}
3835

39-
private ValueTask TrySendKeepAliveFrameAsync(MessageOpcode opcode, ReadOnlyMemory<byte>? payload = null)
36+
private ValueTask TrySendKeepAliveFrameAsync(MessageOpcode opcode, ReadOnlyMemory<byte> payload = default)
4037
{
41-
Debug.Assert(opcode is MessageOpcode.Pong || !IsUnsolicitedPongKeepAlive && opcode is MessageOpcode.Ping);
38+
Debug.Assert((opcode is MessageOpcode.Pong) || (!IsUnsolicitedPongKeepAlive && opcode is MessageOpcode.Ping));
4239

43-
if (!IsValidSendState(_state))
40+
if (!WebSocketStateHelper.IsValidSendState(_state))
4441
{
4542
if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"Cannot send keep-alive frame in {nameof(_state)}={_state}");
4643

4744
// we can't send any frames, but no need to throw as we are not observing errors anyway
4845
return ValueTask.CompletedTask;
4946
}
5047

51-
payload ??= ReadOnlyMemory<byte>.Empty;
52-
53-
return SendFrameAsync(opcode, endOfMessage: true, disableCompression: true, payload.Value, CancellationToken.None);
48+
return SendFrameAsync(opcode, endOfMessage: true, disableCompression: true, payload, CancellationToken.None);
5449
}
5550

5651
private void KeepAlivePingHeartBeat()
@@ -76,7 +71,7 @@ private void KeepAlivePingHeartBeat()
7671

7772
if (_keepAlivePingState.PingSent)
7873
{
79-
if (Environment.TickCount64 > _keepAlivePingState.PingTimeoutTimestamp)
74+
if (now > _keepAlivePingState.PingTimeoutTimestamp)
8075
{
8176
if (NetEventSource.Log.IsEnabled())
8277
{
@@ -92,7 +87,7 @@ private void KeepAlivePingHeartBeat()
9287
}
9388
else
9489
{
95-
if (Environment.TickCount64 > _keepAlivePingState.NextPingRequestTimestamp)
90+
if (now > _keepAlivePingState.NextPingRequestTimestamp)
9691
{
9792
_keepAlivePingState.OnNextPingRequestCore(); // we are holding the lock
9893
shouldSendPing = true;
@@ -119,18 +114,12 @@ private async ValueTask SendPingAsync(long pingPayload)
119114
{
120115
Debug.Assert(_keepAlivePingState != null);
121116

122-
byte[] pingPayloadBuffer = ArrayPool<byte>.Shared.Rent(sizeof(long));
117+
byte[] pingPayloadBuffer = new byte[sizeof(long)];
123118
BinaryPrimitives.WriteInt64BigEndian(pingPayloadBuffer, pingPayload);
124-
try
125-
{
126-
await TrySendKeepAliveFrameAsync(MessageOpcode.Ping, pingPayloadBuffer.AsMemory(0, sizeof(long))).ConfigureAwait(false);
127119

128-
if (NetEventSource.Log.IsEnabled()) NetEventSource.KeepAlivePingSent(this, pingPayload);
129-
}
130-
finally
131-
{
132-
ArrayPool<byte>.Shared.Return(pingPayloadBuffer);
133-
}
120+
await TrySendKeepAliveFrameAsync(MessageOpcode.Ping, pingPayloadBuffer).ConfigureAwait(false);
121+
122+
if (NetEventSource.Log.IsEnabled()) NetEventSource.KeepAlivePingSent(this, pingPayload);
134123
}
135124

136125
// "Observe" either a ValueTask result, or any exception, ignoring it

‎src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs‎

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,6 @@ internal sealed partial class ManagedWebSocket : WebSocket
3232
/// <summary>Encoding for the payload of text messages: UTF-8 encoding that throws if invalid bytes are discovered, per the RFC.</summary>
3333
private static readonly UTF8Encoding s_textEncoding = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true);
3434

35-
/// <summary>Valid states to be in when calling SendAsync.</summary>
36-
private static readonly WebSocketState[] s_validSendStates = { WebSocketState.Open, WebSocketState.CloseReceived };
37-
/// <summary>Valid states to be in when calling ReceiveAsync.</summary>
38-
private static readonly WebSocketState[] s_validReceiveStates = { WebSocketState.Open, WebSocketState.CloseSent };
39-
/// <summary>Valid states to be in when calling CloseOutputAsync.</summary>
40-
private static readonly WebSocketState[] s_validCloseOutputStates = { WebSocketState.Open, WebSocketState.CloseReceived };
41-
/// <summary>Valid states to be in when calling CloseAsync.</summary>
42-
private static readonly WebSocketState[] s_validCloseStates = { WebSocketState.Open, WebSocketState.CloseReceived, WebSocketState.CloseSent };
43-
4435
/// <summary>The maximum size in bytes of a message frame header that includes mask bytes.</summary>
4536
internal const int MaxMessageHeaderLength = 14;
4637
/// <summary>The maximum size of a control message payload.</summary>
@@ -337,7 +328,7 @@ public override ValueTask SendAsync(ReadOnlyMemory<byte> buffer, WebSocketMessag
337328

338329
try
339330
{
340-
ThrowIfInvalidState(s_validSendStates);
331+
ThrowIfInvalidState(WebSocketStateHelper.ValidSendStates);
341332
}
342333
catch (Exception exc)
343334
{
@@ -377,7 +368,7 @@ public override Task<WebSocketReceiveResult> ReceiveAsync(ArraySegment<byte> buf
377368

378369
try
379370
{
380-
ThrowIfInvalidState(s_validReceiveStates);
371+
ThrowIfInvalidState(WebSocketStateHelper.ValidReceiveStates);
381372

382373
return ReceiveAsyncPrivate<WebSocketReceiveResult>(buffer, cancellationToken).AsTask();
383374
}
@@ -394,7 +385,7 @@ public override ValueTask<ValueWebSocketReceiveResult> ReceiveAsync(Memory<byte>
394385

395386
try
396387
{
397-
ThrowIfInvalidState(s_validReceiveStates);
388+
ThrowIfInvalidState(WebSocketStateHelper.ValidReceiveStates);
398389

399390
return ReceiveAsyncPrivate<ValueWebSocketReceiveResult>(buffer, cancellationToken);
400391
}
@@ -413,7 +404,7 @@ public override Task CloseAsync(WebSocketCloseStatus closeStatus, string? status
413404

414405
try
415406
{
416-
ThrowIfInvalidState(s_validCloseStates);
407+
ThrowIfInvalidState(WebSocketStateHelper.ValidCloseStates);
417408
}
418409
catch (Exception exc)
419410
{
@@ -436,7 +427,7 @@ private async Task CloseOutputAsyncCore(WebSocketCloseStatus closeStatus, string
436427
{
437428
if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this);
438429

439-
ThrowIfInvalidState(s_validCloseOutputStates);
430+
ThrowIfInvalidState(WebSocketStateHelper.ValidCloseOutputStates);
440431

441432
await SendCloseFrameAsync(closeStatus, statusDescription, cancellationToken).ConfigureAwait(false);
442433

@@ -1737,9 +1728,9 @@ private void ThrowIfOperationInProgress(bool operationCompleted, [CallerMemberNa
17371728
cancellationToken);
17381729
}
17391730

1740-
private void ThrowIfDisposed() => ThrowIfInvalidState();
1731+
private void ThrowIfDisposed() => ThrowIfInvalidState(validStates: ManagedWebSocketStates.All);
17411732

1742-
private void ThrowIfInvalidState(WebSocketState[]? validStates = null)
1733+
private void ThrowIfInvalidState(ManagedWebSocketStates validStates)
17431734
{
17441735
bool disposed = _disposed;
17451736
WebSocketState state = _state;
@@ -1758,7 +1749,7 @@ private void ThrowIfInvalidState(WebSocketState[]? validStates = null)
17581749

17591750
if (NetEventSource.Log.IsEnabled()) NetEventSource.Trace(this, $"_state={state}, _disposed={disposed}, _keepAlivePingState.Exception={keepAliveException}");
17601751

1761-
WebSocketValidate.ThrowIfInvalidState(state, disposed, keepAliveException, validStates);
1752+
WebSocketStateHelper.ThrowIfInvalidState(state, disposed, keepAliveException, validStates);
17621753
}
17631754

17641755
// From https://github.com/aspnet/WebSockets/blob/aa63e27fce2e9202698053620679a9a1059b501e/src/Microsoft.AspNetCore.WebSockets.Protocol/Utilities.cs#L75

‎src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocketStates.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
namespace System.Net.WebSockets
5+
{
6+
[Flags]
7+
internal enum ManagedWebSocketStates
8+
{
9+
None = 0,
10+
11+
// WebSocketState.None = 0 -- this state is invalid for the managed implementation
12+
// WebSocketState.Connecting = 1 -- this state is invalid for the managed implementation
13+
Open = 0x04, // WebSocketState.Open = 2
14+
CloseSent = 0x08, // WebSocketState.CloseSent = 3
15+
CloseReceived = 0x10, // WebSocketState.CloseReceived = 4
16+
Closed = 0x20, // WebSocketState.Closed = 5
17+
Aborted = 0x40, // WebSocketState.Aborted = 6
18+
19+
All = Open | CloseSent | CloseReceived | Closed | Aborted
20+
}
21+
}

‎src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/NetEventSource.WebSockets.cs‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ internal sealed partial class NetEventSource
3434

3535
private const int MutexEnterId = SendStopId + 1;
3636
private const int MutexExitId = MutexEnterId + 1;
37-
private const int MutexContendedId = MutexExitId + 1;
3837

3938
//
4039
// Keep-Alive
@@ -185,10 +184,6 @@ private void MutexEnter(string objName, string memberName) =>
185184
private void MutexExit(string objName, string memberName) =>
186185
WriteEvent(MutexExitId, objName, memberName);
187186

188-
[Event(MutexContendedId, Keywords = Keywords.Debug, Level = EventLevel.Verbose)]
189-
private void MutexContended(string objName, string memberName, int queueLength) =>
190-
WriteEvent(MutexContendedId, objName, memberName, queueLength);
191-
192187
[NonEvent]
193188
public static void MutexEntered(object? obj, [CallerMemberName] string? memberName = null)
194189
{
@@ -203,13 +198,6 @@ public static void MutexExited(object? obj, [CallerMemberName] string? memberNam
203198
Log.MutexExit(IdOf(obj), memberName ?? MissingMember);
204199
}
205200

206-
[NonEvent]
207-
public static void MutexContended(object? obj, int gateValue, [CallerMemberName] string? memberName = null)
208-
{
209-
Debug.Assert(Log.IsEnabled());
210-
Log.MutexContended(IdOf(obj), memberName ?? MissingMember, -gateValue);
211-
}
212-
213201
//
214202
// WriteEvent overloads
215203
//

‎src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/WebSocketStateHelper.cs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Diagnostics;
5+
6+
namespace System.Net.WebSockets
7+
{
8+
internal static class WebSocketStateHelper
9+
{
10+
/// <summary>Valid states to be in when calling SendAsync.</summary>
11+
internal const ManagedWebSocketStates ValidSendStates = ManagedWebSocketStates.Open | ManagedWebSocketStates.CloseReceived;
12+
/// <summary>Valid states to be in when calling ReceiveAsync.</summary>
13+
internal const ManagedWebSocketStates ValidReceiveStates = ManagedWebSocketStates.Open | ManagedWebSocketStates.CloseSent;
14+
/// <summary>Valid states to be in when calling CloseOutputAsync.</summary>
15+
internal const ManagedWebSocketStates ValidCloseOutputStates = ManagedWebSocketStates.Open | ManagedWebSocketStates.CloseReceived;
16+
/// <summary>Valid states to be in when calling CloseAsync.</summary>
17+
internal const ManagedWebSocketStates ValidCloseStates = ManagedWebSocketStates.Open | ManagedWebSocketStates.CloseReceived | ManagedWebSocketStates.CloseSent;
18+
19+
internal static bool IsValidSendState(WebSocketState state) => ValidSendStates.HasFlag(ToFlag(state));
20+
21+
internal static void ThrowIfInvalidState(WebSocketState currentState, bool isDisposed, Exception? innerException, ManagedWebSocketStates validStates)
22+
{
23+
ManagedWebSocketStates state = ToFlag(currentState);
24+
25+
if ((state & validStates) == 0)
26+
{
27+
string invalidStateMessage = SR.Format(
28+
SR.net_WebSockets_InvalidState, currentState, validStates);
29+
30+
throw new WebSocketException(WebSocketError.InvalidState, invalidStateMessage, innerException);
31+
}
32+
33+
if (innerException is not null)
34+
{
35+
Debug.Assert(state == ManagedWebSocketStates.Aborted);
36+
throw new OperationCanceledException(nameof(WebSocketState.Aborted), innerException);
37+
}
38+
39+
// Ordering is important to maintain .NET 4.5 WebSocket implementation exception behavior.
40+
ObjectDisposedException.ThrowIf(isDisposed, typeof(WebSocket));
41+
}
42+
43+
private static ManagedWebSocketStates ToFlag(WebSocketState value)
44+
{
45+
ManagedWebSocketStates flag = (ManagedWebSocketStates)(1 << (int)value);
46+
Debug.Assert(Enum.IsDefined(flag));
47+
return flag;
48+
}
49+
}
50+
}

0 commit comments

Comments
 (0)