Thread class improvements

src/Common/src/Interop/Windows/mincore/Interop.Threading.cs

@@ -53,7 +53,7 @@ internal static partial class mincore
         internal extern static void Sleep(uint milliseconds);
 
         [DllImport(Libraries.Kernel32)]
-        internal extern static unsafe IntPtr CreateThread(
+        internal extern static unsafe SafeWaitHandle CreateThread(
             IntPtr lpThreadAttributes,
             IntPtr dwStackSize,
             IntPtr lpStartAddress,
@@ -63,6 +63,9 @@ internal extern static unsafe IntPtr CreateThread(
 
         internal delegate uint ThreadProc(IntPtr lpParameter);
 
+        [DllImport(Libraries.Kernel32)]
+        internal extern static uint ResumeThread(SafeWaitHandle hThread);
+
         [DllImport(Libraries.Kernel32)]
         internal extern static IntPtr GetCurrentProcess();
 

src/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeThread.Unix.cs

@@ -18,6 +18,8 @@ private void PlatformSpecificInitializeExistingThread() { }
 
         internal WaitSubsystem.ThreadWaitInfo WaitInfo => _waitInfo;
 
+        private static ThreadPriority GetPriority() { return ThreadPriority.Normal; }
+
         private static bool SetPriority(ThreadPriority priority) { return true; }
 
         private bool HasFinishedExecution()

src/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeThread.Windows.cs

@@ -69,12 +69,13 @@ private static SafeWaitHandle GetOSHandleForCurrentThread()
 
         private static ThreadPriority MapFromOSPriority(OSThreadPriority priority)
         {
+            if (priority <= OSThreadPriority.Lowest)
+            {
+                // OS thread priorities in the [Idle,Lowest] range are mapped to ThreadPriority.Lowest
+                return ThreadPriority.Lowest;
+            }
             switch (priority)
             {
-                case OSThreadPriority.Idle:
-                case OSThreadPriority.Lowest:
-                    return ThreadPriority.Lowest;
-
                 case OSThreadPriority.BelowNormal:
                     return ThreadPriority.BelowNormal;
 
@@ -84,17 +85,18 @@ private static ThreadPriority MapFromOSPriority(OSThreadPriority priority)
                 case OSThreadPriority.AboveNormal:
                     return ThreadPriority.AboveNormal;
 
-                case OSThreadPriority.Highest:
-                case OSThreadPriority.TimeCritical:
-                    return ThreadPriority.Highest;
-
                 case OSThreadPriority.ErrorReturn:
                     Debug.Fail("GetThreadPriority failed");
                     return ThreadPriority.Normal;
-
-                default:
-                    return ThreadPriority.Normal;
             }
+            // Handle OSThreadPriority.ErrorReturn value before this check!
+            if (priority >= OSThreadPriority.Highest)
+            {
+                // OS thread priorities in the [Highest,TimeCritical] range are mapped to ThreadPriority.Highest
+                return ThreadPriority.Highest;
+            }
+            Debug.Fail("Unreachable");
+            return ThreadPriority.Normal;
         }
 
         private static OSThreadPriority MapToOSPriority(ThreadPriority priority)
@@ -117,15 +119,31 @@ private static OSThreadPriority MapToOSPriority(ThreadPriority priority)
                     return OSThreadPriority.Highest;
 
                 default:
-                    Environment.FailFast("Unreached");
+                    Debug.Fail("Unreachable");
                     return OSThreadPriority.Normal;
             }
         }
 
+        private ThreadPriority GetPriority()
+        {
+            if (_osHandle.IsInvalid)
+            {
+                // The thread has not been started yet; return the value assigned to the Priority property.
+                // Race condition with setting the priority or starting the thread is OK, we may return an old value.
+                return _priority;
+            }
+
+            // The priority might have been changed by external means. Obtain the actual value from the OS
+            // rather than using the value saved in _priority.
+            OSThreadPriority osPriority = Interop.mincore.GetThreadPriority(_osHandle);
+            return MapFromOSPriority(osPriority);
+        }
+
         private bool SetPriority(ThreadPriority priority)
         {
             if (_osHandle.IsInvalid)
             {
+                Debug.Assert(GetThreadStateBit(ThreadState.Unstarted));
                 // We will set the priority (saved in _priority) when we create an OS thread
                 return true;
             }
@@ -135,8 +153,16 @@ private bool SetPriority(ThreadPriority priority)
         /// <summary>
         /// Checks if the underlying OS thread has finished execution.
         /// </summary>
+        /// <remarks>
+        /// Use this method only on started threads and threads being started in StartCore.
+        /// </remarks>
         private bool HasFinishedExecution()
         {
+            // If an external thread dies and its Thread object is resurrected, _osHandle will be finalized, i.e. invalid
+            if (_osHandle.IsInvalid)
+            {
+                return true;
+            }
             uint result = Interop.mincore.WaitForSingleObject(_osHandle, dwMilliseconds: 0);
             return result == (uint)Interop.Constants.WaitObject0;
         }
@@ -168,35 +194,68 @@ private void StartCore(object parameter)
                     throw new ThreadStateException(SR.ThreadState_AlreadyStarted);
                 }
 
-                // TODO: OOM hardening, _maxStackSize
+                const int AllocationGranularity = (int)System.Runtime.Constants.AllocationGranularity;
+
+                int stackSize = _maxStackSize;
+                if ((0 < stackSize) && (stackSize < AllocationGranularity))
+                {
+                    // If StackSizeParamIsAReservation flag is set and the reserve size specified by CreateThread's
+                    // dwStackSize parameter is less than or equal to the initially committed stack size specified in
+                    // the executable header, the reserve size will be set to the initially committed size rounded up
+                    // to the nearest multiple of 1 MiB. In all cases the reserve size is rounded up to the nearest
+                    // multiple of the system's allocation granularity (typically 64 KiB).
+                    //
+                    // To prevent overreservation of stack memory for small stackSize values, we increase stackSize to
+                    // the allocation granularity. We assume that the SizeOfStackCommit field of IMAGE_OPTIONAL_HEADER
+                    // is strictly smaller than the allocation granularity (the field's default value is 4 KiB);
+                    // otherwise, at least 1 MiB of memory will be reserved. Note that the desktop CLR increases
+                    // stackSize to 256 KiB if it is smaller than that.
+                    stackSize = AllocationGranularity;
+                }
+
+                bool waitingForThreadStart = false;
                 GCHandle threadHandle = GCHandle.Alloc(this);
                 _threadStartArg = parameter;
+                uint threadId;
 
                 try
                 {
-                    uint threadId;
+                    _osHandle = Interop.mincore.CreateThread(IntPtr.Zero, (IntPtr)stackSize,
+                        AddrofIntrinsics.AddrOf<Interop.mincore.ThreadProc>(StartThread), (IntPtr)threadHandle,
+                        (uint)(Interop.Constants.CreateSuspended | Interop.Constants.StackSizeParamIsAReservation),
+                        out threadId);
 
-                    _osHandle = new SafeWaitHandle(Interop.mincore.CreateThread(IntPtr.Zero, IntPtr.Zero,
-                        AddrofIntrinsics.AddrOf<Interop.mincore.ThreadProc>(StartThread), (IntPtr)threadHandle, 0, out threadId),
-                        ownsHandle: true);
-
-                    // Ignore errors (as in CoreCLR)
+                    // CoreCLR ignores OS errors while setting the priority, so do we
                     SetPriority(_priority);
 
-                    // Wait until the new thread is started (as in CoreCLR)
-                    while (GetThreadStateBit(ThreadState.Unstarted))
+                    Interop.mincore.ResumeThread(_osHandle);
+
+                    // Skip cleanup if any asynchronous exception happens while waiting for the thread start
+                    waitingForThreadStart = true;
+
+                    // Wait until the new thread either dies or reports itself as started
+                    while (GetThreadStateBit(ThreadState.Unstarted) && !HasFinishedExecution())
                     {
                         Yield();
                     }
+
+                    waitingForThreadStart = false;
                 }
                 finally
                 {
-                    if (_osHandle == null)
+                    Debug.Assert(!waitingForThreadStart, "Leaked threadHandle");
+                    if (!waitingForThreadStart)
                     {
                         threadHandle.Free();
                         _threadStartArg = null;
                     }
                 }
+
+                if (GetThreadStateBit(ThreadState.Unstarted))
+                {
+                    // Lack of memory is the only expected reason for thread creation failure
+                    throw new ThreadStartException(new OutOfMemoryException());
+                }
             }
         }
 
@@ -205,21 +264,32 @@ private static uint StartThread(IntPtr parameter)
         {
             GCHandle threadHandle = (GCHandle)parameter;
             RuntimeThread thread = (RuntimeThread)threadHandle.Target;
-            // TODO: OOM hardening
-            t_currentThread = thread;
-            System.Threading.ManagedThreadId.SetForCurrentThread(thread._managedThreadId);
-            threadHandle.Free();
+            Delegate threadStart = thread._threadStart;
+            // Get the value before clearing the ThreadState.Unstarted bit
+            object threadStartArg = thread._threadStartArg;
+
+            try
+            {
+                t_currentThread = thread;
+                System.Threading.ManagedThreadId.SetForCurrentThread(thread._managedThreadId);
+            }
+            catch (OutOfMemoryException)
+            {
+                // Terminate the current thread. The creator thread will throw a ThreadStartException.
+                return 0;
+            }
 
+            // Report success to the creator thread, which will free threadHandle and _threadStartArg
             thread.ClearThreadStateBit(ThreadState.Unstarted);
 
             try
             {
-                Delegate threadStart = thread._threadStart;
-                object threadStartArg = thread._threadStartArg;
+                // The Thread cannot be started more than once, so we may clean up the delegate
                 thread._threadStart = null;
-                thread._threadStartArg = null;
 
 #if ENABLE_WINRT
+                // If this call fails, COM and WinRT calls on this thread will fail with CO_E_NOTINITIALIZED.
+                // We may continue and fail on the actual call.
                 Interop.WinRT.RoInitialize(Interop.WinRT.RO_INIT_TYPE.RO_INIT_MULTITHREADED);
 #endif
 

src/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeThread.cs

@@ -123,29 +123,26 @@ public bool IsAlive
             }
         }
 
-        public bool IsDead
+        private bool IsDead()
         {
-            get
-            {
-                // Refresh ThreadState.Stopped bit if necessary
-                ThreadState state = GetThreadState();
-                return (state & (ThreadState.Stopped | ThreadState.Aborted)) != 0;
-            }
+            // Refresh ThreadState.Stopped bit if necessary
+            ThreadState state = GetThreadState();
+            return (state & (ThreadState.Stopped | ThreadState.Aborted)) != 0;
         }
 
         public bool IsBackground
         {
             get
             {
-                if (IsDead)
+                if (IsDead())
                 {
                     throw new ThreadStateException(SR.ThreadState_Dead_State);
                 }
                 return GetThreadStateBit(ThreadState.Background);
             }
             set
             {
-                if (IsDead)
+                if (IsDead())
                 {
                     throw new ThreadStateException(SR.ThreadState_Dead_State);
                 }
@@ -165,7 +162,7 @@ public bool IsThreadPoolThread
         {
             get
             {
-                if (IsDead)
+                if (IsDead())
                 {
                     throw new ThreadStateException(SR.ThreadState_Dead_State);
                 }
@@ -195,19 +192,19 @@ public ThreadPriority Priority
         {
             get
             {
-                if (IsDead)
+                if (IsDead())
                 {
                     throw new ThreadStateException(SR.ThreadState_Dead_Priority);
                 }
-                return _priority;
+                return GetPriority();
             }
             set
             {
                 if ((value < ThreadPriority.Lowest) || (ThreadPriority.Highest < value))
                 {
                     throw new ArgumentOutOfRangeException(SR.Argument_InvalidFlag);
                 }
-                if (IsDead)
+                if (IsDead())
                 {
                     throw new ThreadStateException(SR.ThreadState_Dead_Priority);
                 }

src/System.Private.CoreLib/src/Interop/Interop.manual.cs

@@ -22,9 +22,11 @@ internal enum Constants : uint
         EventModifyState = 0x2u,
         DuplicateSameAccess = 0x2u,
         FileTypeChar = 0x2u,
+        CreateSuspended = 0x4u,
         WaitAbandoned0 = 0x80u,
         WaitTimeout = 0x102u,
         MaxPath = 0x104u,
+        StackSizeParamIsAReservation = 0x10000u,
         Synchronize = 0x100000u,
         MutexAllAccess = 0x1F0001u,
         EventAllAccess = 0x1F0003u,

src/System.Private.CoreLib/src/System/Threading/ThreadStartException.cs

@@ -9,13 +9,13 @@ namespace System.Threading
     [Serializable]
     public sealed class ThreadStartException : SystemException
     {
-        private ThreadStartException()
+        internal ThreadStartException()
             : base(SR.Arg_ThreadStartException)
         {
             HResult = __HResults.COR_E_THREADSTART;
         }
 
-        private ThreadStartException(Exception reason)
+        internal ThreadStartException(Exception reason)
             : base(SR.Arg_ThreadStartException, reason)
         {
             HResult = __HResults.COR_E_THREADSTART;