Skip to content

Commit

Permalink
Lazy init of ThreadWaitInfo in Thread.Mono.cs to prevent crash on ext…
Browse files Browse the repository at this point in the history
…ernal attached threads. (#51483)

* Init ThreadWaitInfo to prevent crash on external attached threads.

Commit 457f58c
added a new managed object into Mono's managed thread object, _waitInfo.

This new object was initialized in Thread managed constructor, problem
with that is for all threads that gets their thread object created in
native code, create_thread_object, that code did the same thing as
constructor before commit, since managed constructor called down into
native code calling the same native method as create_thread_object meaning
a thread object created from managed or native would be initialized
identical.

After above commit this is no longer true, meaning that all managed thread
objects created in native code (like attached external threads) won't
get _waitInfo initialized and that leads to a crash when OnThreadExiting
gets called from Finalizer thread.

Fix is to make sure _waitInfo get initialize regardless of thread object
gets created from managed or native code.

This fix implements a lazy init as part of property access, I tested a
native version of the fix, calling back into managed to init managed
parts of thread when tread gets created in native, but turns out that
such a fix gets a little complicated, since some of the threads are
created early when it's not possible to run managed code, so that in turn
needed additional changes into the init order, that in turn has bigger
impact and could cause other unwanted side effects.

Fixing it in managed using lazy/on demand init is simpler/cleaner approach
but might come with a small performance hit for callers of WaitInfo property
(will add volatile read and conditional branch), but looking at callers
of the property, they appear in functions with rather high "cost",
like Sleep, NewMutex, Wait, SignalAndWait, Interrupt and
RelinquishOwnership.
  • Loading branch information
lateralusX committed Apr 21, 2021
1 parent f1a9ad1 commit 65bcece
Showing 1 changed file with 14 additions and 18 deletions.
32 changes: 14 additions & 18 deletions src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public partial class Thread
internal ExecutionContext? _executionContext;
internal SynchronizationContext? _synchronizationContext;
#if TARGET_UNIX || TARGET_BROWSER
internal WaitSubsystem.ThreadWaitInfo _waitInfo;
internal WaitSubsystem.ThreadWaitInfo? _waitInfo;
#endif

// This is used for a quick check on thread pool threads after running a work item to determine if the name, background
Expand Down Expand Up @@ -138,9 +138,20 @@ internal static int OptimalMaxSpinWaitsPerSpinIteration
return 7;
}
}

#if TARGET_UNIX || TARGET_BROWSER
internal WaitSubsystem.ThreadWaitInfo WaitInfo => _waitInfo;
internal WaitSubsystem.ThreadWaitInfo WaitInfo
{
get
{
return Volatile.Read(ref _waitInfo) ?? AllocateWaitInfo();

WaitSubsystem.ThreadWaitInfo AllocateWaitInfo()
{
Interlocked.CompareExchange(ref _waitInfo, new WaitSubsystem.ThreadWaitInfo(this), null!);
return _waitInfo;
}
}
}
#endif

public ThreadPriority Priority
Expand Down Expand Up @@ -199,25 +210,13 @@ public bool Join(int millisecondsTimeout)
}

#if TARGET_UNIX || TARGET_BROWSER
[MemberNotNull(nameof(_waitInfo))]
[DynamicDependency(nameof(OnThreadExiting))]
#endif
private void Initialize()
{
InitInternal(this);
#if TARGET_UNIX || TARGET_BROWSER
_waitInfo = new WaitSubsystem.ThreadWaitInfo(this);
#endif
}

#if TARGET_UNIX || TARGET_BROWSER
private void EnsureWaitInfo()
{
if (_waitInfo == null)
_waitInfo = new WaitSubsystem.ThreadWaitInfo(this);
}
#endif

public static void SpinWait(int iterations)
{
if (iterations < 0)
Expand Down Expand Up @@ -321,9 +320,6 @@ private static void OnThreadExiting(Thread thread)
private static Thread InitializeCurrentThread()
{
var current = GetCurrentThread();
#if TARGET_UNIX || TARGET_BROWSER
current.EnsureWaitInfo();
#endif
t_currentThread = current;
return t_currentThread;
}
Expand Down

0 comments on commit 65bcece

Please sign in to comment.