Nullable: ReaderWriterLockSlim (#23657)
authorStephen Toub <stoub@microsoft.com>
Wed, 3 Apr 2019 13:40:20 +0000 (09:40 -0400)
committerGitHub <noreply@github.com>
Wed, 3 Apr 2019 13:40:20 +0000 (09:40 -0400)
* Nullable: ReaderWriterLockSlim

* Address PR feedback

* Audit System.Threading for unannotated statics

src/System.Private.CoreLib/shared/System/Threading/ManualResetEventSlim.cs
src/System.Private.CoreLib/shared/System/Threading/ReaderWriterLockSlim.cs
src/System.Private.CoreLib/shared/System/Threading/SemaphoreSlim.cs
src/System.Private.CoreLib/shared/System/Threading/SpinLock.cs
src/System.Private.CoreLib/shared/System/Threading/Thread.cs
src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Portable.cs

index c4bdafa..0361327 100644 (file)
@@ -683,7 +683,7 @@ namespace System.Threading
         /// <summary>
         /// Private helper method to wake up waiters when a cancellationToken gets canceled.
         /// </summary>
-        private static Action<object?> s_cancellationTokenCallback = new Action<object?>(CancellationTokenCallback);
+        private static readonly Action<object?> s_cancellationTokenCallback = new Action<object?>(CancellationTokenCallback);
         private static void CancellationTokenCallback(object? obj)
         {
             Debug.Assert(obj is ManualResetEventSlim, "Expected a ManualResetEventSlim");
index 9776871..f25c08d 100644 (file)
@@ -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;
     }
 
     /// <summary>
@@ -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.
         /// </summary>
         [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' 
         /// </summary>
-        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;
 
index 750d24d..ea5bc4d 100644 (file)
@@ -898,7 +898,7 @@ namespace System.Threading
         /// <summary>
         /// Private helper method to wake up waiters when a cancellationToken gets canceled.
         /// </summary>
-        private static Action<object?> s_cancellationTokenCanceledEventHandler = new Action<object?>(CancellationTokenCanceledEventHandler);
+        private static readonly Action<object?> s_cancellationTokenCanceledEventHandler = new Action<object?>(CancellationTokenCanceledEventHandler);
         private static void CancellationTokenCanceledEventHandler(object? obj)
         {
             Debug.Assert(obj is SemaphoreSlim, "Expected a SemaphoreSlim");
index 6149c82..1e6bcc5 100644 (file)
@@ -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)
index ff8d646..3a64d01 100644 (file)
@@ -19,7 +19,7 @@ namespace System.Threading
         private static AsyncLocal<IPrincipal?>? s_asyncLocalPrincipal;
 
         [ThreadStatic]
-        private static Thread t_currentThread;
+        private static Thread? t_currentThread;
 
         public Thread(ThreadStart start)
             : this()
index c9469c0..808ccc9 100644 (file)
@@ -12,8 +12,8 @@ namespace System.Threading
     //
     internal partial class TimerQueue : IThreadPoolWorkItem
     {
-        private static List<TimerQueue> s_scheduledTimers;
-        private static List<TimerQueue> s_scheduledTimersToFire;
+        private static List<TimerQueue>? s_scheduledTimers;
+        private static List<TimerQueue>? s_scheduledTimersToFire;
 
         /// <summary>
         /// This event is used by the timer thread to wait for timer expiration. It is also