Start thread pool worker threads in the default execution context (#4…

…6181)

Start thread pool worker threads in the default execution context

The execution context transfers to new threads, and this creates an extra stack frame on the thread and may cause AsyncLocal value change notifications to be sent unnecessarily. Added an internal `Thread.UnsafeStart` similarly to those on the `ThreadPool`, which would not capture the execution context, and used those for thread pool and timer threads.

src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs

@@ -70,7 +70,7 @@ internal void ThreadStart(object? obj)
             _startArg = obj;
 
             ExecutionContext? context = _executionContext;
-            if (context != null)
+            if (context != null && !context.IsDefault)
             {
                 ExecutionContext.RunInternal(context, s_threadStartContextCallback, this);
             }
@@ -87,7 +87,7 @@ internal void ThreadStart()
             Debug.Assert(_start is ThreadStart);
 
             ExecutionContext? context = _executionContext;
-            if (context != null)
+            if (context != null && !context.IsDefault)
             {
                 ExecutionContext.RunInternal(context, s_threadStartContextCallback, this);
             }
@@ -212,9 +212,6 @@ public void Start()
             StartupSetApartmentStateInternal();
 #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT
 
-            // Attach current thread's security principal object to the new
-            // thread. Be careful not to bind the current thread to a principal
-            // if it's not already bound.
             if (_delegate != null)
             {
                 // If we reach here with a null delegate, something is broken. But we'll let the StartInternal method take care of
@@ -229,6 +226,29 @@ public void Start()
             StartInternal();
         }
 
+        [UnsupportedOSPlatform("browser")]
+        internal void UnsafeStart(object? parameter)
+        {
+            // We expect the thread to be setup with a ParameterizedThreadStart if this Start is called.
+            Debug.Assert(_delegate is ThreadStart);
+
+            _threadStartArg = parameter;
+            UnsafeStart();
+        }
+
+        [UnsupportedOSPlatform("browser")]
+        internal void UnsafeStart()
+        {
+#if FEATURE_COMINTEROP_APARTMENT_SUPPORT
+            // Eagerly initialize the COM Apartment state of the thread if we're allowed to.
+            StartupSetApartmentStateInternal();
+#endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT
+
+            Debug.Assert(_delegate != null);
+            Debug.Assert(_delegate!.Target is ThreadHelper);
+            StartInternal();
+        }
+
         private void SetCultureOnUnstartedThreadNoCheck(CultureInfo value, bool uiCulture)
         {
             Debug.Assert(_delegate != null);

src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.GateThread.cs

@@ -168,11 +168,13 @@ private static void CreateGateThread(PortableThreadPool threadPoolInstance)
                 bool created = false;
                 try
                 {
+                    // Thread pool threads must start in the default execution context without transferring the context, so
+                    // using UnsafeStart() instead of Start()
                     Thread gateThread = new Thread(GateThreadStart, SmallStackSizeBytes);
                     gateThread.IsThreadPoolThread = true;
                     gateThread.IsBackground = true;
                     gateThread.Name = ".NET ThreadPool Gate";
-                    gateThread.Start();
+                    gateThread.UnsafeStart();
                     created = true;
                 }
                 finally

src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WaitThread.cs

@@ -183,11 +183,14 @@ internal class WaitThread
             public WaitThread()
             {
                 _waitHandles[0] = _changeHandlesEvent.SafeWaitHandle;
+
+                // Thread pool threads must start in the default execution context without transferring the context, so
+                // using UnsafeStart() instead of Start()
                 Thread waitThread = new Thread(WaitThreadStart, SmallStackSizeBytes);
                 waitThread.IsThreadPoolThread = true;
                 waitThread.IsBackground = true;
                 waitThread.Name = ".NET ThreadPool Wait";
-                waitThread.Start();
+                waitThread.UnsafeStart();
             }
 
             /// <summary>

src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs

@@ -288,10 +288,12 @@ private static bool TryCreateWorkerThread()
             {
                 try
                 {
+                    // Thread pool threads must start in the default execution context without transferring the context, so
+                    // using UnsafeStart() instead of Start()
                     Thread workerThread = new Thread(WorkerThreadStart);
                     workerThread.IsThreadPoolThread = true;
                     workerThread.IsBackground = true;
-                    workerThread.Start();
+                    workerThread.UnsafeStart();
                 }
                 catch (ThreadStartException)
                 {
@@ -301,6 +303,7 @@ private static bool TryCreateWorkerThread()
                 {
                     return false;
                 }
+
                 return true;
             }
         }

src/libraries/System.Private.CoreLib/src/System/Threading/TimerQueue.Portable.cs

@@ -34,9 +34,11 @@ private static List<TimerQueue> InitializeScheduledTimerManager_Locked()
             var timers = new List<TimerQueue>(Instances.Length);
             s_scheduledTimersToFire ??= new List<TimerQueue>(Instances.Length);
 
+            // The timer thread must start in the default execution context without transferring the context, so
+            // using UnsafeStart() instead of Start()
             Thread timerThread = new Thread(TimerThread);
             timerThread.IsBackground = true;
-            timerThread.Start();
+            timerThread.UnsafeStart();
 
             // Do this after creating the thread in case thread creation fails so that it will try again next time
             s_scheduledTimers = timers;

src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs

@@ -835,6 +835,56 @@ public static void ThreadPoolCanProcessManyWorkItemsInParallelWithoutDeadlocking
             done.CheckedWait();
         }
 
+        [ThreadStatic]
+        private static int t_ThreadPoolThreadCreationDoesNotTransferExecutionContext_asyncLocalSideEffect;
+
+        [ConditionalFact(nameof(IsThreadingAndRemoteExecutorSupported))]
+        public static void ThreadPoolThreadCreationDoesNotTransferExecutionContext()
+        {
+            // Run in a separate process to test in a clean thread pool environment such that work items queued by the test
+            // would cause the thread pool to create threads
+            RemoteExecutor.Invoke(() =>
+            {
+                var done = new AutoResetEvent(false);
+
+                // Create an AsyncLocal with value change notifications, this changes the EC on this thread to non-default
+                var asyncLocal = new AsyncLocal<int>(e =>
+                {
+                    // There is nothing in this test that should cause a thread's EC to change due to EC flow
+                    Assert.False(e.ThreadContextChanged);
+
+                    // Record a side-effect from AsyncLocal value changes caused by flow. This is mainly because AsyncLocal
+                    // value change notifications can have side-effects like impersonation, we want to ensure that not only the
+                    // AsyncLocal's value is correct, but also that the side-effect matches the value, confirming that any value
+                    // changes cause matching notifications.
+                    t_ThreadPoolThreadCreationDoesNotTransferExecutionContext_asyncLocalSideEffect = e.CurrentValue;
+                });
+                asyncLocal.Value = 1;
+
+                ThreadPool.UnsafeQueueUserWorkItem(_ =>
+                {
+                    // The EC should not have flowed. If the EC had flowed, the assertion in the value change notification would
+                    // fail. Just for additional verification, check the side-effect as well.
+                    Assert.Equal(0, t_ThreadPoolThreadCreationDoesNotTransferExecutionContext_asyncLocalSideEffect);
+
+                    done.Set();
+                }, null);
+                done.CheckedWait();
+
+                ThreadPool.UnsafeRegisterWaitForSingleObject(done, (_, timedOut) =>
+                {
+                    Assert.True(timedOut);
+
+                    // The EC should not have flowed. If the EC had flowed, the assertion in the value change notification would
+                    // fail. Just for additional verification, check the side-effect as well.
+                    Assert.Equal(0, t_ThreadPoolThreadCreationDoesNotTransferExecutionContext_asyncLocalSideEffect);
+
+                    done.Set();
+                }, null, 0, true);
+                done.CheckedWait();
+            }).Dispose();
+        }
+
         public static bool IsThreadingAndRemoteExecutorSupported =>
             PlatformDetection.IsThreadingSupported && RemoteExecutor.IsSupported;
     }

src/libraries/System.Threading/tests/ExecutionContextTests.cs

@@ -237,14 +237,19 @@ private static void VerifyExecutionContextFlow(AsyncLocal<int> asyncLocal, int e
             }
             VerifyExecutionContext(ExecutionContext.Capture(), asyncLocal, expectedValue);
 
+            // Creating a thread flows context if and only if flow is not suppressed
             int asyncLocalValue = -1;
-            var done = new ManualResetEvent(false);
-            ThreadPool.QueueUserWorkItem(
-                state =>
-                {
-                    asyncLocalValue = asyncLocal.Value;
-                    done.Set();
-                });
+            ThreadTestHelpers.RunTestInBackgroundThread(() => asyncLocalValue = asyncLocal.Value);
+            Assert.Equal(expectedValue, asyncLocalValue);
+
+            // Queueing a thread pool work item flows context if and only if flow is not suppressed
+            asyncLocalValue = -1;
+            var done = new AutoResetEvent(false);
+            ThreadPool.QueueUserWorkItem(state =>
+            {
+                asyncLocalValue = asyncLocal.Value;
+                done.Set();
+            });
             done.CheckedWait();
             Assert.Equal(expectedValue, asyncLocalValue);
         }

src/mono/netcore/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.Runtime.CompilerServices;
@@ -254,6 +255,7 @@ public static void Sleep(int millisecondsTimeout)
         [UnsupportedOSPlatform("browser")]
         public void Start()
         {
+            _executionContext = ExecutionContext.Capture();
             StartInternal(this);
         }
 
@@ -264,23 +266,70 @@ public void Start(object parameter)
                 throw new InvalidOperationException(SR.InvalidOperation_ThreadWrongThreadStart);
 
             m_start_arg = parameter;
+            Start();
+        }
+
+        [UnsupportedOSPlatform("browser")]
+        internal void UnsafeStart()
+        {
             StartInternal(this);
         }
 
+        [UnsupportedOSPlatform("browser")]
+        internal void UnsafeStart(object parameter)
+        {
+            Debug.Assert(m_start is ThreadStart);
+
+            m_start_arg = parameter;
+            UnsafeStart();
+        }
+
         // Called from the runtime
         internal void StartCallback()
+        {
+            ExecutionContext? context = _executionContext;
+            _executionContext = null;
+            if (context != null && !context.IsDefault)
+            {
+                ExecutionContext.RunInternal(context, s_threadStartContextCallback, this);
+            }
+            else
+            {
+                StartCallbackWorker();
+            }
+        }
+
+        private static readonly ContextCallback s_threadStartContextCallback = new ContextCallback(StartCallback_Context);
+
+        private static void StartCallback_Context(object? state)
+        {
+            Debug.Assert(state is Thread);
+            ((Thread)state).StartCallbackWorker();
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)] // otherwise an unnecessary long-lived stack frame in many threads
+        private void StartCallbackWorker()
         {
             if (culture != null)
-                CurrentCulture = culture;
+            {
+                CultureInfo.CurrentCulture = culture;
+                culture = null;
+            }
+
             if (ui_culture != null)
-                CurrentUICulture = ui_culture;
+            {
+                CultureInfo.CurrentUICulture = ui_culture;
+                ui_culture = null;
+            }
+
             if (m_start is ThreadStart del)
             {
                 m_start = null;
                 del();
             }
             else
             {
+                Debug.Assert(m_start is ParameterizedThreadStart);
                 var pdel = (ParameterizedThreadStart)m_start!;
                 object? arg = m_start_arg;
                 m_start = null;