From 83e3f8346f49868df369776d6c11779ad6c479bc Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 3 Apr 2019 09:40:20 -0400 Subject: [PATCH] Nullable: ReaderWriterLockSlim (#23657) * Nullable: ReaderWriterLockSlim * Address PR feedback * Audit System.Threading for unannotated statics --- .../System/Threading/ManualResetEventSlim.cs | 2 +- .../System/Threading/ReaderWriterLockSlim.cs | 81 +++++++++++----------- .../shared/System/Threading/SemaphoreSlim.cs | 2 +- .../shared/System/Threading/SpinLock.cs | 2 +- .../shared/System/Threading/Thread.cs | 2 +- .../shared/System/Threading/TimerQueue.Portable.cs | 4 +- 6 files changed, 47 insertions(+), 46 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Threading/ManualResetEventSlim.cs b/src/System.Private.CoreLib/shared/System/Threading/ManualResetEventSlim.cs index c4bdafa..0361327 100644 --- a/src/System.Private.CoreLib/shared/System/Threading/ManualResetEventSlim.cs +++ b/src/System.Private.CoreLib/shared/System/Threading/ManualResetEventSlim.cs @@ -683,7 +683,7 @@ namespace System.Threading /// /// Private helper method to wake up waiters when a cancellationToken gets canceled. /// - private static Action s_cancellationTokenCallback = new Action(CancellationTokenCallback); + private static readonly Action s_cancellationTokenCallback = new Action(CancellationTokenCallback); private static void CancellationTokenCallback(object? obj) { Debug.Assert(obj is ManualResetEventSlim, "Expected a ManualResetEventSlim"); diff --git a/src/System.Private.CoreLib/shared/System/Threading/ReaderWriterLockSlim.cs b/src/System.Private.CoreLib/shared/System/Threading/ReaderWriterLockSlim.cs index 9776871..f25c08d 100644 --- a/src/System.Private.CoreLib/shared/System/Threading/ReaderWriterLockSlim.cs +++ b/src/System.Private.CoreLib/shared/System/Threading/ReaderWriterLockSlim.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +#nullable enable using System.Diagnostics; // for TraceInformation using System.Runtime.CompilerServices; @@ -40,7 +41,7 @@ namespace System.Threading public int upgradecount; // Next RWC in this thread's list. - public ReaderWriterCount next; + public ReaderWriterCount? next; } /// @@ -74,10 +75,10 @@ namespace System.Threading private int _writeLockOwnerId; // conditions we wait on. - private EventWaitHandle _writeEvent; // threads waiting to acquire a write lock go here. - private EventWaitHandle _readEvent; // threads waiting to acquire a read lock go here (will be released in bulk) - private EventWaitHandle _upgradeEvent; // thread waiting to acquire the upgrade lock - private EventWaitHandle _waitUpgradeEvent; // thread waiting to upgrade from the upgrade lock to a write lock go here (at most one) + private EventWaitHandle? _writeEvent; // threads waiting to acquire a write lock go here. + private EventWaitHandle? _readEvent; // threads waiting to acquire a read lock go here (will be released in bulk) + private EventWaitHandle? _upgradeEvent; // thread waiting to acquire the upgrade lock + private EventWaitHandle? _waitUpgradeEvent; // thread waiting to upgrade from the upgrade lock to a write lock go here (at most one) // Every lock instance has a unique ID, which is used by ReaderWriterCount to associate itself with the lock // without holding a reference to it. @@ -86,7 +87,7 @@ namespace System.Threading // See comments on ReaderWriterCount. [ThreadStatic] - private static ReaderWriterCount t_rwc; + private static ReaderWriterCount? t_rwc; private bool _fUpgradeThreadHoldingRead; @@ -195,10 +196,10 @@ namespace System.Threading /// could not be found. /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - private ReaderWriterCount GetThreadRWCount(bool dontAllocate) + private ReaderWriterCount? GetThreadRWCount(bool dontAllocate) // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 { - ReaderWriterCount rwc = t_rwc; - ReaderWriterCount empty = null; + ReaderWriterCount? rwc = t_rwc; + ReaderWriterCount? empty = null; while (rwc != null) { if (rwc.lockID == _lockID) @@ -305,7 +306,7 @@ namespace System.Threading if (_fDisposed) throw new ObjectDisposedException(null); - ReaderWriterCount lrwc = null; + ReaderWriterCount lrwc; int id = Environment.CurrentManagedThreadId; if (!_fIsReentrant) @@ -318,7 +319,7 @@ namespace System.Threading _spinLock.Enter(EnterSpinLockReason.EnterAnyRead); - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 //Check if the reader lock is already acquired. Note, we could //check the presence of a reader by not allocating rwc (But that @@ -343,7 +344,7 @@ namespace System.Threading else { _spinLock.Enter(EnterSpinLockReason.EnterAnyRead); - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 if (lrwc.readercount > 0) { lrwc.readercount++; @@ -401,7 +402,7 @@ namespace System.Threading _spinLock.Enter(EnterSpinLockReason.EnterAnyRead); //The per-thread structure may have been recycled as the lock is acquired (due to message pumping), load again. if (IsRwHashEntryChanged(lrwc)) - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 continue; } @@ -410,7 +411,7 @@ namespace System.Threading { LazyCreateEvent(ref _readEvent, EnterLockType.Read); if (IsRwHashEntryChanged(lrwc)) - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 continue; // since we left the lock, start over. } @@ -420,7 +421,7 @@ namespace System.Threading return false; } if (IsRwHashEntryChanged(lrwc)) - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 } _spinLock.Exit(); @@ -453,7 +454,7 @@ namespace System.Threading throw new ObjectDisposedException(null); int id = Environment.CurrentManagedThreadId; - ReaderWriterCount lrwc; + ReaderWriterCount? lrwc; bool upgradingToWrite = false; if (!_fIsReentrant) @@ -476,7 +477,7 @@ namespace System.Threading } _spinLock.Enter(enterMyLockReason); - lrwc = GetThreadRWCount(true); + lrwc = GetThreadRWCount(dontAllocate: true); //Can't acquire write lock with reader lock held. if (lrwc != null && lrwc.readercount > 0) @@ -502,7 +503,7 @@ namespace System.Threading } _spinLock.Enter(enterMyLockReason); - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 if (id == _writeLockOwnerId) { @@ -555,7 +556,7 @@ namespace System.Threading if (lrwc != null) { if (IsRwHashEntryChanged(lrwc)) - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 if (lrwc.readercount > 0) { @@ -622,8 +623,9 @@ namespace System.Threading if (_fIsReentrant) { + Debug.Assert(lrwc != null, "Initialized based on _fIsReentrant earlier in the method"); if (IsRwHashEntryChanged(lrwc)) - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 lrwc.writercount++; } @@ -660,7 +662,7 @@ namespace System.Threading throw new ObjectDisposedException(null); int id = Environment.CurrentManagedThreadId; - ReaderWriterCount lrwc; + ReaderWriterCount? lrwc; if (!_fIsReentrant) { @@ -676,7 +678,7 @@ namespace System.Threading } _spinLock.Enter(EnterSpinLockReason.EnterAnyRead); - lrwc = GetThreadRWCount(true); + lrwc = GetThreadRWCount(dontAllocate: true); //Can't acquire upgrade lock with reader lock held. if (lrwc != null && lrwc.readercount > 0) { @@ -687,7 +689,7 @@ namespace System.Threading else { _spinLock.Enter(EnterSpinLockReason.EnterAnyRead); - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 if (id == _upgradeLockOwnerId) { @@ -764,8 +766,9 @@ namespace System.Threading { //The lock may have been dropped getting here, so make a quick check to see whether some other //thread did not grab the entry. + Debug.Assert(lrwc != null, "Initialized based on _fIsReentrant earlier in the method"); if (IsRwHashEntryChanged(lrwc)) - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 lrwc.upgradecount++; } @@ -776,11 +779,9 @@ namespace System.Threading public void ExitReadLock() { - ReaderWriterCount lrwc = null; - _spinLock.Enter(EnterSpinLockReason.ExitAnyRead); - lrwc = GetThreadRWCount(true); + ReaderWriterCount? lrwc = GetThreadRWCount(dontAllocate: true); if (lrwc == null || lrwc.readercount < 1) { @@ -829,7 +830,7 @@ namespace System.Threading else { _spinLock.Enter(EnterSpinLockReason.ExitAnyWrite); - lrwc = GetThreadRWCount(false); + lrwc = GetThreadRWCount(dontAllocate: false)!; // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 if (lrwc == null) { @@ -863,7 +864,7 @@ namespace System.Threading public void ExitUpgradeableReadLock() { - ReaderWriterCount lrwc; + ReaderWriterCount? lrwc; if (!_fIsReentrant) { if (Environment.CurrentManagedThreadId != _upgradeLockOwnerId) @@ -876,7 +877,7 @@ namespace System.Threading else { _spinLock.Enter(EnterSpinLockReason.ExitAnyRead); - lrwc = GetThreadRWCount(true); + lrwc = GetThreadRWCount(dontAllocate: true); if (lrwc == null) { @@ -913,7 +914,7 @@ namespace System.Threading /// while holding a spin lock). If all goes well, reenter the lock and /// set 'waitEvent' /// - private void LazyCreateEvent(ref EventWaitHandle waitEvent, EnterLockType enterLockType) + private void LazyCreateEvent(ref EventWaitHandle? waitEvent, EnterLockType enterLockType) // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/26761 { #if DEBUG Debug.Assert(_spinLock.IsHeld); @@ -1098,7 +1099,7 @@ namespace System.Threading if (_numWriteUpgradeWaiters > 0 && _fUpgradeThreadHoldingRead && readercount == 2) { _spinLock.Exit(); // Exit before signaling to improve efficiency (wakee will need the lock) - _waitUpgradeEvent.Set(); // release all upgraders (however there can be at most one). + _waitUpgradeEvent!.Set(); // release all upgraders (however there can be at most one). Known non-null because _numWriteUpgradeWaiters > 0. return; } } @@ -1107,10 +1108,10 @@ namespace System.Threading { //We have to be careful now, as we are dropping the lock. //No new writes should be allowed to sneak in if an upgrade - //was pending. + //was pending. _spinLock.Exit(); // Exit before signaling to improve efficiency (wakee will need the lock) - _waitUpgradeEvent.Set(); // release all upgraders (however there can be at most one). + _waitUpgradeEvent!.Set(); // release all upgraders (however there can be at most one). Known non-null because _numWriteUpgradeWaiters > 0. } else if (readercount == 0 && _numWriteWaiters > 0) { @@ -1126,7 +1127,7 @@ namespace System.Threading if (signaled == WaiterStates.None) { - _writeEvent.Set(); // release one writer. + _writeEvent!.Set(); // release one writer. Known non-null because _numWriteWaiters > 0. } } else @@ -1168,10 +1169,10 @@ namespace System.Threading _spinLock.Exit(); // Exit before signaling to improve efficiency (wakee will need the lock) if (setReadEvent) - _readEvent.Set(); // release all readers. + _readEvent!.Set(); // release all readers. Known non-null because _numUpgradeWaiters != 0. if (setUpgradeEvent) - _upgradeEvent.Set(); //release one upgrader. + _upgradeEvent!.Set(); //release one upgrader. } private bool IsWriterAcquired() @@ -1364,7 +1365,7 @@ namespace System.Threading get { int count = 0; - ReaderWriterCount lrwc = GetThreadRWCount(true); + ReaderWriterCount? lrwc = GetThreadRWCount(dontAllocate: true); if (lrwc != null) count = lrwc.readercount; @@ -1380,7 +1381,7 @@ namespace System.Threading { int count = 0; - ReaderWriterCount lrwc = GetThreadRWCount(true); + ReaderWriterCount? lrwc = GetThreadRWCount(dontAllocate: true); if (lrwc != null) count = lrwc.upgradecount; @@ -1404,7 +1405,7 @@ namespace System.Threading { int count = 0; - ReaderWriterCount lrwc = GetThreadRWCount(true); + ReaderWriterCount? lrwc = GetThreadRWCount(dontAllocate: true); if (lrwc != null) count = lrwc.writercount; diff --git a/src/System.Private.CoreLib/shared/System/Threading/SemaphoreSlim.cs b/src/System.Private.CoreLib/shared/System/Threading/SemaphoreSlim.cs index 750d24d..ea5bc4d 100644 --- a/src/System.Private.CoreLib/shared/System/Threading/SemaphoreSlim.cs +++ b/src/System.Private.CoreLib/shared/System/Threading/SemaphoreSlim.cs @@ -898,7 +898,7 @@ namespace System.Threading /// /// Private helper method to wake up waiters when a cancellationToken gets canceled. /// - private static Action s_cancellationTokenCanceledEventHandler = new Action(CancellationTokenCanceledEventHandler); + private static readonly Action s_cancellationTokenCanceledEventHandler = new Action(CancellationTokenCanceledEventHandler); private static void CancellationTokenCanceledEventHandler(object? obj) { Debug.Assert(obj is SemaphoreSlim, "Expected a SemaphoreSlim"); diff --git a/src/System.Private.CoreLib/shared/System/Threading/SpinLock.cs b/src/System.Private.CoreLib/shared/System/Threading/SpinLock.cs index 6149c82..1e6bcc5 100644 --- a/src/System.Private.CoreLib/shared/System/Threading/SpinLock.cs +++ b/src/System.Private.CoreLib/shared/System/Threading/SpinLock.cs @@ -91,7 +91,7 @@ namespace System.Threading // The maximum number of waiters (only used if the thread tracking is disabled) // The actual maximum waiters count is this number divided by two because each waiter increments the waiters count by 2 // The waiters count is calculated by m_owner & WAITERS_MASK 01111....110 - private static int MAXIMUM_WAITERS = WAITERS_MASK; + private const int MAXIMUM_WAITERS = WAITERS_MASK; [MethodImpl(MethodImplOptions.AggressiveInlining)] private static int CompareExchange(ref int location, int value, int comparand, ref bool success) diff --git a/src/System.Private.CoreLib/shared/System/Threading/Thread.cs b/src/System.Private.CoreLib/shared/System/Threading/Thread.cs index ff8d646..3a64d01 100644 --- a/src/System.Private.CoreLib/shared/System/Threading/Thread.cs +++ b/src/System.Private.CoreLib/shared/System/Threading/Thread.cs @@ -19,7 +19,7 @@ namespace System.Threading private static AsyncLocal? s_asyncLocalPrincipal; [ThreadStatic] - private static Thread t_currentThread; + private static Thread? t_currentThread; public Thread(ThreadStart start) : this() diff --git a/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Portable.cs b/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Portable.cs index c9469c0..808ccc9 100644 --- a/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Portable.cs +++ b/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Portable.cs @@ -12,8 +12,8 @@ namespace System.Threading // internal partial class TimerQueue : IThreadPoolWorkItem { - private static List s_scheduledTimers; - private static List s_scheduledTimersToFire; + private static List? s_scheduledTimers; + private static List? s_scheduledTimersToFire; /// /// This event is used by the timer thread to wait for timer expiration. It is also -- 2.7.4