From 41e02e5549a7954b068f342cd9bd6611ef764cc3 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 19 Jul 2024 12:44:27 -0400 Subject: [PATCH] Remove class constraint from Interlocked.{Compare}Exchange (#104558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove class constraint from Interlocked.{Compare}Exchange Today `Interlocked.CompareExchange` and `Interlocked.Exchange` support only reference type `T`s. Now that we have corresponding {Compare}Exchange methods that support types of size 1, 2, 4, and 8, we can remove the constraint and support any `T` that's either a reference type, a primitive type, or an enum type, making the generic overload more useful and avoiding consumers needing to choose less-than-ideal types just because of the need for atomicity with Interlocked.{Compare}Exchange. --------- Co-authored-by: Michal Strehovský --- .../System/Threading/Interlocked.CoreCLR.cs | 38 --- src/coreclr/jit/importercalls.cpp | 8 +- .../src/System/Threading/Interlocked.cs | 18 -- .../TypeSystem/IL/NativeAotILProvider.cs | 12 +- .../IL/Stubs/InterlockedIntrinsics.cs | 56 +++- .../IL/ReadyToRunILProvider.cs | 10 +- src/coreclr/vm/corelib.h | 4 + src/coreclr/vm/jitinterface.cpp | 91 ++++-- src/coreclr/vm/metasig.h | 2 + .../Common/src/System/Net/StreamBuffer.cs | 12 +- .../System/Net/WebSockets/WebSocketStream.cs | 8 +- .../Collections/Concurrent/ConcurrentBag.cs | 26 +- .../tests/TypeDescriptorTests.cs | 6 +- .../src/System/IO/Compression/BrotliStream.cs | 15 +- .../Compression/DeflateZLib/DeflateStream.cs | 21 +- .../Parallel/Channels/AsynchronousChannel.cs | 24 +- .../SocketsHttpHandler/Http3Connection.cs | 14 +- .../Http/SocketsHttpHandler/HttpConnection.cs | 7 +- .../HttpContentReadStream.cs | 10 +- .../Net/Windows/HttpListener.Windows.cs | 28 +- .../Net/Windows/WebSockets/WebSocketBase.cs | 23 +- .../Net/Windows/WebSockets/WebSocketBuffer.cs | 12 +- .../WebSocketHttpListenerDuplexStream.cs | 24 +- .../src/System/Net/Mail/SmtpClient.cs | 6 +- .../src/System/Net/Quic/QuicConnection.cs | 16 +- .../src/System/Net/Quic/QuicListener.cs | 8 +- .../src/System/Net/Quic/QuicStream.Stream.cs | 16 +- .../src/System/Net/Quic/QuicStream.cs | 14 +- .../src/System/Net/HttpWebRequest.cs | 22 +- .../src/System/Net/TimerThread.cs | 4 +- .../System/Net/Security/NegotiateStream.cs | 18 +- .../src/System/Net/Security/SslStream.IO.cs | 60 ++-- .../src/System/Net/Security/SslStream.cs | 18 +- .../Fakes/FakeSslStream.Implementation.cs | 4 +- .../src/System/Net/Sockets/NetworkStream.cs | 8 +- .../System/Net/Sockets/SafeSocketHandle.cs | 5 +- .../src/System/Net/Sockets/Socket.Unix.cs | 2 +- .../src/System/Net/Sockets/Socket.cs | 6 +- .../Net/Sockets/SocketAsyncContext.Unix.cs | 42 +-- .../Net/Sockets/SocketAsyncEngine.Unix.cs | 30 +- .../Sockets/SocketAsyncEventArgs.Windows.cs | 4 +- .../Net/Sockets/SocketAsyncEventArgs.cs | 38 +-- .../src/System/Net/Sockets/TCPClient.cs | 6 +- .../src/System/Net/WebProxy.NonBrowser.cs | 6 +- .../System/Net/WebSockets/ClientWebSocket.cs | 22 +- .../src/Resources/Strings.resx | 5 +- .../src/System/Buffers/SharedArrayPool.cs | 4 +- .../Diagnostics/Tracing/ActivityTracker.cs | 15 +- .../Threading/CancellationTokenSource.cs | 49 +-- .../src/System/Threading/Interlocked.cs | 129 ++++++++ .../System/Threading/ReaderWriterLockSlim.cs | 12 +- .../System/Threading/ThreadPoolWorkQueue.cs | 89 +++--- .../WaitSubsystem.ThreadWaitInfo.Unix.cs | 10 +- .../System.Private.Uri/src/System/Uri.cs | 7 +- .../InteropServices/Marshalling/ComObject.cs | 7 +- .../Threading/Tasks/Parallel.ForEachAsync.cs | 42 +-- .../Threading/Tasks/ParallelLoopState.cs | 4 +- .../Threading/Tasks/ParallelRangeManager.cs | 10 +- .../tests/ThreadPoolTests.cs | 6 +- .../System.Threading/ref/System.Threading.cs | 4 +- .../tests/InterlockedTests.cs | 298 +++++++++++++++++- .../src/System/Threading/Interlocked.Mono.cs | 37 --- 62 files changed, 973 insertions(+), 579 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs index 93df3bdfed9fb..4a62578672c75 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Interlocked.CoreCLR.cs @@ -102,21 +102,6 @@ public static long Exchange(ref long location1, long value) [return: NotNullIfNotNull(nameof(location1))] [MethodImpl(MethodImplOptions.InternalCall)] private static extern object? ExchangeObject([NotNullIfNotNull(nameof(value))] ref object? location1, object? value); - - // The below whole method reduces to a single call to Exchange(ref object, object) but - // the JIT thinks that it will generate more native code than it actually does. - - /// Sets a variable of the specified type to a specified value and returns the original value, as an atomic operation. - /// The variable to set to the specified value. - /// The value to which the parameter is set. - /// The original value of . - /// The address of location1 is a null pointer. - /// The type to be used for and . This type must be a reference type. - [Intrinsic] - [return: NotNullIfNotNull(nameof(location1))] - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class? => - Unsafe.As(Exchange(ref Unsafe.As(ref location1), value)); #endregion #region CompareExchange @@ -183,29 +168,6 @@ public static long CompareExchange(ref long location1, long value, long comparan [MethodImpl(MethodImplOptions.InternalCall)] [return: NotNullIfNotNull(nameof(location1))] private static extern object? CompareExchangeObject(ref object? location1, object? value, object? comparand); - - // Note that getILIntrinsicImplementationForInterlocked() in vm\jitinterface.cpp replaces - // the body of the following method with the following IL: - // ldarg.0 - // ldarg.1 - // ldarg.2 - // call System.Threading.Interlocked::CompareExchange(ref Object, Object, Object) - // ret - // The workaround is no longer strictly necessary now that we have Unsafe.As but it does - // have the advantage of being less sensitive to JIT's inliner decisions. - - /// Compares two instances of the specified reference type for reference equality and, if they are equal, replaces the first one. - /// The destination, whose value is compared by reference with and possibly replaced. - /// The value that replaces the destination value if the comparison by reference results in equality. - /// The object that is compared by reference to the value at . - /// The original value in . - /// The address of is a null pointer. - /// The type to be used for , , and . This type must be a reference type. - [Intrinsic] - [MethodImpl(MethodImplOptions.AggressiveInlining)] - [return: NotNullIfNotNull(nameof(location1))] - public static T CompareExchange(ref T location1, T value, T comparand) where T : class? => - Unsafe.As(CompareExchange(ref Unsafe.As(ref location1), value, comparand)); #endregion #region Add diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 0d2045d553c82..322ce25ca96f8 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -4130,11 +4130,7 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd, case NI_System_Threading_Interlocked_Exchange: case NI_System_Threading_Interlocked_ExchangeAdd: { - assert(callType != TYP_STRUCT); - assert(sig->numArgs == 2); - var_types retType = JITtype2varType(sig->retType); - assert((genTypeSize(retType) >= 4) || (ni == NI_System_Threading_Interlocked_Exchange)); if (genTypeSize(retType) > TARGET_POINTER_SIZE) { @@ -4159,6 +4155,10 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd, break; } + assert(callType != TYP_STRUCT); + assert(sig->numArgs == 2); + assert((genTypeSize(retType) >= 4) || (ni == NI_System_Threading_Interlocked_Exchange)); + GenTree* op2 = impPopStack().val; GenTree* op1 = impPopStack().val; diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs index 09b3609b45699..0a7fc2e65f2e3 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -36,14 +36,6 @@ public static long CompareExchange(ref long location1, long value, long comparan #endif } - [Intrinsic] - [return: NotNullIfNotNull(nameof(location1))] - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static T CompareExchange(ref T location1, T value, T comparand) where T : class? - { - return Unsafe.As(CompareExchange(ref Unsafe.As(ref location1), value, comparand)); - } - [Intrinsic] [MethodImpl(MethodImplOptions.AggressiveInlining)] [return: NotNullIfNotNull(nameof(location1))] @@ -92,16 +84,6 @@ public static long Exchange(ref long location1, long value) #endif } - [Intrinsic] - [MethodImpl(MethodImplOptions.AggressiveInlining)] - [return: NotNullIfNotNull(nameof(location1))] - public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class? - { - if (Unsafe.IsNullRef(ref location1)) - ThrowHelper.ThrowNullReferenceException(); - return Unsafe.As(RuntimeImports.InterlockedExchange(ref Unsafe.As(ref location1), value)); - } - [Intrinsic] [MethodImpl(MethodImplOptions.AggressiveInlining)] [return: NotNullIfNotNull(nameof(location1))] diff --git a/src/coreclr/tools/Common/TypeSystem/IL/NativeAotILProvider.cs b/src/coreclr/tools/Common/TypeSystem/IL/NativeAotILProvider.cs index 0e9ffcc0157cc..f5296b93ec826 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/NativeAotILProvider.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/NativeAotILProvider.cs @@ -46,12 +46,6 @@ private static MethodIL TryGetIntrinsicMethodIL(MethodDesc method) switch (owningType.Name) { - case "Interlocked": - { - if (owningType.Namespace == "System.Threading") - return InterlockedIntrinsics.EmitIL(method); - } - break; case "Unsafe": { if (owningType.Namespace == "System.Runtime.CompilerServices") @@ -108,6 +102,12 @@ private static MethodIL TryGetPerInstantiationIntrinsicMethodIL(MethodDesc metho switch (owningType.Name) { + case "Interlocked": + { + if (owningType.Namespace == "System.Threading") + return InterlockedIntrinsics.EmitIL(method); + } + break; case "Activator": { TypeSystemContext context = owningType.Context; diff --git a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/InterlockedIntrinsics.cs b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/InterlockedIntrinsics.cs index 7f0a8d2f191a4..c5aa13bd3ddf1 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/InterlockedIntrinsics.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/InterlockedIntrinsics.cs @@ -19,6 +19,7 @@ public static MethodIL EmitIL( MethodDesc method) { Debug.Assert(((MetadataType)method.OwningType).Name == "Interlocked"); + Debug.Assert(!method.IsGenericMethodDefinition); if (method.HasInstantiation && method.Name == "CompareExchange") { @@ -30,22 +31,45 @@ public static MethodIL EmitIL( if (compilationModuleGroup.ContainsType(method.OwningType)) #endif // READYTORUN { - TypeDesc objectType = method.Context.GetWellKnownType(WellKnownType.Object); - MethodDesc compareExchangeObject = method.OwningType.GetKnownMethod("CompareExchange", - new MethodSignature( - MethodSignatureFlags.Static, - genericParameterCount: 0, - returnType: objectType, - parameters: new TypeDesc[] { objectType.MakeByRefType(), objectType, objectType })); - - ILEmitter emit = new ILEmitter(); - ILCodeStream codeStream = emit.NewCodeStream(); - codeStream.EmitLdArg(0); - codeStream.EmitLdArg(1); - codeStream.EmitLdArg(2); - codeStream.Emit(ILOpcode.call, emit.NewToken(compareExchangeObject)); - codeStream.Emit(ILOpcode.ret); - return emit.Link(method); + // Rewrite the generic Interlocked.CompareExchange to be a call to one of the non-generic overloads. + TypeDesc ceArgType = null; + + TypeDesc tType = method.Instantiation[0]; + if (!tType.IsValueType) + { + ceArgType = method.Context.GetWellKnownType(WellKnownType.Object); + } + else if ((tType.IsPrimitive || tType.IsEnum) && (tType.UnderlyingType.Category is not (TypeFlags.Single or TypeFlags.Double))) + { + int size = tType.GetElementSize().AsInt; + Debug.Assert(size is 1 or 2 or 4 or 8); + ceArgType = size switch + { + 1 => method.Context.GetWellKnownType(WellKnownType.Byte), + 2 => method.Context.GetWellKnownType(WellKnownType.UInt16), + 4 => method.Context.GetWellKnownType(WellKnownType.Int32), + _ => method.Context.GetWellKnownType(WellKnownType.Int64), + }; + } + + if (ceArgType is not null) + { + MethodDesc compareExchangeNonGeneric = method.OwningType.GetKnownMethod("CompareExchange", + new MethodSignature( + MethodSignatureFlags.Static, + genericParameterCount: 0, + returnType: ceArgType, + parameters: [ceArgType.MakeByRefType(), ceArgType, ceArgType])); + + ILEmitter emit = new ILEmitter(); + ILCodeStream codeStream = emit.NewCodeStream(); + codeStream.EmitLdArg(0); + codeStream.EmitLdArg(1); + codeStream.EmitLdArg(2); + codeStream.Emit(ILOpcode.call, emit.NewToken(compareExchangeNonGeneric)); + codeStream.Emit(ILOpcode.ret); + return emit.Link(method); + } } } diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs index c5e67286dc200..7fed0872c7dc5 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs @@ -85,11 +85,6 @@ private MethodIL TryGetIntrinsicMethodIL(MethodDesc method) return UnsafeIntrinsics.EmitIL(method); } - if (mdType.Name == "Interlocked" && mdType.Namespace == "System.Threading") - { - return InterlockedIntrinsics.EmitIL(_compilationModuleGroup, method); - } - return null; } @@ -114,6 +109,11 @@ private MethodIL TryGetPerInstantiationIntrinsicMethodIL(MethodDesc method) return TryGetIntrinsicMethodILForActivator(method); } + if (mdType.Name == "Interlocked" && mdType.Namespace == "System.Threading") + { + return InterlockedIntrinsics.EmitIL(_compilationModuleGroup, method); + } + return null; } diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index cca199fbe7552..76e40d4477f3e 100644 --- a/src/coreclr/vm/corelib.h +++ b/src/coreclr/vm/corelib.h @@ -694,6 +694,10 @@ DEFINE_METHOD(MEMORY_MARSHAL, GET_ARRAY_DATA_REFERENCE_MDARRAY, GetArrayDa DEFINE_CLASS(INTERLOCKED, Threading, Interlocked) DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_T, CompareExchange, GM_RefT_T_T_RetT) DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_OBJECT,CompareExchange, SM_RefObject_Object_Object_RetObject) +DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_BYTE, CompareExchange, SM_RefByte_Byte_Byte_RetByte) +DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_USHRT, CompareExchange, SM_RefUShrt_UShrt_UShrt_RetUShrt) +DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_INT, CompareExchange, SM_RefInt_Int_Int_RetInt) +DEFINE_METHOD(INTERLOCKED, COMPARE_EXCHANGE_LONG, CompareExchange, SM_RefLong_Long_Long_RetLong) DEFINE_CLASS(RAW_DATA, CompilerServices, RawData) DEFINE_FIELD(RAW_DATA, DATA, Data) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 0aa34005bdb07..3b8c32e20fcd5 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -7154,28 +7154,79 @@ bool getILIntrinsicImplementationForInterlocked(MethodDesc * ftn, if (ftn->GetMemberDef() != CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_T)->GetMemberDef()) return false; - // Get MethodDesc for non-generic System.Threading.Interlocked.CompareExchange() - MethodDesc* cmpxchgObject = CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_OBJECT); - - // Setup up the body of the method - static BYTE il[] = { - CEE_LDARG_0, - CEE_LDARG_1, - CEE_LDARG_2, - CEE_CALL,0,0,0,0, - CEE_RET - }; - - // Get the token for non-generic System.Threading.Interlocked.CompareExchange(), and patch [target] - mdMethodDef cmpxchgObjectToken = cmpxchgObject->GetMemberDef(); - il[4] = (BYTE)((int)cmpxchgObjectToken >> 0); - il[5] = (BYTE)((int)cmpxchgObjectToken >> 8); - il[6] = (BYTE)((int)cmpxchgObjectToken >> 16); - il[7] = (BYTE)((int)cmpxchgObjectToken >> 24); + // Determine the type of the generic T method parameter + _ASSERTE(ftn->HasMethodInstantiation()); + _ASSERTE(ftn->GetNumGenericMethodArgs() == 1); + TypeHandle typeHandle = ftn->GetMethodInstantiation()[0]; + + // Setup up the body of the CompareExchange methods; the method token will be patched on first use. + static BYTE il[5][9] = + { + { CEE_LDARG_0, CEE_LDARG_1, CEE_LDARG_2, CEE_CALL, 0, 0, 0, 0, CEE_RET }, // object + { CEE_LDARG_0, CEE_LDARG_1, CEE_LDARG_2, CEE_CALL, 0, 0, 0, 0, CEE_RET }, // byte + { CEE_LDARG_0, CEE_LDARG_1, CEE_LDARG_2, CEE_CALL, 0, 0, 0, 0, CEE_RET }, // ushort + { CEE_LDARG_0, CEE_LDARG_1, CEE_LDARG_2, CEE_CALL, 0, 0, 0, 0, CEE_RET }, // int + { CEE_LDARG_0, CEE_LDARG_1, CEE_LDARG_2, CEE_CALL, 0, 0, 0, 0, CEE_RET }, // long + }; + + // Based on the generic method parameter, determine which overload of CompareExchange + // to delegate to, or if we can't handle the type at all. + int ilIndex; + MethodDesc* cmpxchgMethod; + if (!typeHandle.IsValueType()) + { + ilIndex = 0; + cmpxchgMethod = CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_OBJECT); + } + else + { + CorElementType elementType = typeHandle.GetVerifierCorElementType(); + if (!CorTypeInfo::IsPrimitiveType(elementType) || + elementType == ELEMENT_TYPE_R4 || + elementType == ELEMENT_TYPE_R8) + { + return false; + } + else + { + switch (typeHandle.GetSize()) + { + case 1: + ilIndex = 1; + cmpxchgMethod = CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_BYTE); + break; + + case 2: + ilIndex = 2; + cmpxchgMethod = CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_USHRT); + break; + + case 4: + ilIndex = 3; + cmpxchgMethod = CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_INT); + break; + + case 8: + ilIndex = 4; + cmpxchgMethod = CoreLibBinder::GetMethod(METHOD__INTERLOCKED__COMPARE_EXCHANGE_LONG); + break; + + default: + _ASSERT(!"Unexpected primitive type size"); + return false; + } + } + } + + mdMethodDef cmpxchgToken = cmpxchgMethod->GetMemberDef(); + il[ilIndex][4] = (BYTE)((int)cmpxchgToken >> 0); + il[ilIndex][5] = (BYTE)((int)cmpxchgToken >> 8); + il[ilIndex][6] = (BYTE)((int)cmpxchgToken >> 16); + il[ilIndex][7] = (BYTE)((int)cmpxchgToken >> 24); // Initialize methInfo - methInfo->ILCode = const_cast(il); - methInfo->ILCodeSize = sizeof(il); + methInfo->ILCode = const_cast(il[ilIndex]); + methInfo->ILCodeSize = sizeof(il[ilIndex]); methInfo->maxStack = 3; methInfo->EHcount = 0; methInfo->options = (CorInfoOptions)0; diff --git a/src/coreclr/vm/metasig.h b/src/coreclr/vm/metasig.h index 69eae039076f8..8a926c6b79097 100644 --- a/src/coreclr/vm/metasig.h +++ b/src/coreclr/vm/metasig.h @@ -586,6 +586,8 @@ DEFINE_METASIG_T(SM(RefDec_RetVoid, r(g(DECIMAL)), v)) DEFINE_METASIG(GM(RefT_T_T_RetT, IMAGE_CEE_CS_CALLCONV_DEFAULT, 1, r(M(0)) M(0) M(0), M(0))) DEFINE_METASIG(SM(RefObject_Object_Object_RetObject, r(j) j j, j)) +DEFINE_METASIG(SM(RefByte_Byte_Byte_RetByte, r(b) b b, b)) +DEFINE_METASIG(SM(RefUShrt_UShrt_UShrt_RetUShrt, r(H) H H, H)) DEFINE_METASIG_T(SM(RefCleanupWorkListElement_RetVoid, r(C(CLEANUP_WORK_LIST_ELEMENT)), v)) DEFINE_METASIG_T(SM(RefCleanupWorkListElement_SafeHandle_RetIntPtr, r(C(CLEANUP_WORK_LIST_ELEMENT)) C(SAFE_HANDLE), I)) diff --git a/src/libraries/Common/src/System/Net/StreamBuffer.cs b/src/libraries/Common/src/System/Net/StreamBuffer.cs index 32bc0f3f4e45c..4af8e4bd75171 100644 --- a/src/libraries/Common/src/System/Net/StreamBuffer.cs +++ b/src/libraries/Common/src/System/Net/StreamBuffer.cs @@ -292,7 +292,7 @@ private sealed class ResettableValueTaskSource : IValueTaskSource private ManualResetValueTaskSourceCore _waitSource; // mutable struct, do not make this readonly private CancellationTokenRegistration _waitSourceCancellation; - private int _hasWaiter; + private bool _hasWaiter; ValueTaskSourceStatus IValueTaskSource.GetStatus(short token) => _waitSource.GetStatus(token); @@ -300,7 +300,7 @@ private sealed class ResettableValueTaskSource : IValueTaskSource void IValueTaskSource.GetResult(short token) { - Debug.Assert(_hasWaiter == 0); + Debug.Assert(!_hasWaiter); // Clean up the registration. This will wait for any in-flight cancellation to complete. _waitSourceCancellation.Dispose(); @@ -312,7 +312,7 @@ void IValueTaskSource.GetResult(short token) public void SignalWaiter() { - if (Interlocked.Exchange(ref _hasWaiter, 0) == 1) + if (Interlocked.Exchange(ref _hasWaiter, false)) { _waitSource.SetResult(true); } @@ -322,7 +322,7 @@ private void CancelWaiter(CancellationToken cancellationToken) { Debug.Assert(cancellationToken.IsCancellationRequested); - if (Interlocked.Exchange(ref _hasWaiter, 0) == 1) + if (Interlocked.Exchange(ref _hasWaiter, false)) { _waitSource.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException(cancellationToken))); } @@ -330,13 +330,13 @@ private void CancelWaiter(CancellationToken cancellationToken) public void Reset() { - if (_hasWaiter != 0) + if (_hasWaiter) { throw new InvalidOperationException("Concurrent use is not supported"); } _waitSource.Reset(); - Volatile.Write(ref _hasWaiter, 1); + Volatile.Write(ref _hasWaiter, true); } public void Wait() diff --git a/src/libraries/Common/tests/System/Net/WebSockets/WebSocketStream.cs b/src/libraries/Common/tests/System/Net/WebSockets/WebSocketStream.cs index f025a7977f28b..5666195ac8628 100644 --- a/src/libraries/Common/tests/System/Net/WebSockets/WebSocketStream.cs +++ b/src/libraries/Common/tests/System/Net/WebSockets/WebSocketStream.cs @@ -21,8 +21,8 @@ public class WebSocketStream : Stream // Used by the class to indicate that the stream is writable. private bool _writeable; - // Whether Dispose has been called. 0 == false, 1 == true - private int _disposed; + // Whether Dispose has been called. + private bool _disposed; public WebSocketStream(WebSocket socket) : this(socket, FileAccess.ReadWrite, ownsSocket: false) @@ -140,7 +140,7 @@ public void Close(int timeout) protected override void Dispose(bool disposing) { - if (Interlocked.Exchange(ref _disposed, 1) != 0) + if (Interlocked.Exchange(ref _disposed, true)) { return; } @@ -269,7 +269,7 @@ public override void SetLength(long value) private void ThrowIfDisposed() { - ObjectDisposedException.ThrowIf(_disposed != 0, this); + ObjectDisposedException.ThrowIf(_disposed, this); } private static IOException WrapException(string resourceFormatString, Exception innerException) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentBag.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentBag.cs index 544aa251d1be6..acc0f1f4d8445 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentBag.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentBag.cs @@ -628,11 +628,11 @@ private void FreezeBag(ref bool lockTaken) // Finally, wait for all unsynchronized operations on each queue to be done. for (WorkStealingQueue? queue = head; queue != null; queue = queue._nextQueue) { - if (queue._currentOp != (int)Operation.None) + if (queue._currentOp != Operation.None) { SpinWait spinner = default; do { spinner.SpinOnce(); } - while (queue._currentOp != (int)Operation.None); + while (queue._currentOp != Operation.None); } } } @@ -684,7 +684,7 @@ private sealed class WorkStealingQueue /// Number of steals; needs to be combined with to get an actual Count. private int _stealCount; /// The current queue operation. Used to quiesce before performing operations from one thread onto another. - internal volatile int _currentOp; + internal volatile Operation _currentOp; /// true if this queue's lock is held as part of a global freeze. internal bool _frozen; /// Next queue in the 's set of thread-local queues. @@ -726,14 +726,14 @@ internal void LocalPush(T item, ref long emptyToNonEmptyListTransitionCount) try { // Full fence to ensure subsequent reads don't get reordered before this - Interlocked.Exchange(ref _currentOp, (int)Operation.Add); + Interlocked.Exchange(ref _currentOp, Operation.Add); int tail = _tailIndex; // Rare corner case (at most once every 2 billion pushes on this thread): // We're going to increment the tail; if we'll overflow, then we need to reset our counts if (tail == int.MaxValue) { - _currentOp = (int)Operation.None; // set back to None temporarily to avoid a deadlock + _currentOp = Operation.None; // set back to None temporarily to avoid a deadlock lock (this) { Debug.Assert(_tailIndex == tail, "No other thread should be changing _tailIndex"); @@ -749,7 +749,7 @@ internal void LocalPush(T item, ref long emptyToNonEmptyListTransitionCount) _tailIndex = tail &= _mask; Debug.Assert(_headIndex - _tailIndex <= 0); - Interlocked.Exchange(ref _currentOp, (int)Operation.Add); // ensure subsequent reads aren't reordered before this + Interlocked.Exchange(ref _currentOp, Operation.Add); // ensure subsequent reads aren't reordered before this } } @@ -778,7 +778,7 @@ internal void LocalPush(T item, ref long emptyToNonEmptyListTransitionCount) else { // We need to contend with foreign operations (e.g. steals, enumeration, etc.), so we lock. - _currentOp = (int)Operation.None; // set back to None to avoid a deadlock + _currentOp = Operation.None; // set back to None to avoid a deadlock Monitor.Enter(this, ref lockTaken); head = _headIndex; @@ -830,7 +830,7 @@ internal void LocalPush(T item, ref long emptyToNonEmptyListTransitionCount) } finally { - _currentOp = (int)Operation.None; + _currentOp = Operation.None; if (lockTaken) { Monitor.Exit(this); @@ -874,7 +874,7 @@ internal bool TryLocalPop([MaybeNullWhen(false)] out T result) // If the read of _headIndex moved before this write to _tailIndex, we could erroneously end up // popping an element that's concurrently being stolen, leading to the same element being // dequeued from the bag twice. - _currentOp = (int)Operation.Take; + _currentOp = Operation.Take; Interlocked.Exchange(ref _tailIndex, --tail); // If there is no interaction with a steal, we can head down the fast path. @@ -895,7 +895,7 @@ internal bool TryLocalPop([MaybeNullWhen(false)] out T result) else { // Interaction with steals: 0 or 1 elements left. - _currentOp = (int)Operation.None; // set back to None to avoid a deadlock + _currentOp = Operation.None; // set back to None to avoid a deadlock Monitor.Enter(this, ref lockTaken); if (_headIndex - tail <= 0) { @@ -920,7 +920,7 @@ internal bool TryLocalPop([MaybeNullWhen(false)] out T result) } finally { - _currentOp = (int)Operation.None; + _currentOp = Operation.None; if (lockTaken) { Monitor.Exit(this); @@ -975,14 +975,14 @@ internal bool TrySteal([MaybeNullWhen(false)] out T result, bool take) // is in progress, as add operations need to accurately count transitions // from empty to non-empty, and they can only do that if there are no concurrent // steal operations happening at the time. - if ((head - (_tailIndex - 2) >= 0) && _currentOp == (int)Operation.Add) + if ((head - (_tailIndex - 2) >= 0) && _currentOp == Operation.Add) { SpinWait spinner = default; do { spinner.SpinOnce(); } - while (_currentOp == (int)Operation.Add); + while (_currentOp == Operation.Add); } // Increment head to tentatively take an element: a full fence is used to ensure the read diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs index a9b6a2cd70919..6fb9f57edc479 100644 --- a/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs +++ b/src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs @@ -1276,11 +1276,11 @@ class TwiceDerivedCultureInfo : DerivedCultureInfo { } - private long _concurrentError = 0; + private volatile bool _concurrentError; private bool ConcurrentError { - get => Interlocked.Read(ref _concurrentError) == 1; - set => Interlocked.Exchange(ref _concurrentError, value ? 1 : 0); + get => _concurrentError; + set => Interlocked.Exchange(ref _concurrentError, value); } private void ConcurrentTest(TypeWithProperty instance) diff --git a/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs b/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs index a7a9e603f1a5b..002186c9b3651 100644 --- a/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs +++ b/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs @@ -128,7 +128,7 @@ private void ReleaseStateForDispose() if (buffer != null) { _buffer = null!; - if (!AsyncOperationIsActive) + if (!_activeAsyncOperation) { ArrayPool.Shared.Return(buffer); } @@ -170,18 +170,19 @@ public override long Position /// The length of the stream. public override void SetLength(long value) => throw new NotSupportedException(); - private int _activeAsyncOperation; // 1 == true, 0 == false - private bool AsyncOperationIsActive => _activeAsyncOperation != 0; + private volatile bool _activeAsyncOperation; private void EnsureNoActiveAsyncOperation() { - if (AsyncOperationIsActive) + if (_activeAsyncOperation) + { ThrowInvalidBeginCall(); + } } private void AsyncOperationStarting() { - if (Interlocked.Exchange(ref _activeAsyncOperation, 1) != 0) + if (Interlocked.Exchange(ref _activeAsyncOperation, true)) { ThrowInvalidBeginCall(); } @@ -189,8 +190,8 @@ private void AsyncOperationStarting() private void AsyncOperationCompleting() { - Debug.Assert(_activeAsyncOperation == 1); - Volatile.Write(ref _activeAsyncOperation, 0); + Debug.Assert(_activeAsyncOperation); + _activeAsyncOperation = false; } private static void ThrowInvalidBeginCall() => diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs index 07af0f3a93efb..2ae2804ef6f9a 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs @@ -20,7 +20,7 @@ public partial class DeflateStream : Stream private Inflater? _inflater; private Deflater? _deflater; private byte[]? _buffer; - private int _activeAsyncOperation; // 1 == true, 0 == false + private volatile bool _activeAsyncOperation; private bool _wroteBytes; internal DeflateStream(Stream stream, CompressionMode mode, long uncompressedSize) : this(stream, mode, leaveOpen: false, ZLibNative.Deflate_DefaultWindowBits, uncompressedSize) @@ -698,7 +698,7 @@ protected override void Dispose(bool disposing) if (buffer != null) { _buffer = null; - if (!AsyncOperationIsActive) + if (!_activeAsyncOperation) { ArrayPool.Shared.Return(buffer); } @@ -751,7 +751,7 @@ async ValueTask Core() if (buffer != null) { _buffer = null; - if (!AsyncOperationIsActive) + if (!_activeAsyncOperation) { ArrayPool.Shared.Return(buffer); } @@ -1059,24 +1059,27 @@ public override void Flush() { } public override void SetLength(long value) { throw new NotSupportedException(); } } - private bool AsyncOperationIsActive => _activeAsyncOperation != 0; - private void EnsureNoActiveAsyncOperation() { - if (AsyncOperationIsActive) + if (_activeAsyncOperation) + { ThrowInvalidBeginCall(); + } } private void AsyncOperationStarting() { - if (Interlocked.Exchange(ref _activeAsyncOperation, 1) != 0) + if (Interlocked.Exchange(ref _activeAsyncOperation, true)) { ThrowInvalidBeginCall(); } } - private void AsyncOperationCompleting() => - Volatile.Write(ref _activeAsyncOperation, 0); + private void AsyncOperationCompleting() + { + Debug.Assert(_activeAsyncOperation); + _activeAsyncOperation = false; + } private static void ThrowInvalidBeginCall() => throw new InvalidOperationException(SR.InvalidBeginCall); diff --git a/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Channels/AsynchronousChannel.cs b/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Channels/AsynchronousChannel.cs index 775228fb66c5f..6bffc272ee8e2 100644 --- a/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Channels/AsynchronousChannel.cs +++ b/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Channels/AsynchronousChannel.cs @@ -86,10 +86,10 @@ internal sealed class AsynchronousChannel : IDisposable private ManualResetEventSlim? _producerEvent; private IntValueEvent? _consumerEvent; - // These two-valued ints track whether a producer or consumer _might_ be waiting. They are marked + // These bools track whether a producer or consumer _might_ be waiting. They are marked // volatile because they are used in synchronization critical regions of code (see usage below). - private volatile int _producerIsWaiting; - private volatile int _consumerIsWaiting; + private volatile bool _producerIsWaiting; + private volatile bool _consumerIsWaiting; private readonly CancellationToken _cancellationToken; //----------------------------------------------------------------------------------- @@ -309,11 +309,11 @@ private void EnqueueChunk(T[] chunk) // our producer index doesn't pass the read of the consumer waiting flags; the CLR memory // model unfortunately permits this reordering. That is handled by using a CAS above.) - if (_consumerIsWaiting == 1 && !IsChunkBufferEmpty) + if (_consumerIsWaiting && !IsChunkBufferEmpty) { TraceHelpers.TraceInfo("AsynchronousChannel::EnqueueChunk - producer waking consumer"); Debug.Assert(_consumerEvent != null); - _consumerIsWaiting = 0; + _consumerIsWaiting = false; _consumerEvent.Set(_index); } } @@ -341,7 +341,7 @@ private void WaitUntilNonFull() // very quickly, suddenly seeing an empty queue. This would lead to deadlock // if we aren't careful. Therefore we check the empty/full state AGAIN after // setting our flag to see if a real wait is warranted. - Interlocked.Exchange(ref _producerIsWaiting, 1); + Interlocked.Exchange(ref _producerIsWaiting, true); // (We have to prevent the reads that go into determining whether the buffer // is full from moving before the write to the producer-wait flag. Hence the CAS.) @@ -359,7 +359,7 @@ private void WaitUntilNonFull() else { // Reset the flags, we don't actually have to wait after all. - _producerIsWaiting = 0; + _producerIsWaiting = false; } } while (IsFull); @@ -558,7 +558,7 @@ private bool TryDequeueChunk([NotNullWhen(true)] ref T[]? chunk, ref bool isDone // very quickly, suddenly seeing a full queue. This would lead to deadlock // if we aren't careful. Therefore we check the empty/full state AGAIN after // setting our flag to see if a real wait is warranted. - Interlocked.Exchange(ref _consumerIsWaiting, 1); + Interlocked.Exchange(ref _consumerIsWaiting, true); // (We have to prevent the reads that go into determining whether the buffer // is full from moving before the write to the producer-wait flag. Hence the CAS.) @@ -580,7 +580,7 @@ private bool TryDequeueChunk([NotNullWhen(true)] ref T[]? chunk, ref bool isDone { // Reset the wait flags, we don't need to wait after all. We loop back around // and recheck that the queue isn't empty, done, etc. - _consumerIsWaiting = 0; + _consumerIsWaiting = false; } } @@ -624,11 +624,11 @@ private T[] InternalDequeueChunk() // that the write to _consumerBufferIndex doesn't pass the read of the wait-flags; the CLR memory // model sadly permits this reordering. Hence the CAS above.) - if (_producerIsWaiting == 1 && !IsFull) + if (_producerIsWaiting && !IsFull) { TraceHelpers.TraceInfo("BoundedSingleLockFreeChannel::DequeueChunk - consumer waking producer"); Debug.Assert(_producerEvent != null); - _producerIsWaiting = 0; + _producerIsWaiting = false; _producerEvent.Set(); } @@ -643,7 +643,7 @@ private T[] InternalDequeueChunk() internal void DoneWithDequeueWait() { // On our way out, be sure to reset the flags. - _consumerIsWaiting = 0; + _consumerIsWaiting = false; } //----------------------------------------------------------------------------------- diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs index d2f1389ae1207..379c97d5c7075 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs @@ -38,10 +38,10 @@ internal sealed class Http3Connection : HttpConnectionBase // https://www.rfc-editor.org/rfc/rfc9114.html#section-7.2.4.1-2.2.1 private uint _maxHeaderListSize = uint.MaxValue; // Defaults to infinite - // Once the server's streams are received, these are set to 1. Further receipt of these streams results in a connection error. - private int _haveServerControlStream; - private int _haveServerQpackDecodeStream; - private int _haveServerQpackEncodeStream; + // Once the server's streams are received, these are set to true. Further receipt of these streams results in a connection error. + private bool _haveServerControlStream; + private bool _haveServerQpackDecodeStream; + private bool _haveServerQpackEncodeStream; // A connection-level error will abort any future operations. private Exception? _abortException; @@ -598,7 +598,7 @@ private async Task ProcessServerStreamAsync(QuicStream stream) switch (buffer.ActiveSpan[0]) { case (byte)Http3StreamType.Control: - if (Interlocked.Exchange(ref _haveServerControlStream, 1) != 0) + if (Interlocked.Exchange(ref _haveServerControlStream, true)) { // A second control stream has been received. throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.StreamCreationError); @@ -614,7 +614,7 @@ private async Task ProcessServerStreamAsync(QuicStream stream) await ProcessServerControlStreamAsync(stream, bufferCopy).ConfigureAwait(false); return; case (byte)Http3StreamType.QPackDecoder: - if (Interlocked.Exchange(ref _haveServerQpackDecodeStream, 1) != 0) + if (Interlocked.Exchange(ref _haveServerQpackDecodeStream, true)) { // A second QPack decode stream has been received. throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.StreamCreationError); @@ -625,7 +625,7 @@ private async Task ProcessServerStreamAsync(QuicStream stream) await stream.CopyToAsync(Stream.Null).ConfigureAwait(false); return; case (byte)Http3StreamType.QPackEncoder: - if (Interlocked.Exchange(ref _haveServerQpackEncodeStream, 1) != 0) + if (Interlocked.Exchange(ref _haveServerQpackEncodeStream, true)) { // A second QPack encode stream has been received. throw HttpProtocolException.CreateHttp3ConnectionException(Http3ErrorCode.StreamCreationError); diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs index e71c4c743d631..0b03489d93aff 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs @@ -67,8 +67,7 @@ internal sealed partial class HttpConnection : HttpConnectionBase private bool _canRetry; private bool _connectionClose; // Connection: close was seen on last response - private const int Status_Disposed = 1; - private int _disposed; + private volatile bool _disposed; public HttpConnection( HttpConnectionPool pool, @@ -98,7 +97,7 @@ private void Dispose(bool disposing) { // Ensure we're only disposed once. Dispose could be called concurrently, for example, // if the request and the response were running concurrently and both incurred an exception. - if (Interlocked.Exchange(ref _disposed, Status_Disposed) != Status_Disposed) + if (!Interlocked.Exchange(ref _disposed, true)) { if (NetEventSource.Log.IsEnabled()) Trace("Connection closing."); @@ -873,7 +872,7 @@ public async Task SendAsync(HttpRequestMessage request, boo // In case the connection is disposed, it's most probable that // expect100Continue timer expired and request content sending failed. // We're awaiting the task to propagate the exception in this case. - if (Volatile.Read(ref _disposed) == Status_Disposed) + if (_disposed) { try { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs index 02ba78ab99ca9..bfdbfb780fbbb 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs @@ -11,13 +11,13 @@ internal sealed partial class HttpConnection { internal abstract class HttpContentReadStream : HttpContentStream { - private int _disposed; // 0==no, 1==yes + private bool _disposed; public HttpContentReadStream(HttpConnection connection) : base(connection) { } - public sealed override bool CanRead => _disposed == 0; + public sealed override bool CanRead => !_disposed; public sealed override bool CanWrite => false; public sealed override void Write(ReadOnlySpan buffer) => throw new NotSupportedException(SR.net_http_content_readonly_stream); @@ -26,7 +26,7 @@ public HttpContentReadStream(HttpConnection connection) : base(connection) public virtual bool NeedsDrain => false; - protected bool IsDisposed => _disposed == 1; + protected bool IsDisposed => _disposed; protected bool CanReadFromConnection { @@ -35,7 +35,7 @@ protected bool CanReadFromConnection // _connection == null typically means that we have finished reading the response. // Cancellation may lead to a state where a disposed _connection is not null. HttpConnection? connection = _connection; - return connection != null && connection._disposed != Status_Disposed; + return connection != null && !connection._disposed; } } @@ -52,7 +52,7 @@ protected override void Dispose(bool disposing) // response stream and response content) will kick off multiple concurrent draining // operations. Also don't delegate to the base if Dispose has already been called, // as doing so will end up disposing of the connection before we're done draining. - if (Interlocked.Exchange(ref _disposed, 1) != 0) + if (Interlocked.Exchange(ref _disposed, true)) { return; } diff --git a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs index 899fff5d788eb..9315b0c511fb8 100644 --- a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs +++ b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs @@ -1535,7 +1535,15 @@ private sealed class DisconnectAsyncResult : IAsyncResult private readonly ulong _connectionId; private readonly HttpListenerSession _listenerSession; private readonly NativeOverlapped* _nativeOverlapped; - private int _ownershipState; // 0 = normal, 1 = in HandleAuthentication(), 2 = disconnected, 3 = cleaned up + private OwnershipState _ownershipState; + + private enum OwnershipState + { + Normal, + InHandleAuthentication, + Disconnected, + CleanedUp + } internal NativeOverlapped* NativeOverlapped { @@ -1577,7 +1585,7 @@ public bool IsCompleted internal unsafe DisconnectAsyncResult(HttpListenerSession session, ulong connectionId) { if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"HttpListener: {session.Listener}, ConnectionId: {connectionId}"); - _ownershipState = 1; + _ownershipState = OwnershipState.InHandleAuthentication; _listenerSession = session; _connectionId = connectionId; @@ -1588,23 +1596,23 @@ internal unsafe DisconnectAsyncResult(HttpListenerSession session, ulong connect internal bool StartOwningDisconnectHandling() { - int oldValue; + OwnershipState oldValue; SpinWait spin = default; - while ((oldValue = Interlocked.CompareExchange(ref _ownershipState, 1, 0)) == 2) + while ((oldValue = Interlocked.CompareExchange(ref _ownershipState, OwnershipState.InHandleAuthentication, OwnershipState.Normal)) == OwnershipState.Disconnected) { // Must block until it equals 3 - we must be in the callback right now. spin.SpinOnce(); } - Debug.Assert(oldValue != 1, "StartOwningDisconnectHandling called twice."); - return oldValue < 2; + Debug.Assert(oldValue != OwnershipState.InHandleAuthentication, "StartOwningDisconnectHandling called twice."); + return oldValue < OwnershipState.Disconnected; } internal void FinishOwningDisconnectHandling() { // If it got disconnected, run the disconnect code. - if (Interlocked.CompareExchange(ref _ownershipState, 0, 1) == 2) + if (Interlocked.CompareExchange(ref _ownershipState, OwnershipState.Normal, OwnershipState.InHandleAuthentication) == OwnershipState.Disconnected) { HandleDisconnect(); } @@ -1620,7 +1628,7 @@ private static unsafe void IOCompleted(DisconnectAsyncResult asyncResult, Native if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, "_connectionId:" + asyncResult._connectionId); asyncResult._listenerSession.RequestQueueBoundHandle.FreeNativeOverlapped(nativeOverlapped); - if (Interlocked.Exchange(ref asyncResult._ownershipState, 2) == 0) + if (Interlocked.Exchange(ref asyncResult._ownershipState, OwnershipState.Disconnected) == OwnershipState.Normal) { asyncResult.HandleDisconnect(); } @@ -1655,8 +1663,8 @@ private void HandleDisconnect() identity.Dispose(); } - int oldValue = Interlocked.Exchange(ref _ownershipState, 3); - Debug.Assert(oldValue == 2, $"Expected OwnershipState of 2, saw {oldValue}."); + OwnershipState oldValue = Interlocked.Exchange(ref _ownershipState, OwnershipState.CleanedUp); + Debug.Assert(oldValue == OwnershipState.Disconnected, $"Expected OwnershipState of Disconnected, saw {oldValue}."); } internal WindowsPrincipal? AuthenticatedConnection { get; set; } diff --git a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs index c1d2e8f81324c..982a4766538df 100644 --- a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs +++ b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBase.cs @@ -50,7 +50,7 @@ internal abstract class WebSocketBase : WebSocket, IDisposable private volatile WebSocketOperation.CloseOutputOperation? _closeOutputOperation; private Nullable _closeStatus; private string? _closeStatusDescription; - private int _receiveState; + private ReceiveState _receiveState; private Exception? _pendingException; protected WebSocketBase(Stream innerStream, @@ -1199,9 +1199,9 @@ private void ThrowIfDisposed() ObjectDisposedException.ThrowIf(_isDisposed, this); } - private void UpdateReceiveState(int newReceiveState, int expectedReceiveState) + private void UpdateReceiveState(ReceiveState newReceiveState, ReceiveState expectedReceiveState) { - int receiveState; + ReceiveState receiveState; if ((receiveState = Interlocked.Exchange(ref _receiveState, newReceiveState)) != expectedReceiveState) { Debug.Fail($"'_receiveState' had an invalid value '{receiveState}'. The expected value was '{expectedReceiveState}'."); @@ -1588,7 +1588,7 @@ protected virtual void ProcessAction_IndicateReceiveComplete( public sealed class ReceiveOperation : WebSocketOperation { - private int _receiveState; + private ReceiveState _receiveState; private bool _pongReceived; private bool _receiveCompleted; @@ -1614,8 +1614,7 @@ protected override void Initialize(Nullable> buffer, Cancella _receiveCompleted = false; _webSocket.ThrowIfDisposed(); - int originalReceiveState = Interlocked.CompareExchange(ref _webSocket._receiveState, - ReceiveState.Application, ReceiveState.Idle); + ReceiveState originalReceiveState = Interlocked.CompareExchange(ref _webSocket._receiveState, ReceiveState.Application, ReceiveState.Idle); switch (originalReceiveState) { @@ -1709,7 +1708,7 @@ protected override void ProcessAction_IndicateReceiveComplete( { ArraySegment payload; WebSocketMessageType messageType = GetMessageType(bufferType); - int newReceiveState = ReceiveState.Idle; + ReceiveState newReceiveState = ReceiveState.Idle; if (bufferType == WebSocketProtocolComponent.BufferType.Close) { @@ -2156,12 +2155,12 @@ internal interface IWebSocketStream Task CloseNetworkConnectionAsync(CancellationToken cancellationToken); } - private static class ReceiveState + private enum ReceiveState { - internal const int SendOperation = -1; - internal const int Idle = 0; - internal const int Application = 1; - internal const int PayloadAvailable = 2; + SendOperation = -1, + Idle = 0, + Application = 1, + PayloadAvailable = 2, } } } diff --git a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBuffer.cs b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBuffer.cs index 245fb112d9314..b1426b89dad4c 100644 --- a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBuffer.cs +++ b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBuffer.cs @@ -44,7 +44,7 @@ internal sealed class WebSocketBuffer : IDisposable private ArraySegment _pinnedSendBuffer; private GCHandle _pinnedSendBufferHandle; private int _stateWhenDisposing = int.MinValue; - private int _sendBufferState; + private SendBufferState _sendBufferState; private WebSocketBuffer(ArraySegment internalBuffer, int receiveBufferSize, int sendBufferSize) { @@ -170,7 +170,7 @@ internal void PinSendBuffer(ArraySegment payload, out bool bufferHasBeenPi { bufferHasBeenPinned = false; WebSocketValidate.ValidateBuffer(payload.Array!, payload.Offset, payload.Count); - int previousState = Interlocked.Exchange(ref _sendBufferState, SendBufferState.SendPayloadSpecified); + SendBufferState previousState = Interlocked.Exchange(ref _sendBufferState, SendBufferState.SendPayloadSpecified); if (previousState != SendBufferState.None) { @@ -274,7 +274,7 @@ internal bool IsPinnedSendPayloadBuffer(Interop.WebSocket.Buffer buffer, // This method is only thread safe for races between Abort and at most 1 uncompleted send operation internal void ReleasePinnedSendBuffer() { - int previousState = Interlocked.Exchange(ref _sendBufferState, SendBufferState.None); + SendBufferState previousState = Interlocked.Exchange(ref _sendBufferState, SendBufferState.None); if (previousState != SendBufferState.SendPayloadSpecified) { @@ -662,10 +662,10 @@ private static int GetInternalBufferSize(int receiveBufferSize, int sendBufferSi return 2 * receiveBufferSize + nativeSendBufferSize + NativeOverheadBufferSize + s_PropertyBufferSize; } - private static class SendBufferState + private enum SendBufferState { - public const int None = 0; - public const int SendPayloadSpecified = 1; + None = 0, + SendPayloadSpecified = 1, } private sealed class PayloadReceiveResult diff --git a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketHttpListenerDuplexStream.cs b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketHttpListenerDuplexStream.cs index d694448435d1a..229d4bcba99bd 100644 --- a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketHttpListenerDuplexStream.cs +++ b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketHttpListenerDuplexStream.cs @@ -30,7 +30,7 @@ internal sealed class WebSocketHttpListenerDuplexStream : Stream, WebSocketBase. private HttpListenerAsyncEventArgs? _readEventArgs; private TaskCompletionSource? _writeTaskCompletionSource; private TaskCompletionSource? _readTaskCompletionSource; - private int _cleanedUp; + private bool _cleanedUp; #if DEBUG private sealed class OutstandingOperations @@ -595,7 +595,7 @@ public async Task CloseNetworkConnectionAsync(CancellationToken cancellationToke protected override void Dispose(bool disposing) { - if (disposing && Interlocked.Exchange(ref _cleanedUp, 1) == 0) + if (disposing && !Interlocked.Exchange(ref _cleanedUp, true)) { _readTaskCompletionSource?.TrySetCanceled(); @@ -716,10 +716,14 @@ private static void OnReadCompleted(object? sender, HttpListenerAsyncEventArgs e internal sealed class HttpListenerAsyncEventArgs : EventArgs, IDisposable { - private const int Free = 0; - private const int InProgress = 1; - private const int Disposed = 2; - private int _operating; + private OperatingState _operating; + + private enum OperatingState + { + Free = 0, + InProgress = 1, + Disposed = 2, + } private bool _disposeCalled; private unsafe NativeOverlapped* _ptrNativeOverlapped; @@ -783,7 +787,7 @@ public IList>? BufferList Debug.Assert(!_shouldCloseOutput, "'m_ShouldCloseOutput' MUST be 'false' at this point."); Debug.Assert(value == null || _buffer == null, "Either 'm_Buffer' or 'm_BufferList' MUST be NULL."); - Debug.Assert(_operating == Free, + Debug.Assert(_operating == OperatingState.Free, "This property can only be modified if no IO operation is outstanding."); Debug.Assert(value == null || value.Count == 2, "This list can only be 'NULL' or MUST have exactly '2' items."); @@ -883,7 +887,7 @@ public void Dispose() _disposeCalled = true; // Check if this object is in-use for an async socket operation. - if (Interlocked.CompareExchange(ref _operating, Disposed, Free) != Free) + if (Interlocked.CompareExchange(ref _operating, OperatingState.Disposed, OperatingState.Free) != OperatingState.Free) { // Either already disposed or will be disposed when current operation completes. return; @@ -930,7 +934,7 @@ private unsafe void FreeOverlapped(bool checkForShutdown) internal void StartOperationCommon(ThreadPoolBoundHandle boundHandle) { // Change status to "in-use". - if (Interlocked.CompareExchange(ref _operating, InProgress, Free) != Free) + if (Interlocked.CompareExchange(ref _operating, OperatingState.InProgress, OperatingState.Free) != OperatingState.Free) { // If it was already "in-use" check if Dispose was called. ObjectDisposedException.ThrowIf(_disposeCalled, this); @@ -1039,7 +1043,7 @@ internal void Complete() { FreeOverlapped(false); // Mark as not in-use - Interlocked.Exchange(ref _operating, Free); + Interlocked.Exchange(ref _operating, OperatingState.Free); // Check for deferred Dispose(). // The deferred Dispose is not guaranteed if Dispose is called while an operation is in progress. diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs index 26a496f87a635..884d3b18a3866 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs @@ -739,7 +739,7 @@ public Task SendMailAsync(MailMessage message, CancellationToken cancellationTok CancellationTokenRegistration ctr = default; // Indicates whether the CTR has been set - captured in handler - int state = 0; + bool ctrSet = false; // Register a handler that will transfer completion results to the TCS Task SendCompletedEventHandler? handler = null; @@ -750,7 +750,7 @@ public Task SendMailAsync(MailMessage message, CancellationToken cancellationTok try { ((SmtpClient)sender).SendCompleted -= handler; - if (Interlocked.Exchange(ref state, 1) != 0) + if (Interlocked.Exchange(ref ctrSet, true)) { // A CTR has been set, we have to wait until it completes before completing the task ctr.Dispose(); @@ -783,7 +783,7 @@ public Task SendMailAsync(MailMessage message, CancellationToken cancellationTok ((SmtpClient)s!).SendAsyncCancel(); }, this); - if (Interlocked.Exchange(ref state, 1) != 0) + if (Interlocked.Exchange(ref ctrSet, true)) { // SendCompleted was already invoked, ensure the CTR completes before returning the task ctr.Dispose(); 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 d5c76ed4b0949..f57fd7651c3ed 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 @@ -109,9 +109,9 @@ static async ValueTask StartConnectAsync(QuicClientConnectionOpt private readonly MsQuicContextSafeHandle _handle; /// - /// Set to non-zero once disposed. Prevents double and/or concurrent disposal. + /// Set to true once disposed. Prevents double and/or concurrent disposal. /// - private int _disposed; + private bool _disposed; private readonly ValueTaskSource _connectedTcs = new ValueTaskSource(); private readonly ResettableValueTaskSource _shutdownTcs = new ResettableValueTaskSource() @@ -502,7 +502,7 @@ private void DecrementStreamCapacity(QuicStreamType streamType) /// An asynchronous task that completes with the opened . public async ValueTask OpenOutboundStreamAsync(QuicStreamType type, CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); QuicStream? stream = null; try @@ -524,7 +524,7 @@ public async ValueTask OpenOutboundStreamAsync(QuicStreamType type, } // Propagate ODE if disposed in the meantime. - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); // Propagate connection error when the connection was closed (remotely = ABORTED / locally = INVALID_STATE). if (ex is QuicException qex && qex.QuicError == QuicError.InternalError && @@ -544,7 +544,7 @@ public async ValueTask OpenOutboundStreamAsync(QuicStreamType type, /// An asynchronous task that completes with the accepted . public async ValueTask AcceptInboundStreamAsync(CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); if (!_canAccept) { @@ -583,7 +583,7 @@ public async ValueTask AcceptInboundStreamAsync(CancellationToken ca /// An asynchronous task that completes when the connection is closed. public ValueTask CloseAsync(long errorCode, CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); ThrowHelper.ValidateErrorCode(nameof(errorCode), errorCode, $"{nameof(CloseAsync)}.{nameof(errorCode)}"); if (_shutdownTcs.TryGetValueTask(out ValueTask valueTask, this, cancellationToken)) @@ -645,7 +645,7 @@ private unsafe int HandleEventShutdownComplete() // make sure we log at least some secrets in case of shutdown before handshake completes. _tlsSecret?.WriteSecret(); - Exception exception = ExceptionDispatchInfo.SetCurrentStackTrace(_disposed == 1 ? new ObjectDisposedException(GetType().FullName) : ThrowHelper.GetOperationAbortedException()); + Exception exception = ExceptionDispatchInfo.SetCurrentStackTrace(_disposed ? new ObjectDisposedException(GetType().FullName) : ThrowHelper.GetOperationAbortedException()); _connectionCloseTcs.TrySetException(exception); _acceptQueue.Writer.TryComplete(exception); _connectedTcs.TrySetException(exception); @@ -783,7 +783,7 @@ private static unsafe int NativeCallback(QUIC_HANDLE* connection, void* context, /// A task that represents the asynchronous dispose operation. public async ValueTask DisposeAsync() { - if (Interlocked.Exchange(ref _disposed, 1) != 0) + if (Interlocked.Exchange(ref _disposed, true)) { return; } 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 7e8acfda32ccb..92733f4bb3963 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 @@ -74,9 +74,9 @@ public static ValueTask ListenAsync(QuicListenerOptions options, C private readonly MsQuicContextSafeHandle _handle; /// - /// Set to non-zero once disposed. Prevents double and/or concurrent disposal. + /// Set to true once disposed. Prevents double and/or concurrent disposal. /// - private int _disposed; + private bool _disposed; /// /// Completed when SHUTDOWN_COMPLETE arrives. @@ -175,7 +175,7 @@ private unsafe QuicListener(QuicListenerOptions options) /// A task that will contain a fully connected which successfully finished the handshake and is ready to be used. public async ValueTask AcceptConnectionAsync(CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); GCHandle keepObject = GCHandle.Alloc(this); try @@ -405,7 +405,7 @@ private static unsafe int NativeCallback(QUIC_HANDLE* listener, void* context, Q /// A task that represents the asynchronous dispose operation. public async ValueTask DisposeAsync() { - if (Interlocked.Exchange(ref _disposed, 1) != 0) + if (Interlocked.Exchange(ref _disposed, true)) { return; } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.Stream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.Stream.cs index 8196c59a1c1bd..42824723a950c 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.Stream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.Stream.cs @@ -50,12 +50,12 @@ public override int ReadTimeout { get { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); return (int)_readTimeout.TotalMilliseconds; } set { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); if (value <= 0 && value != Timeout.Infinite) { throw new ArgumentOutOfRangeException(nameof(value), SR.net_quic_timeout_use_gt_zero); @@ -69,12 +69,12 @@ public override int WriteTimeout { get { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); return (int)_writeTimeout.TotalMilliseconds; } set { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); if (value <= 0 && value != Timeout.Infinite) { throw new ArgumentOutOfRangeException(nameof(value), SR.net_quic_timeout_use_gt_zero); @@ -86,7 +86,7 @@ public override int WriteTimeout // Read boilerplate. /// /// Gets a value indicating whether the supports reading. - public override bool CanRead => Volatile.Read(ref _disposed) == 0 && _canRead; + public override bool CanRead => !Volatile.Read(ref _disposed) && _canRead; /// public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) @@ -113,7 +113,7 @@ public override int ReadByte() /// public override int Read(Span buffer) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); byte[] rentedBuffer = ArrayPool.Shared.Rent(buffer.Length); CancellationTokenSource? cts = null; @@ -149,7 +149,7 @@ public override Task ReadAsync(byte[] buffer, int offset, int count, Cancel // Write boilerplate. /// /// Gets a value indicating whether the supports writing. - public override bool CanWrite => Volatile.Read(ref _disposed) == 0 && _canWrite; + public override bool CanWrite => !Volatile.Read(ref _disposed) && _canWrite; /// public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) @@ -175,7 +175,7 @@ public override void WriteByte(byte value) /// public override void Write(ReadOnlySpan buffer) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); CancellationTokenSource? cts = null; if (_writeTimeout > TimeSpan.Zero) 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 66568b937ed65..ce0128e779e9a 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 @@ -60,9 +60,9 @@ public sealed partial class QuicStream private readonly MsQuicContextSafeHandle _handle; /// - /// Set to non-zero once disposed. Prevents double and/or concurrent disposal. + /// Set to true once disposed. Prevents double and/or concurrent disposal. /// - private int _disposed; + private bool _disposed; private readonly ValueTaskSource _startedTcs = new ValueTaskSource(); private readonly ValueTaskSource _shutdownTcs = new ValueTaskSource(); @@ -272,7 +272,7 @@ internal ValueTask StartAsync(Action decrementStreamCapacity, Ca /// public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); if (!_canRead) { @@ -359,7 +359,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo /// Notifies the peer about gracefully closing the write side, i.e.: sends FIN flag with the data. public ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrites, CancellationToken cancellationToken = default) { - if (_disposed == 1) + if (_disposed) { return ValueTask.FromException(ExceptionDispatchInfo.SetCurrentStackTrace(new ObjectDisposedException(nameof(QuicStream)))); } @@ -451,7 +451,7 @@ public ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrites, Ca /// The error code with which to abort the stream, this value is application protocol (layer above QUIC) dependent. public void Abort(QuicAbortDirection abortDirection, long errorCode) { - if (_disposed == 1) + if (_disposed) { return; } @@ -510,7 +510,7 @@ public void Abort(QuicAbortDirection abortDirection, long errorCode) /// public void CompleteWrites() { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + ObjectDisposedException.ThrowIf(_disposed, this); // Nothing to complete, the writing side is already closed. if (_sendTcs.IsCompleted) @@ -712,7 +712,7 @@ private static unsafe int NativeCallback(QUIC_HANDLE* stream, void* context, QUI /// A task that represents the asynchronous dispose operation. public override async ValueTask DisposeAsync() { - if (Interlocked.Exchange(ref _disposed, 1) != 0) + if (Interlocked.Exchange(ref _disposed, true)) { return; } diff --git a/src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs b/src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs index fbc32738ef21d..ee2466d285a14 100644 --- a/src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs +++ b/src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs @@ -51,10 +51,10 @@ public class HttpWebRequest : WebRequest, ISerializable private static int _defaultMaxResponseHeadersLength = HttpHandlerDefaults.DefaultMaxResponseHeadersLength; private static int _defaultMaximumErrorResponseLength = -1; - private int _beginGetRequestStreamCalled; - private int _beginGetResponseCalled; - private int _endGetRequestStreamCalled; - private int _endGetResponseCalled; + private bool _beginGetRequestStreamCalled; + private bool _beginGetResponseCalled; + private bool _endGetRequestStreamCalled; + private bool _endGetResponseCalled; private int _maximumAllowedRedirections = HttpHandlerDefaults.DefaultMaxAutomaticRedirections; private int _maximumResponseHeadersLen = _defaultMaxResponseHeadersLength; @@ -74,7 +74,7 @@ public class HttpWebRequest : WebRequest, ISerializable private TaskCompletionSource? _responseOperation; private AsyncCallback? _requestStreamCallback; private AsyncCallback? _responseCallback; - private int _abortCalled; + private volatile bool _abortCalled; private CancellationTokenSource? _sendRequestCts; private X509CertificateCollection? _clientCertificates; private Booleans _booleans = Booleans.Default; @@ -995,7 +995,7 @@ public override IWebProxy? Proxy public override void Abort() { - if (Interlocked.Exchange(ref _abortCalled, 1) != 0) + if (Interlocked.Exchange(ref _abortCalled, true)) { return; } @@ -1125,7 +1125,7 @@ public override IAsyncResult BeginGetRequestStream(AsyncCallback? callback, obje { CheckAbort(); - if (Interlocked.Exchange(ref _beginGetRequestStreamCalled, 1) != 0) + if (Interlocked.Exchange(ref _beginGetRequestStreamCalled, true)) { throw new InvalidOperationException(SR.net_repcall); } @@ -1147,7 +1147,7 @@ public override Stream EndGetRequestStream(IAsyncResult asyncResult) throw new ArgumentException(SR.net_io_invalidasyncresult, nameof(asyncResult)); } - if (Interlocked.Exchange(ref _endGetRequestStreamCalled, 1) != 0) + if (Interlocked.Exchange(ref _endGetRequestStreamCalled, true)) { throw new InvalidOperationException(SR.Format(SR.net_io_invalidendcall, "EndGetRequestStream")); } @@ -1398,7 +1398,7 @@ public override IAsyncResult BeginGetResponse(AsyncCallback? callback, object? s { CheckAbort(); - if (Interlocked.Exchange(ref _beginGetResponseCalled, 1) != 0) + if (Interlocked.Exchange(ref _beginGetResponseCalled, true)) { throw new InvalidOperationException(SR.net_repcall); } @@ -1418,7 +1418,7 @@ public override WebResponse EndGetResponse(IAsyncResult asyncResult) throw new ArgumentException(SR.net_io_invalidasyncresult, nameof(asyncResult)); } - if (Interlocked.Exchange(ref _endGetResponseCalled, 1) != 0) + if (Interlocked.Exchange(ref _endGetResponseCalled, true)) { throw new InvalidOperationException(SR.Format(SR.net_io_invalidendcall, "EndGetResponse")); } @@ -1565,7 +1565,7 @@ private bool RequestSubmitted private void CheckAbort() { - if (Volatile.Read(ref _abortCalled) == 1) + if (_abortCalled) { throw new WebException(SR.net_reqaborted, WebExceptionStatus.RequestCanceled); } diff --git a/src/libraries/System.Net.Requests/src/System/Net/TimerThread.cs b/src/libraries/System.Net.Requests/src/System/Net/TimerThread.cs index 3aef8b469fded..4bbfd0f4bb8a5 100644 --- a/src/libraries/System.Net.Requests/src/System/Net/TimerThread.cs +++ b/src/libraries/System.Net.Requests/src/System/Net/TimerThread.cs @@ -445,14 +445,14 @@ private sealed class InfiniteTimer : Timer { internal InfiniteTimer() : base(Timeout.Infinite) { } - private int _cancelled; + private bool _canceled; internal override bool HasExpired => false; /// /// Cancels the timer. Returns true the first time, false after that. /// - internal override bool Cancel() => Interlocked.Exchange(ref _cancelled, 1) == 0; + internal override bool Cancel() => !Interlocked.Exchange(ref _canceled, true); } /// diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs b/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs index 48e3923037cc4..b0f25d431b2b9 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs @@ -42,9 +42,9 @@ public partial class NegotiateStream : AuthenticatedStream private int _readBufferCount; private ArrayBufferWriter? _writeBuffer; - private volatile int _writeInProgress; - private volatile int _readInProgress; - private volatile int _authInProgress; + private volatile bool _writeInProgress; + private volatile bool _readInProgress; + private volatile bool _authInProgress; private ExceptionDispatchInfo? _exception; private StreamFramer? _framer; @@ -340,7 +340,7 @@ private async ValueTask ReadAsync(Memory buffer, Cancella { Debug.Assert(_context is not null); - if (Interlocked.Exchange(ref _readInProgress, 1) == 1) + if (Interlocked.Exchange(ref _readInProgress, true)) { throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read")); } @@ -441,7 +441,7 @@ private async ValueTask ReadAsync(Memory buffer, Cancella } finally { - _readInProgress = 0; + _readInProgress = false; } static async ValueTask ReadAllAsync(Stream stream, Memory buffer, bool allowZeroRead, CancellationToken cancellationToken) @@ -506,7 +506,7 @@ private async Task WriteAsync(ReadOnlyMemory buffer, Cancellat Debug.Assert(_context is not null); Debug.Assert(_writeBuffer is not null); - if (Interlocked.Exchange(ref _writeInProgress, 1) == 1) + if (Interlocked.Exchange(ref _writeInProgress, true)) { throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "write")); } @@ -556,7 +556,7 @@ private async Task WriteAsync(ReadOnlyMemory buffer, Cancellat finally { _writeBuffer.Clear(); - _writeInProgress = 0; + _writeInProgress = false; } } @@ -724,7 +724,7 @@ private async Task AuthenticateAsync(CancellationToken cancellationT Debug.Assert(_context != null); ThrowIfFailed(authSuccessCheck: false); - if (Interlocked.Exchange(ref _authInProgress, 1) == 1) + if (Interlocked.Exchange(ref _authInProgress, true)) { throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "authenticate")); } @@ -742,7 +742,7 @@ private async Task AuthenticateAsync(CancellationToken cancellationT } finally { - _authInProgress = 0; + _authInProgress = false; } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs index 0275b8fa5cfea..faf807ff2603f 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs @@ -16,7 +16,7 @@ namespace System.Net.Security public partial class SslStream { private readonly SslAuthenticationOptions _sslAuthenticationOptions = new SslAuthenticationOptions(); - private int _nestedAuth; + private NestedState _nestedAuth; private bool _isRenego; private TlsFrameHelper.TlsFrameInfo _lastFrame; @@ -32,10 +32,14 @@ public partial class SslStream private bool _receivedEOF; // Used by Telemetry to ensure we log connection close exactly once - // 0 = no handshake - // 1 = handshake completed, connection opened - // 2 = SslStream disposed, connection closed - private int _connectionOpenedStatus; + private enum ConnectionStatus + { + NoHandshake = 0, + HandshakeCompleted = 1, // connection opened + Disposed = 2, // connection closed + } + + private ConnectionStatus _connectionOpenedStatus; private void SetException(Exception e) { @@ -56,10 +60,10 @@ private void CloseInternal() // Ensure a Read or Auth operation is not in progress, // block potential future read and auth operations since SslStream is disposing. - // This leaves the _nestedRead = 2 and _nestedAuth = 2, but that's ok, since + // This leaves the _nestedRead = StreamDisposed and _nestedAuth = StreamDisposed, but that's ok, since // subsequent operations check the _exception sentinel first - if (Interlocked.Exchange(ref _nestedRead, StreamDisposed) == StreamNotInUse && - Interlocked.Exchange(ref _nestedAuth, StreamDisposed) == StreamNotInUse) + if (Interlocked.Exchange(ref _nestedRead, NestedState.StreamDisposed) == NestedState.StreamNotInUse && + Interlocked.Exchange(ref _nestedAuth, NestedState.StreamDisposed) == NestedState.StreamNotInUse) { _buffer.ReturnBuffer(); } @@ -73,7 +77,7 @@ private void CloseInternal() if (NetSecurityTelemetry.Log.IsEnabled()) { // Set the status to disposed. If it was opened before, log ConnectionClosed - if (Interlocked.Exchange(ref _connectionOpenedStatus, 2) == 1) + if (Interlocked.Exchange(ref _connectionOpenedStatus, ConnectionStatus.Disposed) == ConnectionStatus.HandshakeCompleted) { NetSecurityTelemetry.Log.ConnectionClosed(GetSslProtocolInternal()); } @@ -143,9 +147,9 @@ private async Task ProcessAuthenticationWithTelemetryAsync(bool isAsync, Cancell if (startingTimestamp is not 0) { - // SslStream could already have been disposed at this point, in which case _connectionOpenedStatus == 2 + // SslStream could already have been disposed at this point, in which case _connectionOpenedStatus == ConnectionStatus.Disposed // Make sure that we increment the open connection counter only if it is guaranteed to be decremented in dispose/finalize - bool connectionOpen = Interlocked.CompareExchange(ref _connectionOpenedStatus, 1, 0) == 0; + bool connectionOpen = Interlocked.CompareExchange(ref _connectionOpenedStatus, ConnectionStatus.HandshakeCompleted, ConnectionStatus.NoHandshake) == ConnectionStatus.NoHandshake; SslProtocols protocol = GetSslProtocolInternal(); NetSecurityTelemetry.Log.HandshakeCompleted(protocol, startingTimestamp, connectionOpen); } @@ -187,22 +191,22 @@ private async Task ReplyOnReAuthenticationAsync(byte[]? buffer, Canc private async Task RenegotiateAsync(CancellationToken cancellationToken) where TIOAdapter : IReadWriteAdapter { - if (Interlocked.CompareExchange(ref _nestedAuth, StreamInUse, StreamNotInUse) != StreamNotInUse) + if (Interlocked.CompareExchange(ref _nestedAuth, NestedState.StreamInUse, NestedState.StreamNotInUse) != NestedState.StreamNotInUse) { - ObjectDisposedException.ThrowIf(_nestedAuth == StreamDisposed, this); + ObjectDisposedException.ThrowIf(_nestedAuth == NestedState.StreamDisposed, this); throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "authenticate")); } - if (Interlocked.CompareExchange(ref _nestedRead, StreamInUse, StreamNotInUse) != StreamNotInUse) + if (Interlocked.CompareExchange(ref _nestedRead, NestedState.StreamInUse, NestedState.StreamNotInUse) != NestedState.StreamNotInUse) { - ObjectDisposedException.ThrowIf(_nestedRead == StreamDisposed, this); + ObjectDisposedException.ThrowIf(_nestedRead == NestedState.StreamDisposed, this); throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read")); } // Write is different since we do not do anything special in Dispose - if (Interlocked.Exchange(ref _nestedWrite, StreamInUse) != StreamNotInUse) + if (Interlocked.Exchange(ref _nestedWrite, NestedState.StreamInUse) != NestedState.StreamNotInUse) { - _nestedRead = StreamNotInUse; + _nestedRead = NestedState.StreamNotInUse; throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "write")); } @@ -265,8 +269,8 @@ private async Task RenegotiateAsync(CancellationToken cancellationTo token.ReleasePayload(); - _nestedRead = StreamNotInUse; - _nestedWrite = StreamNotInUse; + _nestedRead = NestedState.StreamNotInUse; + _nestedWrite = NestedState.StreamNotInUse; _isRenego = false; // We will not release _nestedAuth at this point to prevent another renegotiation attempt. } @@ -284,7 +288,7 @@ private async Task ForceAuthenticationAsync(bool receiveFirst, byte[ if (reAuthenticationData == null) { // prevent nesting only when authentication functions are called explicitly. e.g. handle renegotiation transparently. - if (Interlocked.Exchange(ref _nestedAuth, StreamInUse) == StreamInUse) + if (Interlocked.Exchange(ref _nestedAuth, NestedState.StreamInUse) == NestedState.StreamInUse) { throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "authenticate")); } @@ -378,7 +382,7 @@ private async Task ForceAuthenticationAsync(bool receiveFirst, byte[ { if (reAuthenticationData == null) { - _nestedAuth = StreamNotInUse; + _nestedAuth = NestedState.StreamNotInUse; _isRenego = false; } @@ -547,7 +551,7 @@ private bool CompleteHandshake(ref ProtocolToken alertToken, out SslPolicyErrors { ProcessHandshakeSuccess(); - if (_nestedAuth != StreamInUse) + if (_nestedAuth != NestedState.StreamInUse) { if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, $"Ignoring unsolicited renegotiated certificate."); // ignore certificates received outside of handshake or requested renegotiation. @@ -797,7 +801,7 @@ private SecurityStatusPal DecryptData(int frameSize) // If that happen before EncryptData() runs, _handshakeWaiter will be set to null // and EncryptData() will work normally e.g. no waiting, just exclusion with DecryptData() - if (_sslAuthenticationOptions.AllowRenegotiation || SslProtocol == SslProtocols.Tls13 || _nestedAuth != 0) + if (_sslAuthenticationOptions.AllowRenegotiation || SslProtocol == SslProtocols.Tls13 || _nestedAuth != NestedState.StreamNotInUse) { // create TCS only if we plan to proceed. If not, we will throw later outside of the lock. // Tls1.3 does not have renegotiation. However on Windows this error code is used @@ -820,9 +824,9 @@ private async ValueTask ReadAsyncInternal(Memory buffer, // Check for disposal is not atomic so we will check again below. ThrowIfExceptionalOrNotAuthenticated(); - if (Interlocked.CompareExchange(ref _nestedRead, StreamInUse, StreamNotInUse) != StreamNotInUse) + if (Interlocked.CompareExchange(ref _nestedRead, NestedState.StreamInUse, NestedState.StreamNotInUse) != NestedState.StreamNotInUse) { - ObjectDisposedException.ThrowIf(_nestedRead == StreamDisposed, this); + ObjectDisposedException.ThrowIf(_nestedRead == NestedState.StreamDisposed, this); throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read")); } @@ -947,7 +951,7 @@ private async ValueTask ReadAsyncInternal(Memory buffer, finally { ReturnReadBufferIfEmpty(); - _nestedRead = StreamNotInUse; + _nestedRead = NestedState.StreamNotInUse; } } @@ -962,7 +966,7 @@ private async ValueTask WriteAsyncInternal(ReadOnlyMemory buff return; } - if (Interlocked.Exchange(ref _nestedWrite, StreamInUse) == StreamInUse) + if (Interlocked.Exchange(ref _nestedWrite, NestedState.StreamInUse) == NestedState.StreamInUse) { throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "write")); } @@ -985,7 +989,7 @@ private async ValueTask WriteAsyncInternal(ReadOnlyMemory buff } finally { - _nestedWrite = StreamNotInUse; + _nestedWrite = NestedState.StreamNotInUse; } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs index 5b991106dbc3b..47ee34664a4ee 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs @@ -167,13 +167,15 @@ public void ReturnBuffer() } } - // used to track ussage in _nested* variables bellow - private const int StreamNotInUse = 0; - private const int StreamInUse = 1; - private const int StreamDisposed = 2; + private enum NestedState + { + StreamNotInUse = 0, + StreamInUse = 1, + StreamDisposed = 2, + } - private int _nestedWrite; - private int _nestedRead; + private NestedState _nestedWrite; + private NestedState _nestedRead; private PoolingPointerMemoryManager? _readPointerMemoryManager; private PoolingPointerMemoryManager? _writePointerMemoryManager; @@ -735,7 +737,7 @@ private static unsafe void ReturnPointerMemoryManager(ref PoolingPointerMemoryMa public override int ReadByte() { ThrowIfExceptionalOrNotAuthenticated(); - if (Interlocked.Exchange(ref _nestedRead, StreamInUse) == StreamInUse) + if (Interlocked.Exchange(ref _nestedRead, NestedState.StreamInUse) == NestedState.StreamInUse) { throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read")); } @@ -756,7 +758,7 @@ public override int ReadByte() // Regardless of whether we were able to read a byte from the buffer, // reset the read tracking. If we weren't able to read a byte, the // subsequent call to Read will set the flag again. - _nestedRead = StreamNotInUse; + _nestedRead = NestedState.StreamNotInUse; } // Otherwise, fall back to reading a byte via Read, the same way Stream.ReadByte does. diff --git a/src/libraries/System.Net.Security/tests/UnitTests/Fakes/FakeSslStream.Implementation.cs b/src/libraries/System.Net.Security/tests/UnitTests/Fakes/FakeSslStream.Implementation.cs index 3ad6be920392a..31346cc9715e5 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/Fakes/FakeSslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/tests/UnitTests/Fakes/FakeSslStream.Implementation.cs @@ -54,12 +54,12 @@ private void ValidateCreateContext(SslClientAuthenticationOptions sslClientAuthe // Without setting (or using) these members you will get a build exception in the unit test project. // The code that normally uses these in the main solution is in the implementation of SslStream. - if (_nestedWrite == 0) + if (_nestedWrite == NestedState.StreamNotInUse) { } _exception = null; - _nestedWrite = 0; + _nestedWrite = NestedState.StreamNotInUse; _handshakeCompleted = false; } diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs index 9cb523300e849..4b94be9b1df57 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs @@ -23,8 +23,8 @@ public class NetworkStream : Stream // Used by the class to indicate that the stream is writable. private bool _writeable; - // Whether Dispose has been called. 0 == false, 1 == true - private int _disposed; + // Whether Dispose has been called. + private bool _disposed; // Creates a new instance of the System.Net.Sockets.NetworkStream class for the specified System.Net.Sockets.Socket. public NetworkStream(Socket socket) @@ -369,7 +369,7 @@ private static int ToTimeoutMilliseconds(TimeSpan timeout) protected override void Dispose(bool disposing) { - if (Interlocked.Exchange(ref _disposed, 1) != 0) + if (Interlocked.Exchange(ref _disposed, true)) { return; } @@ -685,7 +685,7 @@ internal void SetSocketTimeoutOption(SocketShutdown mode, int timeout, bool sile private void ThrowIfDisposed() { - ObjectDisposedException.ThrowIf(_disposed != 0, this); + ObjectDisposedException.ThrowIf(_disposed, this); } private static IOException WrapException(string resourceFormatString, Exception innerException) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs index f427aa88cc1f4..7f653dab759b8 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs @@ -28,7 +28,7 @@ public sealed partial class SafeSocketHandle : SafeHandleMinusOneIsInvalid private int _closeSocketThread; private int _closeSocketTick; #endif - private int _ownClose; + private bool _ownClose; /// /// Creates a . @@ -54,8 +54,7 @@ public SafeSocketHandle(IntPtr preexistingHandle, bool ownsHandle) internal bool HasShutdownSend => _hasShutdownSend; - private bool TryOwnClose() - => Interlocked.CompareExchange(ref _ownClose, 1, 0) == 0; + private bool TryOwnClose() => !Interlocked.Exchange(ref _ownClose, true); private volatile bool _released; private bool _hasShutdownSend; diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs index 73ff6521aa31f..3e4650f547c48 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs @@ -148,7 +148,7 @@ internal SocketError ReplaceHandle() return errorCode; } - if (Volatile.Read(ref _disposed) != 0) + if (Volatile.Read(ref _disposed)) { _handle.Dispose(); throw new ObjectDisposedException(GetType().FullName); diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index 02edb8853f909..e1f19c03d6d0a 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -67,7 +67,7 @@ public partial class Socket : IDisposable private bool _receivingPacketInformation; private int _closeTimeout = Socket.DefaultCloseTimeout; - private int _disposed; // 0 == false, anything else == true + private bool _disposed; public Socket(SocketType socketType, ProtocolType protocolType) : this(OSSupportsIPv6 ? AddressFamily.InterNetworkV6 : AddressFamily.InterNetwork, socketType, protocolType) @@ -3148,7 +3148,7 @@ private bool SendToAsync(SocketAsyncEventArgs e, CancellationToken cancellationT // Internal and private properties // - internal bool Disposed => _disposed != 0; + internal bool Disposed => _disposed; // // Internal and private methods @@ -3240,7 +3240,7 @@ protected virtual void Dispose(bool disposing) } // Make sure we're the first call to Dispose - if (Interlocked.CompareExchange(ref _disposed, 1, 0) == 1) + if (Interlocked.Exchange(ref _disposed, true)) { return; } diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs index dd50676ac55c4..d5613dc91f481 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs @@ -119,10 +119,10 @@ private enum State Canceled } - private int _state; // Actually AsyncOperation.State. + private volatile AsyncOperation.State _state; #if DEBUG - private int _callbackQueued; // When non-zero, the callback has been queued. + private bool _callbackQueued; // When true, the callback has been queued. #endif public readonly SocketAsyncContext AssociatedContext; @@ -141,11 +141,11 @@ public AsyncOperation(SocketAsyncContext context) public void Reset() { - _state = (int)State.Waiting; + _state = State.Waiting; Event = null; Next = this; #if DEBUG - _callbackQueued = 0; + _callbackQueued = false; #endif } @@ -154,34 +154,34 @@ public OperationResult TryComplete(SocketAsyncContext context) TraceWithContext(context, "Enter"); // Set state to Running, unless we've been canceled - int oldState = Interlocked.CompareExchange(ref _state, (int)State.Running, (int)State.Waiting); - if (oldState == (int)State.Canceled) + State oldState = Interlocked.CompareExchange(ref _state, State.Running, State.Waiting); + if (oldState == State.Canceled) { TraceWithContext(context, "Exit, Previously canceled"); return OperationResult.Cancelled; } - Debug.Assert(oldState == (int)State.Waiting, $"Unexpected operation state: {(State)oldState}"); + Debug.Assert(oldState == State.Waiting, $"Unexpected operation state: {(State)oldState}"); // Try to perform the IO if (DoTryComplete(context)) { - Debug.Assert((State)Volatile.Read(ref _state) is State.Running or State.RunningWithPendingCancellation, "Unexpected operation state"); + Debug.Assert(_state is State.Running or State.RunningWithPendingCancellation, "Unexpected operation state"); - Volatile.Write(ref _state, (int)State.Complete); + _state = State.Complete; TraceWithContext(context, "Exit, Completed"); return OperationResult.Completed; } // Set state back to Waiting, unless we were canceled, in which case we have to process cancellation now - int newState; + State newState; while (true) { - int state = Volatile.Read(ref _state); - Debug.Assert(state is (int)State.Running or (int)State.RunningWithPendingCancellation, $"Unexpected operation state: {(State)state}"); + State state = _state; + Debug.Assert(state is State.Running or State.RunningWithPendingCancellation, $"Unexpected operation state: {(State)state}"); - newState = (state == (int)State.Running ? (int)State.Waiting : (int)State.Canceled); + newState = (state == State.Running ? State.Waiting : State.Canceled); if (state == Interlocked.CompareExchange(ref _state, newState, state)) { break; @@ -190,7 +190,7 @@ public OperationResult TryComplete(SocketAsyncContext context) // Race to update the state. Loop and try again. } - if (newState == (int)State.Canceled) + if (newState == State.Canceled) { ProcessCancellation(); TraceWithContext(context, "Exit, Newly cancelled"); @@ -208,16 +208,16 @@ public bool TryCancel() // Note we could be cancelling because of socket close. Regardless, we don't need the registration anymore. CancellationRegistration.Dispose(); - int newState; + State newState; while (true) { - int state = Volatile.Read(ref _state); - if (state is (int)State.Complete or (int)State.Canceled or (int)State.RunningWithPendingCancellation) + State state = _state; + if (state is State.Complete or State.Canceled or State.RunningWithPendingCancellation) { return false; } - newState = (state == (int)State.Waiting ? (int)State.Canceled : (int)State.RunningWithPendingCancellation); + newState = (state == State.Waiting ? State.Canceled : State.RunningWithPendingCancellation); if (state == Interlocked.CompareExchange(ref _state, newState, state)) { break; @@ -226,7 +226,7 @@ public bool TryCancel() // Race to update the state. Loop and try again. } - if (newState == (int)State.RunningWithPendingCancellation) + if (newState == State.RunningWithPendingCancellation) { // TryComplete will either succeed, or it will see the pending cancellation and deal with it. return false; @@ -243,7 +243,7 @@ public void ProcessCancellation() { Trace("Enter"); - Debug.Assert(_state == (int)State.Canceled); + Debug.Assert(_state == State.Canceled); ErrorCode = SocketError.OperationAborted; @@ -255,7 +255,7 @@ public void ProcessCancellation() else { #if DEBUG - Debug.Assert(Interlocked.CompareExchange(ref _callbackQueued, 1, 0) == 0, $"Unexpected _callbackQueued: {_callbackQueued}"); + Debug.Assert(!Interlocked.Exchange(ref _callbackQueued, true), $"Unexpected _callbackQueued: {_callbackQueued}"); #endif // We've marked the operation as canceled, and so should invoke the callback, but // we can't pool the object, as ProcessQueue may still have a reference to it, due to diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs index f14c6753e93d7..4336420311847 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs @@ -103,7 +103,7 @@ private enum EventQueueProcessingStage Scheduled } - private int _eventQueueProcessingStage; + private EventQueueProcessingStage _eventQueueProcessingStage; // // Registers the Socket with a SocketAsyncEngine, and returns the associated engine. @@ -206,7 +206,7 @@ private void EventLoop() if (handler.HandleSocketEvents(numEvents) && Interlocked.Exchange( ref _eventQueueProcessingStage, - (int)EventQueueProcessingStage.Scheduled) == (int)EventQueueProcessingStage.NotScheduled) + EventQueueProcessingStage.Scheduled) == EventQueueProcessingStage.NotScheduled) { ThreadPool.UnsafeQueueUserWorkItem(this, preferLocal: false); } @@ -223,20 +223,20 @@ private void UpdateEventQueueProcessingStage(bool isEventQueueEmpty) if (!isEventQueueEmpty) { // There are more events to process, set stage to Scheduled and enqueue a work item. - _eventQueueProcessingStage = (int)EventQueueProcessingStage.Scheduled; + _eventQueueProcessingStage = EventQueueProcessingStage.Scheduled; } else { // The stage here would be Scheduled if an enqueuer has enqueued work and changed the stage, or Determining // otherwise. If the stage is Determining, there's no more work to do. If the stage is Scheduled, the enqueuer // would not have scheduled a work item to process the work, so schedule one now. - int stageBeforeUpdate = + EventQueueProcessingStage stageBeforeUpdate = Interlocked.CompareExchange( ref _eventQueueProcessingStage, - (int)EventQueueProcessingStage.NotScheduled, - (int)EventQueueProcessingStage.Determining); - Debug.Assert(stageBeforeUpdate != (int)EventQueueProcessingStage.NotScheduled); - if (stageBeforeUpdate == (int)EventQueueProcessingStage.Determining) + EventQueueProcessingStage.NotScheduled, + EventQueueProcessingStage.Determining); + Debug.Assert(stageBeforeUpdate != EventQueueProcessingStage.NotScheduled); + if (stageBeforeUpdate == EventQueueProcessingStage.Determining) { return; } @@ -251,14 +251,14 @@ void IThreadPoolWorkItem.Execute() SocketIOEvent ev; while (true) { - Debug.Assert(_eventQueueProcessingStage == (int)EventQueueProcessingStage.Scheduled); + Debug.Assert(_eventQueueProcessingStage == EventQueueProcessingStage.Scheduled); // The change needs to be visible to other threads that may request a worker thread before a work item is attempted // to be dequeued by the current thread. In particular, if an enqueuer queues a work item and does not request a // thread because it sees a Determining or Scheduled stage, and the current thread is the last thread processing // work items, the current thread must either see the work item queued by the enqueuer, or it must see a stage of // Scheduled, and try to dequeue again or request another thread. - _eventQueueProcessingStage = (int)EventQueueProcessingStage.Determining; + _eventQueueProcessingStage = EventQueueProcessingStage.Determining; Interlocked.MemoryBarrier(); if (eventQueue.TryDequeue(out ev)) @@ -269,13 +269,13 @@ void IThreadPoolWorkItem.Execute() // The stage here would be Scheduled if an enqueuer has enqueued work and changed the stage, or Determining // otherwise. If the stage is Determining, there's no more work to do. If the stage is Scheduled, the enqueuer // would not have scheduled a work item to process the work, so try to dequeue a work item again. - int stageBeforeUpdate = + EventQueueProcessingStage stageBeforeUpdate = Interlocked.CompareExchange( ref _eventQueueProcessingStage, - (int)EventQueueProcessingStage.NotScheduled, - (int)EventQueueProcessingStage.Determining); - Debug.Assert(stageBeforeUpdate != (int)EventQueueProcessingStage.NotScheduled); - if (stageBeforeUpdate == (int)EventQueueProcessingStage.Determining) + EventQueueProcessingStage.NotScheduled, + EventQueueProcessingStage.Determining); + Debug.Assert(stageBeforeUpdate != EventQueueProcessingStage.NotScheduled); + if (stageBeforeUpdate == EventQueueProcessingStage.Determining) { return; } diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs index 19aa0fa8ab4d7..c98d0cede114e 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs @@ -100,7 +100,7 @@ private void FreeInternals() private unsafe NativeOverlapped* AllocateNativeOverlapped() { Debug.Assert(OperatingSystem.IsWindows()); - Debug.Assert(_operating == InProgress, $"Expected {nameof(_operating)} == {nameof(InProgress)}, got {_operating}"); + Debug.Assert(_operating == OperationState.InProgress, $"Expected {nameof(_operating)} == {nameof(OperationState.InProgress)}, got {_operating}"); Debug.Assert(_currentSocket != null, "_currentSocket is null"); Debug.Assert(_currentSocket.SafeHandle != null, "_currentSocket.SafeHandle is null"); Debug.Assert(_preAllocatedOverlapped != null, "_preAllocatedOverlapped is null"); @@ -113,7 +113,7 @@ private unsafe void FreeNativeOverlapped(ref NativeOverlapped* overlapped) { Debug.Assert(OperatingSystem.IsWindows()); Debug.Assert(overlapped != null, "overlapped is null"); - Debug.Assert(_operating == InProgress, $"Expected _operating == InProgress, got {_operating}"); + Debug.Assert(_operating == OperationState.InProgress, $"Expected _operating == OperationState.InProgress, got {_operating}"); Debug.Assert(_currentSocket != null, "_currentSocket is null"); Debug.Assert(_currentSocket.SafeHandle != null, "_currentSocket.SafeHandle is null"); Debug.Assert(_currentSocket.SafeHandle.IOCPBoundHandle != null, "_currentSocket.SafeHandle.IOCPBoundHandle is null"); diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs index 158d56182aae2..b635d27da4479 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs @@ -77,12 +77,14 @@ public partial class SocketAsyncEventArgs : EventArgs, IDisposable private bool _userSocket; // if false when performing Connect, _currentSocket should be disposed private bool _disposeCalled; - // Controls thread safety via Interlocked. - private const int Configuring = -1; - private const int Free = 0; - private const int InProgress = 1; - private const int Disposed = 2; - private int _operating; + private enum OperationState + { + Configuring = -1, + Free = 0, + InProgress = 1, + Disposed = 2, + } + private OperationState _operating; private CancellationTokenSource? _multipleConnectCancellation; @@ -480,7 +482,7 @@ internal void Complete() _context = null; // Mark as not in-use. - _operating = Free; + _operating = OperationState.Free; // Check for deferred Dispose(). // The deferred Dispose is not guaranteed if Dispose is called while an operation is in progress. @@ -498,7 +500,7 @@ public void Dispose() _disposeCalled = true; // Check if this object is in-use for an async socket operation. - if (Interlocked.CompareExchange(ref _operating, Disposed, Free) != Free) + if (Interlocked.CompareExchange(ref _operating, OperationState.Disposed, OperationState.Free) != OperationState.Free) { // Either already disposed or will be disposed when current operation completes. return; @@ -525,17 +527,17 @@ public void Dispose() // NOTE: Use a try/finally to make sure Complete is called when you're done private void StartConfiguring() { - int status = Interlocked.CompareExchange(ref _operating, Configuring, Free); - if (status != Free) + OperationState status = Interlocked.CompareExchange(ref _operating, OperationState.Configuring, OperationState.Free); + if (status != OperationState.Free) { ThrowForNonFreeStatus(status); } } - private void ThrowForNonFreeStatus(int status) + private void ThrowForNonFreeStatus(OperationState status) { - Debug.Assert(status == InProgress || status == Configuring || status == Disposed, $"Unexpected status: {status}"); - ObjectDisposedException.ThrowIf(status == Disposed, this); + Debug.Assert(status == OperationState.InProgress || status == OperationState.Configuring || status == OperationState.Disposed, $"Unexpected status: {status}"); + ObjectDisposedException.ThrowIf(status == OperationState.Disposed, this); throw new InvalidOperationException(SR.net_socketopinprogress); } @@ -544,8 +546,8 @@ private void ThrowForNonFreeStatus(int status) internal void StartOperationCommon(Socket? socket, SocketAsyncOperation operation) { // Change status to "in-use". - int status = Interlocked.CompareExchange(ref _operating, InProgress, Free); - if (status != Free) + OperationState status = Interlocked.CompareExchange(ref _operating, OperationState.InProgress, OperationState.Free); + if (status != OperationState.Free) { ThrowForNonFreeStatus(status); } @@ -606,7 +608,7 @@ internal void StartOperationConnect(bool saeaMultiConnectCancelable, bool userSo internal void CancelConnectAsync() { - if (_operating == InProgress && _completedOperation == SocketAsyncOperation.Connect) + if (_operating == OperationState.InProgress && _completedOperation == SocketAsyncOperation.Connect) { CancellationTokenSource? multipleConnectCancellation = _multipleConnectCancellation; if (multipleConnectCancellation != null) @@ -865,7 +867,7 @@ caughtException is OperationCanceledException || private sealed class MultiConnectSocketAsyncEventArgs : SocketAsyncEventArgs, IValueTaskSource { private ManualResetValueTaskSourceCore _mrvtsc; - private int _isCompleted; + private bool _isCompleted; public MultiConnectSocketAsyncEventArgs() : base(unsafeSuppressExecutionContextFlow: false) { } @@ -878,7 +880,7 @@ public MultiConnectSocketAsyncEventArgs() : base(unsafeSuppressExecutionContextF protected override void OnCompleted(SocketAsyncEventArgs e) => _mrvtsc.SetResult(true); - public bool ReachedCoordinationPointFirst() => Interlocked.Exchange(ref _isCompleted, 1) == 0; + public bool ReachedCoordinationPointFirst() => !Interlocked.Exchange(ref _isCompleted, true); } internal void FinishOperationSyncSuccess(int bytesTransferred, SocketFlags flags) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/TCPClient.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/TCPClient.cs index 4d60b197f7a93..54998d3e02e93 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/TCPClient.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/TCPClient.cs @@ -17,10 +17,10 @@ public class TcpClient : IDisposable private AddressFamily _family; private Socket _clientSocket = null!; // initialized by helper called from ctor private NetworkStream? _dataStream; - private volatile int _disposed; + private volatile bool _disposed; private bool _active; - private bool Disposed => _disposed != 0; + private bool Disposed => _disposed; // Initializes a new instance of the System.Net.Sockets.TcpClient class. public TcpClient() : this(AddressFamily.Unknown) @@ -252,7 +252,7 @@ public NetworkStream GetStream() // Disposes the Tcp connection. protected virtual void Dispose(bool disposing) { - if (Interlocked.CompareExchange(ref _disposed, 1, 0) == 0) + if (!Interlocked.Exchange(ref _disposed, true)) { if (disposing) { diff --git a/src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.NonBrowser.cs b/src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.NonBrowser.cs index 8716439eac8e2..b865732117622 100644 --- a/src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.NonBrowser.cs +++ b/src/libraries/System.Net.WebProxy/src/System/Net/WebProxy.NonBrowser.cs @@ -11,7 +11,7 @@ public partial class WebProxy : IWebProxy, ISerializable { private static volatile string? s_domainName; private static volatile IPAddress[]? s_localAddresses; - private static int s_networkChangeRegistered; + private static bool s_networkChangeRegistered; private static bool IsLocal(Uri host) { @@ -47,13 +47,13 @@ private static bool IsLocal(Uri host) /// Ensures we've registered with NetworkChange to clear out statically-cached state upon a network change notification. private static void EnsureNetworkChangeRegistration() { - if (s_networkChangeRegistered == 0) + if (!s_networkChangeRegistered) { Register(); static void Register() { - if (Interlocked.Exchange(ref s_networkChangeRegistered, 1) != 0) + if (Interlocked.Exchange(ref s_networkChangeRegistered, true)) { return; } diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs index 5780555470c1e..37b5565b74675 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs @@ -12,12 +12,12 @@ namespace System.Net.WebSockets public sealed partial class ClientWebSocket : WebSocket { /// This is really an InternalState value, but Interlocked doesn't support operations on values of enum types. - private int _state; + private InternalState _state; private WebSocketHandle? _innerWebSocket; public ClientWebSocket() { - _state = (int)InternalState.Created; + _state = InternalState.Created; Options = WebSocketHandle.CreateDefaultOptions(); } @@ -39,14 +39,14 @@ public override WebSocketState State return _innerWebSocket.State; } - switch ((InternalState)_state) + switch (_state) { case InternalState.Created: return WebSocketState.None; case InternalState.Connecting: return WebSocketState.Connecting; default: // We only get here if disposed before connecting - Debug.Assert((InternalState)_state == InternalState.Disposed); + Debug.Assert(_state == InternalState.Disposed); return WebSocketState.Closed; } } @@ -105,7 +105,7 @@ public Task ConnectAsync(Uri uri, HttpMessageInvoker? invoker, CancellationToken } // Check that we have not started already. - switch ((InternalState)Interlocked.CompareExchange(ref _state, (int)InternalState.Connecting, (int)InternalState.Created)) + switch (Interlocked.CompareExchange(ref _state, InternalState.Connecting, InternalState.Created)) { case InternalState.Disposed: throw new ObjectDisposedException(GetType().FullName); @@ -135,9 +135,9 @@ private async Task ConnectAsyncCore(Uri uri, HttpMessageInvoker? invoker, Cancel throw; } - if ((InternalState)Interlocked.CompareExchange(ref _state, (int)InternalState.Connected, (int)InternalState.Connecting) != InternalState.Connecting) + if (Interlocked.CompareExchange(ref _state, InternalState.Connected, InternalState.Connecting) != InternalState.Connecting) { - Debug.Assert(_state == (int)InternalState.Disposed); + Debug.Assert(_state == InternalState.Disposed); throw new ObjectDisposedException(GetType().FullName); } } @@ -167,9 +167,9 @@ private WebSocket ConnectedWebSocket { get { - ObjectDisposedException.ThrowIf((InternalState)_state == InternalState.Disposed, this); + ObjectDisposedException.ThrowIf(_state == InternalState.Disposed, this); - if ((InternalState)_state != InternalState.Connected) + if (_state != InternalState.Connected) { throw new InvalidOperationException(SR.net_WebSockets_NotConnected); } @@ -183,7 +183,7 @@ private WebSocket ConnectedWebSocket public override void Abort() { - if ((InternalState)_state != InternalState.Disposed) + if (_state != InternalState.Disposed) { _innerWebSocket?.Abort(); Dispose(); @@ -192,7 +192,7 @@ public override void Abort() public override void Dispose() { - if ((InternalState)Interlocked.Exchange(ref _state, (int)InternalState.Disposed) != InternalState.Disposed) + if (Interlocked.Exchange(ref _state, InternalState.Disposed) != InternalState.Disposed) { _innerWebSocket?.Dispose(); } diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index ad6476701c339..7fbe6aa096382 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -4328,10 +4328,13 @@ Emitting debug info is not supported for this member. + + The specified type must be a reference type, a primitive type, or an enum type. + The field is invalid for initializing array or span. Only array or span of primitive or enum types can be initialized from static data. - \ No newline at end of file + diff --git a/src/libraries/System.Private.CoreLib/src/System/Buffers/SharedArrayPool.cs b/src/libraries/System.Private.CoreLib/src/System/Buffers/SharedArrayPool.cs index 3fa044d074d60..8c6ffe9b61c46 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Buffers/SharedArrayPool.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Buffers/SharedArrayPool.cs @@ -35,7 +35,7 @@ internal sealed partial class SharedArrayPool : ArrayPool /// private readonly SharedArrayPoolPartitions?[] _buckets = new SharedArrayPoolPartitions[NumBuckets]; /// Whether the callback to trim arrays in response to memory pressure has been created. - private int _trimCallbackCreated; + private bool _trimCallbackCreated; /// Allocate a new and try to store it into the array. private unsafe SharedArrayPoolPartitions CreatePerCorePartitions(int bucketIndex) @@ -283,7 +283,7 @@ private SharedArrayPoolThreadLocalArray[] InitializeTlsBucketsAndTrimming() t_tlsBuckets = tlsBuckets; _allTlsBuckets.Add(tlsBuckets, null); - if (Interlocked.Exchange(ref _trimCallbackCreated, 1) == 0) + if (!Interlocked.Exchange(ref _trimCallbackCreated, true)) { Gen2GcCallback.Register(s => ((SharedArrayPool)s).Trim(), this); } diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/ActivityTracker.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/ActivityTracker.cs index 6bba8725c848a..8700c8bd52c53 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/ActivityTracker.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/ActivityTracker.cs @@ -165,7 +165,7 @@ public void OnStop(string providerName, string activityName, int task, ref Guid ActivityInfo? orphan = currentActivity; while (orphan != activityToStop && orphan != null) { - if (orphan.m_stopped != 0) // Skip dead activities. + if (orphan.m_stopped) // Skip dead activities. { orphan = orphan.m_creator; continue; @@ -177,14 +177,13 @@ public void OnStop(string providerName, string activityName, int task, ref Guid } else { - orphan.m_stopped = 1; - Debug.Assert(orphan.m_stopped != 0); + orphan.m_stopped = true; } orphan = orphan.m_creator; } // try to Stop the activity atomically. Other threads may be trying to do this as well. - if (Interlocked.CompareExchange(ref activityToStop.m_stopped, 1, 0) == 0) + if (!Interlocked.Exchange(ref activityToStop.m_stopped, true)) { // I succeeded stopping this activity. Now we update our m_current pointer @@ -239,7 +238,7 @@ public void Enable() ActivityInfo? activity = startLocation; while (activity != null) { - if (name == activity.m_name && activity.m_stopped == 0) + if (name == activity.m_name && !activity.m_stopped) return activity; activity = activity.m_creator; } @@ -309,7 +308,7 @@ public static string Path(ActivityInfo? activityInfo) public override string ToString() { - return m_name + "(" + Path(this) + (m_stopped != 0 ? ",DEAD)" : ")"); + return m_name + "(" + Path(this) + (m_stopped ? ",DEAD)" : ")"); } public static string LiveActivities(ActivityInfo? list) @@ -520,7 +519,7 @@ private static unsafe void WriteNibble(ref byte* ptr, byte* endPtr, uint value) internal readonly int m_level; // current depth of the Path() of the activity (used to keep recursion under control) internal readonly EventActivityOptions m_eventOptions; // Options passed to start. internal long m_lastChildID; // used to create a unique ID for my children activities - internal int m_stopped; // This work item has stopped + internal bool m_stopped; // This work item has stopped internal readonly ActivityInfo? m_creator; // My parent (creator). Forms the Path() for the activity. internal readonly Guid m_activityIdToRestore; // The Guid to restore after a stop. #endregion @@ -578,7 +577,7 @@ private void ActivityChanging(AsyncLocalValueChangedArgs args) while (cur != null) { // We found a live activity (typically the first time), set it to that. - if (cur.m_stopped == 0) + if (!cur.m_stopped) { EventSource.SetCurrentThreadActivityId(cur.ActivityId); return; diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs index f1358083a220f..26c2eb7078598 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs @@ -25,7 +25,7 @@ namespace System.Threading public class CancellationTokenSource : IDisposable { /// A that's already canceled. - internal static readonly CancellationTokenSource s_canceledSource = new CancellationTokenSource() { _state = NotifyingCompleteState }; + internal static readonly CancellationTokenSource s_canceledSource = new CancellationTokenSource() { _state = States.NotifyingCompleteState }; /// A that's never canceled. This isn't enforced programmatically, only by usage. Do not cancel! internal static readonly CancellationTokenSource s_neverCanceledSource = new CancellationTokenSource(); @@ -35,7 +35,7 @@ private static void TimerCallback(object? state) => // separated out into a name ((CancellationTokenSource)state!).NotifyCancellation(throwOnFirstException: false); // skip ThrowIfDisposed() check in Cancel() /// The current state of the CancellationTokenSource. - private volatile int _state; + private volatile States _state; /// Whether this has been disposed. private bool _disposed; /// ITimer used by CancelAfter and Timer-related ctors. Used instead of Timer to avoid extra allocations and because the rooted behavior is desired. @@ -46,10 +46,13 @@ private static void TimerCallback(object? state) => // separated out into a name /// Lazily-initialized, also serving as the lock to protect its contained state. private Registrations? _registrations; - // legal values for _state - private const int NotCanceledState = 0; // default value of _state - private const int NotifyingState = 1; - private const int NotifyingCompleteState = 2; + /// Legal values for . + private enum States + { + NotCanceledState = 0, // default value of _state + NotifyingState = 1, + NotifyingCompleteState = 2, + } /// Gets whether cancellation has been requested for this . /// Whether cancellation has been requested for this . @@ -66,10 +69,10 @@ private static void TimerCallback(object? state) => // separated out into a name /// canceled concurrently. /// /// - public bool IsCancellationRequested => _state != NotCanceledState; + public bool IsCancellationRequested => _state != States.NotCanceledState; /// A simple helper to determine whether cancellation has finished. - internal bool IsCancellationCompleted => _state == NotifyingCompleteState; + internal bool IsCancellationCompleted => _state == States.NotifyingCompleteState; /// Gets the associated with this . /// The associated with this . @@ -201,7 +204,7 @@ private void InitializeWithTimer(TimeSpan millisecondsDelay, TimeProvider timePr { if (millisecondsDelay == TimeSpan.Zero) { - _state = NotifyingCompleteState; + _state = States.NotifyingCompleteState; } else { @@ -480,7 +483,7 @@ public bool TryReset() // We can only reset if cancellation has not yet been requested: we never want to allow a CancellationToken // to transition from canceled to non-canceled. - if (_state == NotCanceledState) + if (_state == States.NotCanceledState) { // If there is no timer, then we're free to reset. If there is a timer, then we need to first try // to reset it to be infinite so that it won't fire, and then recognize that it could have already @@ -556,7 +559,7 @@ protected virtual void Dispose(bool disposing) if (_kernelEvent != null) { ManualResetEvent? mre = Interlocked.Exchange(ref _kernelEvent!, null); - if (mre != null && _state != NotifyingState) + if (mre != null && _state != States.NotifyingState) { mre.Dispose(); } @@ -698,13 +701,13 @@ private void NotifyCancellation(bool throwOnFirstException) } } - /// Transitions from to . + /// Transitions from to . /// true if it successfully transitioned; otherwise, false. /// If it successfully transitions, it will also have disposed of and set . private bool TransitionToCancellationRequested() { if (!IsCancellationRequested && - Interlocked.CompareExchange(ref _state, NotifyingState, NotCanceledState) == NotCanceledState) + Interlocked.CompareExchange(ref _state, States.NotifyingState, States.NotCanceledState) == States.NotCanceledState) { // Dispose of the timer, if any. Dispose may be running concurrently here, but ITimer.Dispose is thread-safe. ITimer? timer = _timer; @@ -737,7 +740,7 @@ private void ExecuteCallbackHandlers(bool throwOnFirstException) Registrations? registrations = Interlocked.Exchange(ref _registrations, null); if (registrations is null) { - Interlocked.Exchange(ref _state, NotifyingCompleteState); + Interlocked.Exchange(ref _state, States.NotifyingCompleteState); return; } @@ -825,7 +828,7 @@ private void ExecuteCallbackHandlers(bool throwOnFirstException) } finally { - _state = NotifyingCompleteState; + _state = States.NotifyingCompleteState; Interlocked.Exchange(ref registrations.ExecutingCallbackId, 0); // for safety, prevent reorderings crossing this point and seeing inconsistent state. } @@ -1021,7 +1024,7 @@ internal sealed class Registrations /// public volatile int ThreadIDExecutingCallbacks = -1; /// Spin lock that protects state in the instance. - private int _lock; + private volatile bool _locked; /// Initializes the instance. /// The associated source. @@ -1030,7 +1033,7 @@ internal sealed class Registrations [MethodImpl(MethodImplOptions.AggressiveInlining)] // used in only two places, one of which is a hot path private void Recycle(CallbackNode node) { - Debug.Assert(_lock == 1); + Debug.Assert(_locked); // Clear out the unused node and put it on the singly-linked free list. // The only field we don't clear out is the associated Registrations, as that's fixed @@ -1163,16 +1166,16 @@ public async ValueTask WaitForCallbackToCompleteAsync(long id) /// Enters the lock for this instance. The current thread must not be holding the lock, but that is not validated. public void EnterLock() { - ref int value = ref _lock; - if (Interlocked.Exchange(ref value, 1) != 0) + ref bool value = ref _locked; + if (Interlocked.Exchange(ref value, true)) { Contention(ref value); [MethodImpl(MethodImplOptions.NoInlining)] - static void Contention(ref int value) + static void Contention(ref bool value) { SpinWait sw = default; - do { sw.SpinOnce(); } while (Interlocked.Exchange(ref value, 1) == 1); + do { sw.SpinOnce(); } while (Interlocked.Exchange(ref value, true)); } } } @@ -1180,8 +1183,8 @@ static void Contention(ref int value) /// Exits the lock for this instance. The current thread must be holding the lock, but that is not validated. public void ExitLock() { - Debug.Assert(_lock == 1); - Volatile.Write(ref _lock, 0); + Debug.Assert(_locked); + _locked = false; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs index 6e33ce716fd6a..f22109a0e11f2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs @@ -2,7 +2,11 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +#pragma warning disable CS8500 // takes the address of, gets the size of, or declares a pointer to a managed type ('T') namespace System.Threading { @@ -222,6 +226,66 @@ public static UIntPtr Exchange(ref UIntPtr location1, UIntPtr value) return (UIntPtr)Exchange(ref Unsafe.As(ref location1), (int)value); #endif } + + /// Sets a variable of the specified type to a specified value and returns the original value, as an atomic operation. + /// The variable to set to the specified value. + /// The value to which the parameter is set. + /// The original value of . + /// The address of location1 is a null pointer. + /// An unsupported is specified. + /// + /// The type to be used for and . + /// This type must be a reference type, an enum type (i.e. typeof(T).IsEnum is true), or a primitive type (i.e. typeof(T).IsPrimitive is true). + /// + [Intrinsic] + [return: NotNullIfNotNull(nameof(location1))] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static unsafe T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T value) + { + // Handle all reference types with CompareExchange(ref object, ...). + if (!typeof(T).IsValueType) + { + object? result = Exchange(ref Unsafe.As(ref location1), value); + return Unsafe.As(ref result); + } + + // Handle everything else with a CompareExchange overload for the unsigned integral type of the corresponding size. + // Only primitive types and enum types (which are backed by primitive types) are supported. + if (!typeof(T).IsPrimitive && !typeof(T).IsEnum) + { + throw new NotSupportedException(SR.NotSupported_ReferenceEnumOrPrimitiveTypeRequired); + } + + if (sizeof(T) == 1) + { + return Unsafe.BitCast( + Exchange( + ref Unsafe.As(ref location1), + Unsafe.BitCast(value))); + } + + if (sizeof(T) == 2) + { + return Unsafe.BitCast( + Exchange( + ref Unsafe.As(ref location1), + Unsafe.BitCast(value))); + } + + if (sizeof(T) == 4) + { + return Unsafe.BitCast( + Exchange( + ref Unsafe.As(ref location1), + Unsafe.BitCast(value))); + } + + Debug.Assert(sizeof(T) == 8); + return Unsafe.BitCast( + Exchange( + ref Unsafe.As(ref location1), + Unsafe.BitCast(value))); + } #endregion #region CompareExchange @@ -413,6 +477,71 @@ public static UIntPtr CompareExchange(ref UIntPtr location1, UIntPtr value, UInt return (UIntPtr)CompareExchange(ref Unsafe.As(ref location1), (int)value, (int)comparand); #endif } + + /// Compares two instances of the specified type for equality and, if they are equal, replaces the first one. + /// The destination, whose value is compared with and possibly replaced. + /// The value that replaces the destination value if the comparison results in equality. + /// The object that is compared to the value at . + /// The original value in . + /// The address of is a null pointer. + /// An unsupported is specified. + /// + /// The type to be used for , , and . + /// This type must be a reference type, an enum type (i.e. typeof(T).IsEnum is true), or a primitive type (i.e. typeof(T).IsPrimitive is true). + /// + [Intrinsic] + [MethodImpl(MethodImplOptions.AggressiveInlining)] + [return: NotNullIfNotNull(nameof(location1))] + public static unsafe T CompareExchange(ref T location1, T value, T comparand) + { + // Handle all reference types with CompareExchange(ref object, ...). + if (!typeof(T).IsValueType) + { + object? result = CompareExchange(ref Unsafe.As(ref location1), value, comparand); + return Unsafe.As(ref result); + } + + // Handle everything else with a CompareExchange overload for the unsigned integral type of the corresponding size. + // Only primitive types and enum types (which are backed by primitive types) are supported. + if (!typeof(T).IsPrimitive && !typeof(T).IsEnum) + { + throw new NotSupportedException(SR.NotSupported_ReferenceEnumOrPrimitiveTypeRequired); + } + + if (sizeof(T) == 1) + { + return Unsafe.BitCast( + CompareExchange( + ref Unsafe.As(ref location1), + Unsafe.BitCast(value), + Unsafe.BitCast(comparand))); + } + + if (sizeof(T) == 2) + { + return Unsafe.BitCast( + CompareExchange( + ref Unsafe.As(ref location1), + Unsafe.BitCast(value), + Unsafe.BitCast(comparand))); + } + + if (sizeof(T) == 4) + { + return Unsafe.BitCast( + CompareExchange( + ref Unsafe.As(ref location1), + Unsafe.BitCast(value), + Unsafe.BitCast(comparand))); + } + + Debug.Assert(sizeof(T) == 8); + return Unsafe.BitCast( + CompareExchange( + ref Unsafe.As(ref location1), + Unsafe.BitCast(value), + Unsafe.BitCast(comparand))); + } #endregion #region Add diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ReaderWriterLockSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ReaderWriterLockSlim.cs index ed62bbbcc9a68..222cf6ecd50d0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ReaderWriterLockSlim.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ReaderWriterLockSlim.cs @@ -1416,7 +1416,7 @@ public int RecursiveWriteCount private struct SpinLock { - private int _isLocked; + private bool _isLocked; /// /// Used to deprioritize threads attempting to enter the lock when they would not make progress after doing so. @@ -1535,7 +1535,7 @@ private bool IsEnterDeprioritized(EnterSpinLockReason reason) [MethodImpl(MethodImplOptions.AggressiveInlining)] private bool TryEnter() { - return Interlocked.CompareExchange(ref _isLocked, 1, 0) == 0; + return !Interlocked.Exchange(ref _isLocked, true); } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -1577,7 +1577,7 @@ private void EnterSpin(EnterSpinLockReason reason) if (!IsEnterDeprioritized(reason)) { - if (_isLocked == 0 && TryEnter()) + if (!_isLocked && TryEnter()) { if (deprioritizationStateChange != 0) { @@ -1606,12 +1606,12 @@ private void EnterSpin(EnterSpinLockReason reason) public void Exit() { - Debug.Assert(_isLocked != 0, "Exiting spin lock that is not held"); - Volatile.Write(ref _isLocked, 0); + Debug.Assert(_isLocked, "Exiting spin lock that is not held"); + Volatile.Write(ref _isLocked, false); } #if DEBUG - public bool IsHeld => _isLocked != 0; + public bool IsHeld => _isLocked; #endif } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs index 3c1d958314ebb..6d4f512b6fa6f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs @@ -399,7 +399,7 @@ public int Count private bool _loggingEnabled; private bool _dispatchNormalPriorityWorkFirst; - private int _mayHaveHighPriorityWorkItems; + private bool _mayHaveHighPriorityWorkItems; // SOS's ThreadPool command depends on the following names internal readonly ConcurrentQueue workItems = new ConcurrentQueue(); @@ -443,7 +443,7 @@ private struct CacheLineSeparated { private readonly Internal.PaddingFor32 pad1; - public int queueProcessingStage; + public QueueProcessingStage queueProcessingStage; private readonly Internal.PaddingFor32 pad2; } @@ -611,7 +611,7 @@ internal void EnsureThreadRequested() // Otherwise let the current requested thread handle parallelization. if (Interlocked.Exchange( ref _separated.queueProcessingStage, - (int)QueueProcessingStage.Scheduled) == (int)QueueProcessingStage.NotScheduled) + QueueProcessingStage.Scheduled) == QueueProcessingStage.NotScheduled) { ThreadPool.RequestWorkerThread(); } @@ -696,7 +696,7 @@ public void EnqueueAtHighPriority(object workItem) highPriorityWorkItems.Enqueue(workItem); // If the change below is seen by another thread, ensure that the enqueued work item will also be visible - Volatile.Write(ref _mayHaveHighPriorityWorkItems, 1); + Volatile.Write(ref _mayHaveHighPriorityWorkItems, true); EnsureThreadRequested(); } @@ -736,8 +736,8 @@ internal static bool LocalFindAndPop(object callback) tl.isProcessingHighPriorityWorkItems = false; } else if ( - _mayHaveHighPriorityWorkItems != 0 && - Interlocked.CompareExchange(ref _mayHaveHighPriorityWorkItems, 0, 1) != 0 && + _mayHaveHighPriorityWorkItems && + Interlocked.CompareExchange(ref _mayHaveHighPriorityWorkItems, false, true) && TryStartProcessingHighPriorityWorkItemsAndDequeue(tl, out workItem)) { return workItem; @@ -816,7 +816,7 @@ private bool TryStartProcessingHighPriorityWorkItemsAndDequeue( } tl.isProcessingHighPriorityWorkItems = true; - _mayHaveHighPriorityWorkItems = 1; + _mayHaveHighPriorityWorkItems = true; return true; } @@ -906,8 +906,8 @@ internal static bool Dispatch() // thread because it sees a Determining or Scheduled stage, and the current thread is the last thread processing // work items, the current thread must either see the work item queued by the enqueuer, or it must see a stage of // Scheduled, and try to dequeue again or request another thread. - Debug.Assert(workQueue._separated.queueProcessingStage == (int)QueueProcessingStage.Scheduled); - workQueue._separated.queueProcessingStage = (int)QueueProcessingStage.Determining; + Debug.Assert(workQueue._separated.queueProcessingStage == QueueProcessingStage.Scheduled); + workQueue._separated.queueProcessingStage = QueueProcessingStage.Determining; Interlocked.MemoryBarrier(); object? workItem = null; @@ -935,8 +935,8 @@ internal static bool Dispatch() workQueue.UnassignWorkItemQueue(tl); } - Debug.Assert(workQueue._separated.queueProcessingStage != (int)QueueProcessingStage.NotScheduled); - workQueue._separated.queueProcessingStage = (int)QueueProcessingStage.Scheduled; + Debug.Assert(workQueue._separated.queueProcessingStage != QueueProcessingStage.NotScheduled); + workQueue._separated.queueProcessingStage = QueueProcessingStage.Scheduled; ThreadPool.RequestWorkerThread(); return true; } @@ -944,13 +944,13 @@ internal static bool Dispatch() // The stage here would be Scheduled if an enqueuer has enqueued work and changed the stage, or Determining // otherwise. If the stage is Determining, there's no more work to do. If the stage is Scheduled, the enqueuer // would not have scheduled a work item to process the work, so try to dequeue a work item again. - int stageBeforeUpdate = + QueueProcessingStage stageBeforeUpdate = Interlocked.CompareExchange( ref workQueue._separated.queueProcessingStage, - (int)QueueProcessingStage.NotScheduled, - (int)QueueProcessingStage.Determining); - Debug.Assert(stageBeforeUpdate != (int)QueueProcessingStage.NotScheduled); - if (stageBeforeUpdate == (int)QueueProcessingStage.Determining) + QueueProcessingStage.NotScheduled, + QueueProcessingStage.Determining); + Debug.Assert(stageBeforeUpdate != QueueProcessingStage.NotScheduled); + if (stageBeforeUpdate == QueueProcessingStage.Determining) { if (s_assignableWorkItemQueueCount > 0) { @@ -964,7 +964,7 @@ internal static bool Dispatch() // by the enqueuer. Set the stage back to Determining and try to dequeue a work item again. // // See the first similarly used memory barrier in the method for why it's necessary. - workQueue._separated.queueProcessingStage = (int)QueueProcessingStage.Determining; + workQueue._separated.queueProcessingStage = QueueProcessingStage.Determining; Interlocked.MemoryBarrier(); } } @@ -976,7 +976,7 @@ internal static bool Dispatch() // be able to detect if an enqueue races with the dequeue below. // // See the first similarly used memory barrier in the method for why it's necessary. - workQueue._separated.queueProcessingStage = (int)QueueProcessingStage.Determining; + workQueue._separated.queueProcessingStage = QueueProcessingStage.Determining; Interlocked.MemoryBarrier(); object? secondWorkItem = DequeueWithPriorityAlternation(workQueue, tl, out bool missedSteal); @@ -993,8 +993,8 @@ internal static bool Dispatch() // responsibility of the new thread and other enqueuers to request more threads as necessary. The // parallelization may be necessary here for correctness (aside from perf) if the work item blocks for some // reason that may have a dependency on other queued work items. - Debug.Assert(workQueue._separated.queueProcessingStage != (int)QueueProcessingStage.NotScheduled); - workQueue._separated.queueProcessingStage = (int)QueueProcessingStage.Scheduled; + Debug.Assert(workQueue._separated.queueProcessingStage != QueueProcessingStage.NotScheduled); + workQueue._separated.queueProcessingStage = QueueProcessingStage.Scheduled; ThreadPool.RequestWorkerThread(); } else @@ -1002,13 +1002,13 @@ internal static bool Dispatch() // The stage here would be Scheduled if an enqueuer has enqueued work and changed the stage, or Determining // otherwise. If the stage is Determining, there's no more work to do. If the stage is Scheduled, the enqueuer // would not have requested a thread, so request one now. - int stageBeforeUpdate = + QueueProcessingStage stageBeforeUpdate = Interlocked.CompareExchange( ref workQueue._separated.queueProcessingStage, - (int)QueueProcessingStage.NotScheduled, - (int)QueueProcessingStage.Determining); - Debug.Assert(stageBeforeUpdate != (int)QueueProcessingStage.NotScheduled); - if (stageBeforeUpdate == (int)QueueProcessingStage.Scheduled) + QueueProcessingStage.NotScheduled, + QueueProcessingStage.Determining); + Debug.Assert(stageBeforeUpdate != QueueProcessingStage.NotScheduled); + if (stageBeforeUpdate == QueueProcessingStage.Scheduled) { // A work item was enqueued after the stage was set to Determining earlier, and a thread was not // requested by the enqueuer, so request a thread now. An alternate is to retry dequeuing, as requesting @@ -1269,7 +1269,7 @@ private enum QueueProcessingStage Scheduled } - private int _queueProcessingStage; + private QueueProcessingStage _queueProcessingStage; private readonly ConcurrentQueue _workItems = new ConcurrentQueue(); public int Count => _workItems.Count; @@ -1287,7 +1287,7 @@ public void CompleteBatchEnqueue() // Otherwise there must be a work item already queued or another thread already handling parallelization. if (Interlocked.Exchange( ref _queueProcessingStage, - (int)QueueProcessingStage.Scheduled) == (int)QueueProcessingStage.NotScheduled) + QueueProcessingStage.Scheduled) == QueueProcessingStage.NotScheduled) { ThreadPool.UnsafeQueueHighPriorityWorkItemInternal(this); } @@ -1298,20 +1298,20 @@ private void UpdateQueueProcessingStage(bool isQueueEmpty) if (!isQueueEmpty) { // There are more items to process, set stage to Scheduled and enqueue a TP work item. - _queueProcessingStage = (int)QueueProcessingStage.Scheduled; + _queueProcessingStage = QueueProcessingStage.Scheduled; } else { // The stage here would be Scheduled if an enqueuer has enqueued work and changed the stage, or Determining // otherwise. If the stage is Determining, there's no more work to do. If the stage is Scheduled, the enqueuer // would not have scheduled a work item to process the work, so schedule one one. - int stageBeforeUpdate = + QueueProcessingStage stageBeforeUpdate = Interlocked.CompareExchange( ref _queueProcessingStage, - (int)QueueProcessingStage.NotScheduled, - (int)QueueProcessingStage.Determining); - Debug.Assert(stageBeforeUpdate != (int)QueueProcessingStage.NotScheduled); - if (stageBeforeUpdate == (int)QueueProcessingStage.Determining) + QueueProcessingStage.NotScheduled, + QueueProcessingStage.Determining); + Debug.Assert(stageBeforeUpdate != QueueProcessingStage.NotScheduled); + if (stageBeforeUpdate == QueueProcessingStage.Determining) { return; } @@ -1325,14 +1325,14 @@ void IThreadPoolWorkItem.Execute() T workItem; while (true) { - Debug.Assert(_queueProcessingStage == (int)QueueProcessingStage.Scheduled); + Debug.Assert(_queueProcessingStage == QueueProcessingStage.Scheduled); // The change needs to be visible to other threads that may request a worker thread before a work item is attempted // to be dequeued by the current thread. In particular, if an enqueuer queues a work item and does not request a // thread because it sees a Determining or Scheduled stage, and the current thread is the last thread processing // work items, the current thread must either see the work item queued by the enqueuer, or it must see a stage of // Scheduled, and try to dequeue again or request another thread. - _queueProcessingStage = (int)QueueProcessingStage.Determining; + _queueProcessingStage = QueueProcessingStage.Determining; Interlocked.MemoryBarrier(); if (_workItems.TryDequeue(out workItem)) @@ -1343,13 +1343,13 @@ void IThreadPoolWorkItem.Execute() // The stage here would be Scheduled if an enqueuer has enqueued work and changed the stage, or Determining // otherwise. If the stage is Determining, there's no more work to do. If the stage is Scheduled, the enqueuer // would not have scheduled a work item to process the work, so try to dequeue a work item again. - int stageBeforeUpdate = + QueueProcessingStage stageBeforeUpdate = Interlocked.CompareExchange( ref _queueProcessingStage, - (int)QueueProcessingStage.NotScheduled, - (int)QueueProcessingStage.Determining); - Debug.Assert(stageBeforeUpdate != (int)QueueProcessingStage.NotScheduled); - if (stageBeforeUpdate == (int)QueueProcessingStage.Determining) + QueueProcessingStage.NotScheduled, + QueueProcessingStage.Determining); + Debug.Assert(stageBeforeUpdate != QueueProcessingStage.NotScheduled); + if (stageBeforeUpdate == QueueProcessingStage.Determining) { return; } @@ -1401,13 +1401,12 @@ void IThreadPoolWorkItem.Execute() internal abstract class QueueUserWorkItemCallbackBase : IThreadPoolWorkItem { #if DEBUG - private int executed; + private bool _executed; ~QueueUserWorkItemCallbackBase() { Interlocked.MemoryBarrier(); // ensure that an old cached value is not read below - Debug.Assert( - executed != 0, "A QueueUserWorkItemCallback was never called!"); + Debug.Assert(_executed, "A QueueUserWorkItemCallback was never called!"); } #endif @@ -1415,9 +1414,7 @@ public virtual void Execute() { #if DEBUG GC.SuppressFinalize(this); - Debug.Assert( - 0 == Interlocked.Exchange(ref executed, 1), - "A QueueUserWorkItemCallback was called twice!"); + Debug.Assert(!Interlocked.Exchange(ref _executed, true), "A QueueUserWorkItemCallback was called twice!"); #endif } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.ThreadWaitInfo.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.ThreadWaitInfo.Unix.cs index 8bcafa15a6271..a26587e5412a5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.ThreadWaitInfo.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.ThreadWaitInfo.Unix.cs @@ -75,7 +75,7 @@ public sealed class ThreadWaitInfo /// - Sleep(0) intentionally does not acquire any lock, so it uses an interlocked compare-exchange for the read and /// reset, see /// - private int _isPendingInterrupt; + private bool _isPendingInterrupt; //////////////////////////////////////////////////////////////// @@ -508,7 +508,7 @@ private void RecordPendingInterrupt() s_lock.VerifyIsLocked(); _waitMonitor.VerifyIsLocked(); - _isPendingInterrupt = 1; + _isPendingInterrupt = true; } public bool CheckAndResetPendingInterrupt @@ -519,11 +519,11 @@ public bool CheckAndResetPendingInterrupt Debug.Assert(s_lock.IsLocked || _waitMonitor.IsLocked); #endif - if (_isPendingInterrupt == 0) + if (!_isPendingInterrupt) { return false; } - _isPendingInterrupt = 0; + _isPendingInterrupt = false; return true; } } @@ -535,7 +535,7 @@ private bool CheckAndResetPendingInterrupt_NotLocked s_lock.VerifyIsNotLocked(); _waitMonitor.VerifyIsNotLocked(); - return Interlocked.CompareExchange(ref _isPendingInterrupt, 0, 1) != 0; + return Interlocked.CompareExchange(ref _isPendingInterrupt, false, true); } } diff --git a/src/libraries/System.Private.Uri/src/System/Uri.cs b/src/libraries/System.Private.Uri/src/System/Uri.cs index 8e34e1d9605ed..2c05a2df4afb6 100644 --- a/src/libraries/System.Private.Uri/src/System/Uri.cs +++ b/src/libraries/System.Private.Uri/src/System/Uri.cs @@ -2396,16 +2396,13 @@ private unsafe void CreateUriInfo(Flags cF) Done: cF |= Flags.MinimalUriInfoSet; - Debug.Assert(sizeof(Flags) == sizeof(ulong)); - Interlocked.CompareExchange(ref _info, info, null!); Flags current = _flags; while ((current & Flags.MinimalUriInfoSet) == 0) { - Flags newValue = (current & ~Flags.IndexMask) | cF; - ulong oldValue = Interlocked.CompareExchange(ref Unsafe.As(ref _flags), (ulong)newValue, (ulong)current); - if (oldValue == (ulong)current) + Flags oldValue = Interlocked.CompareExchange(ref _flags, (current & ~Flags.IndexMask) | cF, current); + if (oldValue == current) { return; } diff --git a/src/libraries/System.Runtime.InteropServices/src/System/Runtime/InteropServices/Marshalling/ComObject.cs b/src/libraries/System.Runtime.InteropServices/src/System/Runtime/InteropServices/Marshalling/ComObject.cs index 216b38d7de253..a88f3edaecff2 100644 --- a/src/libraries/System.Runtime.InteropServices/src/System/Runtime/InteropServices/Marshalling/ComObject.cs +++ b/src/libraries/System.Runtime.InteropServices/src/System/Runtime/InteropServices/Marshalling/ComObject.cs @@ -23,8 +23,7 @@ public sealed unsafe class ComObject : IDynamicInterfaceCastable, IUnmanagedVirt private readonly object? _runtimeCallableWrapper; - // This is an int so we can use the Interlocked APIs to update it. - private volatile int _released; + private volatile bool _released; /// /// Initialize ComObject instance. @@ -81,7 +80,7 @@ internal ComObject(IIUnknownInterfaceDetailsStrategy interfaceDetailsStrategy, I /// public void FinalRelease() { - if (UniqueInstance && Interlocked.CompareExchange(ref _released, 1, 0) == 0) + if (UniqueInstance && !Interlocked.Exchange(ref _released, true)) { GC.SuppressFinalize(this); CacheStrategy.Clear(IUnknownStrategy); @@ -115,7 +114,7 @@ bool IDynamicInterfaceCastable.IsInterfaceImplemented(RuntimeTypeHandle interfac private bool LookUpVTableInfo(RuntimeTypeHandle handle, out IIUnknownCacheStrategy.TableInfo result, out int qiHResult) { - ObjectDisposedException.ThrowIf(_released != 0, this); + ObjectDisposedException.ThrowIf(_released, this); qiHResult = 0; if (!CacheStrategy.TryGetTableInfo(handle, out result)) diff --git a/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.ForEachAsync.cs b/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.ForEachAsync.cs index 3cff4876f085f..643ea8590f0cb 100644 --- a/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.ForEachAsync.cs +++ b/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.ForEachAsync.cs @@ -92,32 +92,10 @@ private static Task ForAsync(T fromInclusive, T toExclusive, int dop, TaskSch return Task.CompletedTask; } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - static bool Interlockable() => - typeof(T) == typeof(sbyte) || - typeof(T) == typeof(byte) || - typeof(T) == typeof(short) || - typeof(T) == typeof(ushort) || - typeof(T) == typeof(char) || - typeof(T) == typeof(int) || - typeof(T) == typeof(uint) || - typeof(T) == typeof(long) || - typeof(T) == typeof(ulong) || - typeof(T) == typeof(nint) || - typeof(T) == typeof(nuint); - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - static unsafe bool CompareExchange(ref T location, T value, T comparand) => - sizeof(T) == sizeof(byte) ? Interlocked.CompareExchange(ref Unsafe.As(ref location), Unsafe.As(ref value), Unsafe.As(ref comparand)) == Unsafe.As(ref comparand) : - sizeof(T) == sizeof(ushort) ? Interlocked.CompareExchange(ref Unsafe.As(ref location), Unsafe.As(ref value), Unsafe.As(ref comparand)) == Unsafe.As(ref comparand) : - sizeof(T) == sizeof(uint) ? Interlocked.CompareExchange(ref Unsafe.As(ref location), Unsafe.As(ref value), Unsafe.As(ref comparand)) == Unsafe.As(ref comparand) : - sizeof(T) == sizeof(ulong) ? Interlocked.CompareExchange(ref Unsafe.As(ref location), Unsafe.As(ref value), Unsafe.As(ref comparand)) == Unsafe.As(ref comparand) : - throw new UnreachableException(); - // The worker body. Each worker will execute this same body. Func taskBody = static async o => { - var state = (ForEachState)o; + var state = (ForAsyncState)o; bool launchedNext = false; #pragma warning disable CA2007 // Explicitly don't use ConfigureAwait, as we want to perform all work on the specified scheduler that's now current @@ -126,10 +104,10 @@ static unsafe bool CompareExchange(ref T location, T value, T comparand) => // Continue to loop while there are more elements to be processed. while (!state.Cancellation.IsCancellationRequested) { - // Get the next element from the enumerator. For some types, we can get the next element with just + // Get the next element from the enumerator. For primitive types, we can get the next element with just // interlocked operations, avoiding the need to take a lock. For other types, we need to take a lock. T element; - if (Interlockable()) + if (typeof(T).IsPrimitive) { TryAgain: element = state.NextAvailable; @@ -138,7 +116,7 @@ static unsafe bool CompareExchange(ref T location, T value, T comparand) => break; } - if (!CompareExchange(ref state.NextAvailable, element + T.One, element)) + if (Interlocked.CompareExchange(ref state.NextAvailable, element + T.One, element) != element) { goto TryAgain; } @@ -200,7 +178,7 @@ static unsafe bool CompareExchange(ref T location, T value, T comparand) => { // Construct a state object that encapsulates all state to be passed and shared between // the workers, and queues the first worker. - var state = new ForEachState(fromInclusive, toExclusive, taskBody, !Interlockable(), dop, scheduler, cancellationToken, body); + var state = new ForAsyncState(fromInclusive, toExclusive, taskBody, dop, scheduler, cancellationToken, body); state.QueueWorkerIfDopAvailable(); return state.Task; } @@ -747,18 +725,18 @@ public ValueTask DisposeAsync() } } - /// Stores the state associated with an IAsyncEnumerable ForEachAsync operation, shared between all its workers. + /// Stores the state associated with an IAsyncEnumerable ForAsyncState operation, shared between all its workers. /// Specifies the type of data being enumerated. - private sealed class ForEachState : ForEachAsyncState, IDisposable + private sealed class ForAsyncState : ForEachAsyncState, IDisposable { public T NextAvailable; public readonly T ToExclusive; - public ForEachState( + public ForAsyncState( T fromExclusive, T toExclusive, Func taskBody, - bool needsLock, int dop, TaskScheduler scheduler, CancellationToken cancellationToken, + int dop, TaskScheduler scheduler, CancellationToken cancellationToken, Func body) : - base(taskBody, needsLock, dop, scheduler, cancellationToken, body) + base(taskBody, !typeof(T).IsPrimitive, dop, scheduler, cancellationToken, body) { NextAvailable = fromExclusive; ToExclusive = toExclusive; diff --git a/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/ParallelLoopState.cs b/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/ParallelLoopState.cs index e00c096abb97e..65f0ba68298ef 100644 --- a/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/ParallelLoopState.cs +++ b/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/ParallelLoopState.cs @@ -211,9 +211,7 @@ internal static void Break(TInt iteration, ParallelLoopStateFlags pf if (iteration < oldLBI) { SpinWait wait = default; - while (typeof(TInt) == typeof(int) ? - Interlocked.CompareExchange(ref Unsafe.As(ref pflags._lowestBreakIteration), Unsafe.As(ref iteration), Unsafe.As(ref oldLBI)) != Unsafe.As(ref oldLBI) : - Interlocked.CompareExchange(ref Unsafe.As(ref pflags._lowestBreakIteration), Unsafe.As(ref iteration), Unsafe.As(ref oldLBI)) != Unsafe.As(ref oldLBI)) + while (Interlocked.CompareExchange(ref pflags._lowestBreakIteration, iteration, oldLBI) != oldLBI) { wait.SpinOnce(); oldLBI = pflags.LowestBreakIteration; diff --git a/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/ParallelRangeManager.cs b/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/ParallelRangeManager.cs index c53a3346c7d9b..a1b4d65c6224a 100644 --- a/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/ParallelRangeManager.cs +++ b/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/ParallelRangeManager.cs @@ -33,8 +33,8 @@ internal struct IndexRange // that range, minimizing the chances it'll be near the objects from other threads. internal volatile StrongBox? _nSharedCurrentIndexOffset; - // to be set to 1 by the worker that finishes this range. It's OK to do a non-interlocked write here. - internal int _bRangeFinished; + // to be set to true by the worker that finishes this range. It's OK to do a non-interlocked write here. + internal bool _bRangeFinished; } @@ -101,7 +101,7 @@ private bool FindNewWork(out long nFromInclusiveLocal, out long nToExclusiveLoca // local snap to save array access bounds checks in places where we only read fields IndexRange currentRange = _indexRanges[_nCurrentIndexRange]; - if (currentRange._bRangeFinished == 0) + if (!currentRange._bRangeFinished) { StrongBox? sharedCurrentIndexOffset = _indexRanges[_nCurrentIndexRange]._nSharedCurrentIndexOffset; if (sharedCurrentIndexOffset == null) @@ -157,7 +157,7 @@ private bool FindNewWork(out long nFromInclusiveLocal, out long nToExclusiveLoca else { // this index range is completed, mark it so that others can skip it quickly - Interlocked.Exchange(ref _indexRanges[_nCurrentIndexRange]._bRangeFinished, 1); + Interlocked.Exchange(ref _indexRanges[_nCurrentIndexRange]._bRangeFinished, true); } } @@ -262,7 +262,7 @@ internal RangeManager(long nFromInclusive, long nToExclusive, long nStep, int nN // the fromInclusive of the new index range is always on nCurrentIndex _indexRanges[i]._nFromInclusive = nCurrentIndex; _indexRanges[i]._nSharedCurrentIndexOffset = null; - _indexRanges[i]._bRangeFinished = 0; + _indexRanges[i]._bRangeFinished = false; // now increment it to find the toExclusive value for our range nCurrentIndex = unchecked(nCurrentIndex + nRangeSize); diff --git a/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs b/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs index cc3b5b082fc89..1f2ad97489a6b 100644 --- a/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs +++ b/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs @@ -452,7 +452,7 @@ public void MetricsTest() bool waitForWorkStart = false; var workStarted = new AutoResetEvent(false); var localWorkScheduled = new AutoResetEvent(false); - int completeWork = 0; + bool completeWork = false; int queuedWorkCount = 0; var allWorkCompleted = new ManualResetEvent(false); Exception backgroundEx = null; @@ -467,7 +467,7 @@ public void MetricsTest() // Blocking can affect thread pool thread injection heuristics, so don't block, pretend like a // long-running CPU-bound work item ThreadTestHelpers.WaitForConditionWithoutRelinquishingTimeSlice( - () => Interlocked.CompareExchange(ref completeWork, 0, 0) != 0); + () => Interlocked.CompareExchange(ref completeWork, false, false)); } catch (Exception ex) { @@ -557,7 +557,7 @@ public void MetricsTest() finally { // Complete the work - Interlocked.Exchange(ref completeWork, 1); + Interlocked.Exchange(ref completeWork, true); } // Wait for work items to exit, for counting diff --git a/src/libraries/System.Threading/ref/System.Threading.cs b/src/libraries/System.Threading/ref/System.Threading.cs index 74743e946890c..dca39d5c4719e 100644 --- a/src/libraries/System.Threading/ref/System.Threading.cs +++ b/src/libraries/System.Threading/ref/System.Threading.cs @@ -269,7 +269,7 @@ public static partial class Interlocked [System.CLSCompliantAttribute(false)] public static ulong CompareExchange(ref ulong location1, ulong value, ulong comparand) { throw null; } [return: System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("location1")] - public static T CompareExchange(ref T location1, T value, T comparand) where T : class? { throw null; } + public static T CompareExchange(ref T location1, T value, T comparand) { throw null; } public static int Decrement(ref int location) { throw null; } public static long Decrement(ref long location) { throw null; } [System.CLSCompliantAttribute(false)] @@ -296,7 +296,7 @@ public static partial class Interlocked [System.CLSCompliantAttribute(false)] public static ulong Exchange(ref ulong location1, ulong value) { throw null; } [return: System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("location1")] - public static T Exchange([System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("value")] ref T location1, T value) where T : class? { throw null; } + public static T Exchange([System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("value")] ref T location1, T value) { throw null; } public static int Increment(ref int location) { throw null; } public static long Increment(ref long location) { throw null; } [System.CLSCompliantAttribute(false)] diff --git a/src/libraries/System.Threading/tests/InterlockedTests.cs b/src/libraries/System.Threading/tests/InterlockedTests.cs index ae9cd493f4dff..b1e4ac92f8f7a 100644 --- a/src/libraries/System.Threading/tests/InterlockedTests.cs +++ b/src/libraries/System.Threading/tests/InterlockedTests.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Runtime.CompilerServices; +using System.Text; using System.Threading.Tasks; using Xunit; @@ -145,9 +146,15 @@ public void InterlockedExchange_Int8() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref sbyte value = ref memory.Span[0]; + value = 42; Assert.Equal(42, Interlocked.Exchange(ref value, 123)); Assert.Equal(123, value); + + value = 42; + Assert.Equal(42, Interlocked.Exchange(ref value, 123)); + Assert.Equal(123, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 123)); } @@ -156,9 +163,15 @@ public void InterlockedExchange_UInt8() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref byte value = ref memory.Span[0]; + value = 42; Assert.Equal(42u, Interlocked.Exchange(ref value, 123)); Assert.Equal(123u, value); + + value = 42; + Assert.Equal(42u, Interlocked.Exchange(ref value, 123)); + Assert.Equal(123u, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 123)); } @@ -167,9 +180,15 @@ public void InterlockedExchange_Int16() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref short value = ref memory.Span[0]; + value = 42; Assert.Equal(42, Interlocked.Exchange(ref value, 12345)); Assert.Equal(12345, value); + + value = 42; + Assert.Equal(42, Interlocked.Exchange(ref value, 12345)); + Assert.Equal(12345, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345)); } @@ -178,9 +197,15 @@ public void InterlockedExchange_UInt16() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref ushort value = ref memory.Span[0]; + value = 42; Assert.Equal(42u, Interlocked.Exchange(ref value, 12345)); Assert.Equal(12345u, value); + + value = 42; + Assert.Equal(42u, Interlocked.Exchange(ref value, 12345)); + Assert.Equal(12345u, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345)); } @@ -189,9 +214,15 @@ public void InterlockedExchange_Int32() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref int value = ref memory.Span[0]; + value = 42; Assert.Equal(42, Interlocked.Exchange(ref value, 12345)); Assert.Equal(12345, value); + + value = 42; + Assert.Equal(42, Interlocked.Exchange(ref value, 12345)); + Assert.Equal(12345, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345)); } @@ -200,9 +231,15 @@ public void InterlockedExchange_UInt32() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref uint value = ref memory.Span[0]; + value = 42; Assert.Equal(42u, Interlocked.Exchange(ref value, 12345u)); Assert.Equal(12345u, value); + + value = 42; + Assert.Equal(42u, Interlocked.Exchange(ref value, 12345u)); + Assert.Equal(12345u, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345)); } @@ -211,9 +248,15 @@ public void InterlockedExchange_Int64() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref long value = ref memory.Span[0]; + value = 42; Assert.Equal(42, Interlocked.Exchange(ref value, 12345)); Assert.Equal(12345, value); + + value = 42; + Assert.Equal(42, Interlocked.Exchange(ref value, 12345)); + Assert.Equal(12345, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345)); } @@ -222,14 +265,22 @@ public void InterlockedExchange_IntPtr() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref nint value = ref memory.Span[0]; + value = 42; Assert.Equal(42, (nint)Interlocked.Exchange(ref value, (nint)12345)); Assert.Equal(12345, value); + value = 42; + Assert.Equal(42, (nint)Interlocked.Exchange(ref value, (nint)12345)); + Assert.Equal(12345, value); + if (Environment.Is64BitProcess) { Assert.Equal(12345, (nint)Interlocked.Exchange(ref value, unchecked((nint)1 + int.MaxValue))); Assert.Equal(unchecked((nint)1 + int.MaxValue), value); + + Assert.Equal(unchecked((nint)1 + int.MaxValue), (nint)Interlocked.Exchange(ref value, unchecked((nint)2 + int.MaxValue))); + Assert.Equal(unchecked((nint)2 + int.MaxValue), value); } Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345)); } @@ -239,9 +290,15 @@ public void InterlockedExchange_UInt64() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref ulong value = ref memory.Span[0]; + value = 42; Assert.Equal(42u, Interlocked.Exchange(ref value, 12345u)); Assert.Equal(12345u, value); + + value = 42; + Assert.Equal(42u, Interlocked.Exchange(ref value, 12345u)); + Assert.Equal(12345u, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345)); } @@ -250,14 +307,22 @@ public void InterlockedExchange_UIntPtr() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref nuint value = ref memory.Span[0]; + value = 42; Assert.Equal(42u, (nuint)Interlocked.Exchange(ref value, (nuint)12345u)); Assert.Equal(12345u, value); + value = 42; + Assert.Equal(42u, (nuint)Interlocked.Exchange(ref value, (nuint)12345u)); + Assert.Equal(12345u, value); + if (Environment.Is64BitProcess) { Assert.Equal(12345u, (nuint)Interlocked.Exchange(ref value, unchecked((nuint)1 + uint.MaxValue))); Assert.Equal(unchecked((nuint)1 + uint.MaxValue), value); + + Assert.Equal(unchecked((nuint)1 + uint.MaxValue), (nuint)Interlocked.Exchange(ref value, unchecked((nuint)2 + uint.MaxValue))); + Assert.Equal(unchecked((nuint)2 + uint.MaxValue), value); } Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345)); } @@ -267,9 +332,15 @@ public void InterlockedExchange_Float() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref float value = ref memory.Span[0]; + value = 42.1f; Assert.Equal(42.1f, Interlocked.Exchange(ref value, 12345.1f)); Assert.Equal(12345.1f, value); + + value = 42.1f; + Assert.Equal(42.1f, Interlocked.Exchange(ref value, 12345.1f)); + Assert.Equal(12345.1f, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345.1f)); } @@ -278,9 +349,15 @@ public void InterlockedExchange_Double() { using BoundedMemory memory = BoundedMemory.Allocate(1); ref double value = ref memory.Span[0]; + value = 42.1; Assert.Equal(42.1, Interlocked.Exchange(ref value, 12345.1)); Assert.Equal(12345.1, value); + + value = 42.1; + Assert.Equal(42.1, Interlocked.Exchange(ref value, 12345.1)); + Assert.Equal(12345.1, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), 12345.1)); } @@ -289,10 +366,15 @@ public void InterlockedExchange_Object() { var oldValue = new object(); var newValue = new object(); - object value = oldValue; + object value = oldValue; Assert.Same(oldValue, Interlocked.Exchange(ref value, newValue)); Assert.Same(newValue, value); + + value = oldValue; + Assert.Same(oldValue, Interlocked.Exchange(ref value, newValue)); + Assert.Same(newValue, value); + Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), null)); Assert.Throws(() => Interlocked.Exchange(ref Unsafe.NullRef(), newValue)); } @@ -311,6 +393,20 @@ public void InterlockedExchange_BoxedObject() Assert.Equal(12345, (int)value); } + [Fact] + public void InterlockedExchange_Unsupported() + { + DateTime value1 = default; + TimeSpan value2 = default; + Rune value3 = default; + ValueTask value4 = default; + + Assert.Throws(() => Interlocked.Exchange(ref value1, default)); + Assert.Throws(() => Interlocked.Exchange(ref value2, default)); + Assert.Throws(() => Interlocked.Exchange(ref value3, default)); + Assert.Throws(() => Interlocked.Exchange(ref value4, default)); + } + [Fact] public void InterlockedCompareExchange_Int8() { @@ -324,7 +420,16 @@ public void InterlockedCompareExchange_Int8() Assert.Equal(42, Interlocked.CompareExchange(ref value, 123, 42)); Assert.Equal(123, value); - Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 123, 41)); + value = 42; + + Assert.Equal(42, Interlocked.CompareExchange(ref value, 123, 41)); + Assert.Equal(42, value); + + Assert.Equal(42, Interlocked.CompareExchange(ref value, 123, 42)); + Assert.Equal(123, value); + + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 123, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 123, 41)); } [Fact] @@ -340,7 +445,32 @@ public void InterlockedCompareExchange_UInt8() Assert.Equal(42u, Interlocked.CompareExchange(ref value, 123, 42)); Assert.Equal(123u, value); - Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 123, 41)); + value = 42; + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, 123, 41)); + Assert.Equal(42u, value); + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, 123, 42)); + Assert.Equal(123u, value); + + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 123, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 123, 41)); + } + + [Fact] + public void InterlockedCompareExchange_Bool() + { + using BoundedMemory memory = BoundedMemory.Allocate(1); + ref bool value = ref memory.Span[0]; + value = false; + + Assert.False(Interlocked.CompareExchange(ref value, true, false)); + Assert.True(value); + + Assert.True(Interlocked.CompareExchange(ref value, false, false)); + Assert.True(value); + + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), false, false)); } [Fact] @@ -356,7 +486,16 @@ public void InterlockedCompareExchange_Int16() Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 42)); Assert.Equal(12345, value); + value = 42; + + Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 41)); + Assert.Equal(42, value); + + Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 42)); + Assert.Equal(12345, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); } [Fact] @@ -372,7 +511,32 @@ public void InterlockedCompareExchange_UInt16() Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345, 42)); Assert.Equal(12345u, value); + value = 42; + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345, 41)); + Assert.Equal(42u, value); + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345, 42)); + Assert.Equal(12345u, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + } + + [Fact] + public void InterlockedCompareExchange_Char() + { + using BoundedMemory memory = BoundedMemory.Allocate(1); + ref char value = ref memory.Span[0]; + value = (char)42; + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, (char)12345, (char)41)); + Assert.Equal(42u, value); + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, (char)12345, (char)42)); + Assert.Equal(12345u, value); + + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), (char)12345, (char)41)); } [Fact] @@ -388,7 +552,16 @@ public void InterlockedCompareExchange_Int32() Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 42)); Assert.Equal(12345, value); + value = 42; + + Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 41)); + Assert.Equal(42, value); + + Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 42)); + Assert.Equal(12345, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); } [Fact] @@ -404,7 +577,32 @@ public void InterlockedCompareExchange_UInt32() Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345u, 42u)); Assert.Equal(12345u, value); + value = 42; + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345u, 41u)); + Assert.Equal(42u, value); + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345u, 42u)); + Assert.Equal(12345u, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + } + + [Fact] + public void InterlockedCompareExchange_Enum() + { + using BoundedMemory memory = BoundedMemory.Allocate(1); + ref DayOfWeek value = ref memory.Span[0]; + value = DayOfWeek.Monday; + + Assert.Equal(DayOfWeek.Monday, Interlocked.CompareExchange(ref value, DayOfWeek.Tuesday, DayOfWeek.Monday)); + Assert.Equal(DayOfWeek.Tuesday, value); + + Assert.Equal(DayOfWeek.Tuesday, Interlocked.CompareExchange(ref value, DayOfWeek.Wednesday, DayOfWeek.Monday)); + Assert.Equal(DayOfWeek.Tuesday, value); + + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), DayOfWeek.Monday, DayOfWeek.Tuesday)); } [Fact] @@ -420,7 +618,16 @@ public void InterlockedCompareExchange_Int64() Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 42)); Assert.Equal(12345, value); + value = 42; + + Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 41)); + Assert.Equal(42, value); + + Assert.Equal(42, Interlocked.CompareExchange(ref value, 12345, 42)); + Assert.Equal(12345, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); } [Fact] @@ -430,19 +637,31 @@ public void InterlockedCompareExchange_IntPtr() ref nint value = ref memory.Span[0]; value = 42; - Assert.Equal(42, (nint)Interlocked.CompareExchange(ref value, (nint)12345, (nint)41)); + Assert.Equal(42, Interlocked.CompareExchange(ref value, (nint)12345, (nint)41)); Assert.Equal(42, value); - Assert.Equal(42, (nint)Interlocked.CompareExchange(ref value, (nint)12345, (nint)42)); + Assert.Equal(42, Interlocked.CompareExchange(ref value, (nint)12345, (nint)42)); + Assert.Equal(12345, value); + + value = 42; + + Assert.Equal(42, Interlocked.CompareExchange(ref value, (nint)12345, (nint)41)); + Assert.Equal(42, value); + + Assert.Equal(42, Interlocked.CompareExchange(ref value, (nint)12345, (nint)42)); Assert.Equal(12345, value); if (Environment.Is64BitProcess) { - Assert.Equal(12345, (nint)Interlocked.CompareExchange(ref value, unchecked((nint)1 + int.MaxValue), (nint)12345u)); + Assert.Equal(12345, Interlocked.CompareExchange(ref value, unchecked((nint)1 + int.MaxValue), (nint)12345)); Assert.Equal(unchecked((nint)1 + int.MaxValue), value); + + Assert.Equal(unchecked((nint)1 + int.MaxValue), Interlocked.CompareExchange(ref value, unchecked((nint)2 + int.MaxValue), unchecked((nint)1 + int.MaxValue))); + Assert.Equal(unchecked((nint)2 + int.MaxValue), value); } Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); } [Fact] @@ -458,7 +677,16 @@ public void InterlockedCompareExchange_UInt64() Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345u, 42u)); Assert.Equal(12345u, value); + value = 42; + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345u, 41u)); + Assert.Equal(42u, value); + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, 12345u, 42u)); + Assert.Equal(12345u, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); } [Fact] @@ -468,19 +696,31 @@ public void InterlockedCompareExchange_UIntPtr() ref nuint value = ref memory.Span[0]; value = 42; - Assert.Equal(42u, (nuint)Interlocked.CompareExchange(ref value, (nuint)12345u, (nuint)41u)); + Assert.Equal(42u, Interlocked.CompareExchange(ref value, (nuint)12345u, (nuint)41u)); + Assert.Equal(42u, value); + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, (nuint)12345u, (nuint)42u)); + Assert.Equal(12345u, value); + + value = 42; + + Assert.Equal(42u, Interlocked.CompareExchange(ref value, (nuint)12345u, (nuint)41u)); Assert.Equal(42u, value); - Assert.Equal(42u, (nuint)Interlocked.CompareExchange(ref value, (nuint)12345u, (nuint)42u)); + Assert.Equal(42u, Interlocked.CompareExchange(ref value, (nuint)12345u, (nuint)42u)); Assert.Equal(12345u, value); if (Environment.Is64BitProcess) { Assert.Equal(12345u, (nuint)Interlocked.CompareExchange(ref value, unchecked((nuint)1 + uint.MaxValue), (nuint)12345u)); Assert.Equal(unchecked((nuint)1 + uint.MaxValue), value); + + Assert.Equal(unchecked((nuint)1 + uint.MaxValue), Interlocked.CompareExchange(ref value, unchecked((nuint)2 + uint.MaxValue), unchecked((nuint)1 + uint.MaxValue))); + Assert.Equal(unchecked((nuint)2 + uint.MaxValue), value); } Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345, 41)); } [Fact] @@ -496,7 +736,16 @@ public void InterlockedCompareExchange_Float() Assert.Equal(42.1f, Interlocked.CompareExchange(ref value, 12345.1f, 42.1f)); Assert.Equal(12345.1f, value); + value = 42.1f; + + Assert.Equal(42.1f, Interlocked.CompareExchange(ref value, 12345.1f, 41.1f)); + Assert.Equal(42.1f, value); + + Assert.Equal(42.1f, Interlocked.CompareExchange(ref value, 12345.1f, 42.1f)); + Assert.Equal(12345.1f, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345.1f, 41.1f)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345.1f, 41.1f)); } [Fact] @@ -512,7 +761,16 @@ public void InterlockedCompareExchange_Double() Assert.Equal(42.1, Interlocked.CompareExchange(ref value, 12345.1, 42.1)); Assert.Equal(12345.1, value); + value = 42.1; + + Assert.Equal(42.1, Interlocked.CompareExchange(ref value, 12345.1, 41.1)); + Assert.Equal(42.1, value); + + Assert.Equal(42.1, Interlocked.CompareExchange(ref value, 12345.1, 42.1)); + Assert.Equal(12345.1, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345.1, 41.1)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), 12345.1, 41.1)); } [Fact] @@ -528,8 +786,18 @@ public void InterlockedCompareExchange_Object() Assert.Same(oldValue, Interlocked.CompareExchange(ref value, newValue, oldValue)); Assert.Same(newValue, value); + value = oldValue; + + Assert.Same(oldValue, Interlocked.CompareExchange(ref value, newValue, new object())); + Assert.Same(oldValue, value); + + Assert.Same(oldValue, Interlocked.CompareExchange(ref value, newValue, oldValue)); + Assert.Same(newValue, value); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), null, null)); Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), newValue, oldValue)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), null, null)); + Assert.Throws(() => Interlocked.CompareExchange(ref Unsafe.NullRef(), newValue, oldValue)); } [Fact] @@ -552,6 +820,20 @@ public void InterlockedCompareExchange_BoxedObject() Assert.Equal(12345, (int)value); } + [Fact] + public void InterlockedCompareExchange_Unsupported() + { + DateTime value1 = default; + TimeSpan value2 = default; + Rune value3 = default; + ValueTask value4 = default; + + Assert.Throws(() => Interlocked.CompareExchange(ref value1, default, default)); + Assert.Throws(() => Interlocked.CompareExchange(ref value2, default, default)); + Assert.Throws(() => Interlocked.CompareExchange(ref value3, default, default)); + Assert.Throws(() => Interlocked.CompareExchange(ref value4, default, default)); + } + [Fact] public void InterlockedRead_Int64() { diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Interlocked.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Interlocked.Mono.cs index 79e193e70e09c..6f878a6a2b758 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Interlocked.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Interlocked.Mono.cs @@ -72,47 +72,10 @@ public static partial class Interlocked [MethodImplAttribute(MethodImplOptions.InternalCall)] public static extern long CompareExchange(ref long location1, long value, long comparand); - [return: NotNullIfNotNull(nameof(location1))] - [Intrinsic] - public static T CompareExchange(ref T location1, T value, T comparand) where T : class? - { - if (Unsafe.IsNullRef(ref location1)) - throw new NullReferenceException(); - // Besides avoiding coop handles for efficiency, - // and correctness, this also appears needed to - // avoid an assertion failure in the runtime, related to - // coop handles over generics. - // - // See CompareExchange(object) for comments. - // - // This is not entirely convincing due to lack of volatile. - // - T? result = null; - // T : class so call the object overload. - CompareExchange(ref Unsafe.As(ref location1), ref Unsafe.As(ref value), ref Unsafe.As(ref comparand), ref Unsafe.As(ref result!)); - return result; - } - [Intrinsic] [MethodImplAttribute(MethodImplOptions.InternalCall)] public static extern long Exchange(ref long location1, long value); - [return: NotNullIfNotNull(nameof(location1))] - [Intrinsic] - public static T Exchange([NotNullIfNotNull(nameof(value))] ref T location1, T value) where T : class? - { - if (Unsafe.IsNullRef(ref location1)) - throw new NullReferenceException(); - // See CompareExchange(T) for comments. - // - // This is not entirely convincing due to lack of volatile. - // - T? result = null; - // T : class so call the object overload. - Exchange(ref Unsafe.As(ref location1), ref Unsafe.As(ref value), ref Unsafe.As(ref result!)); - return result; - } - [Intrinsic] [MethodImplAttribute(MethodImplOptions.InternalCall)] public static extern long Read(ref long location);