[NativeAOT] Cleanup and rationalizing process/thread termination scenario #80063

VSadov · 2022-12-30T06:17:06Z

It was noticed a while ago that while the current scheme works, it has some ad-hoc parts that may not be robust enough for broader set of scenarios such as terminating the process from multiple threads or by calling ExitProcess from native code, or in a context of a .dll

This PR tries to take care of that debt.
Fixes: #74711

unified CrstSuspendEE and threadstore lock into one CrstThreadStore
the locks had largerly overlapping purpose and one lock is better than two.
3 stage thread detach

call managed pre-mortem callbacks while still safe. (especially important on Unix as we need to release/signal wait subsystem locks)
remove ability to run managed code (this stage takes threadstore lock for thread removal and to make sure that GC cannot happen)
perform remaining cleanup of native data structures.

rationalized thread shutdown on Windows vs. non-Windows
on Windows the thread that initiates and performs the process shutdown, by calling native ExitProcess, processing CTRL-C or in few other ways is special. Blocking it will prevent process exiting. We detect this thread via DLL_PROCESS_DETACH (.dll) or atexit() (.exe) and remember to not detach this thread. We do not want to run managed code or take threadstore lock and risk blocking this thread when process may already be in inconsitent state.
other, not special, threads are allowed to do regular detach sequence. At process shutdown, if timing is tight, some such threads may be destroyed rudely or get into deadlocks. Not much can be done about that, but we do not care at that point.
on Unix there are no such special threads.
the purpose of PalAttachThread is to register with OS thread premortem notification. PalDetachThread just unregisters.
We use:
fiber detach notification (Windows)
pthread_setspecific (Linux, Android)
tls destructor (default)

In either case the notification comes without loader/OS locks held, so detaching is safe.
The only case where loader lock could be held is shutting down the special thread on Windows. One more reason to not detach that one.

addressed some TODOs and obvious dead code.

ghost · 2022-12-30T06:17:19Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

It was noticed a while ago that while the current scheme works, it has some ad-hoc parts that may not be robust enough for broader set of scenarios such as terminating the process from multiple threads or by calling ExitProcess from native code, or in a context of a .dll

This PR tries to take care of that debt.
Fixes: #74711

unified CrstSuspendEE and threadstore lock into one CrstThreadStore
the locks had largerly overlapping purpose and one lock is better than two.
3 stage thread detach

call managed pre-mortem callbacks while still safe. (especially important on Unix as we need to release or signal wait subsystem locks)
remove ability to run managed code (this takes threadstore lock for thread removal and to make sure that GC cannot happen)
perform native cleanup.

rationalized thread shutdown on Windows vs. non-Windows
on Windows the thread that initiates and performs the process shutdown, by calling native ExitProcess, processing CTRL-C or in few other ways is special. Blocking it will prevent process exiting. We detect this thread via DLL_PROCESS_DETACH (.dll) or atexit() (.exe) and remember to not detach this thread. We do not want to run managed code or take threadstore lock and risk blocking this thread when process may already be in inconsitent state.
other, not special, threads are allowed to do regular detach sequence. At process shutdown, if timing is tight, some such threads may be destroyed rudely or get into deadlocks. Not much can be done about that, but we do not care at that point.
on Unix there are no such special threads.
the purpose of PalAttachThread is to register with OS thread premortem notification. PalDetachThread just unregisters.
We use:
fiber detach notification (Windows)
pthread_setspecific (Linux, Android)
tls destructor (default)

In either case the notification comes without loader/OS locks held, so detaching is safe.
The only case where loader lock could be held is shutting down the special thread on Windows. One more reason to not detach that one.

addressed some TODOs and obvious dead code.

Author:	VSadov
Assignees:	VSadov
Labels:	`area-NativeAOT-coreclr`
Milestone:	-

src/coreclr/nativeaot/Bootstrap/main.cpp

…down)

jkotas · 2022-12-30T14:03:18Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-12-30T14:03:38Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2022-12-30T14:22:10Z

src/coreclr/nativeaot/Runtime/startup.cpp

-    // Indicate that runtime shutdown is complete and that the caller is about to start shutting down the entire process.
-    g_threadPerformingShutdown = ThreadStore::RawGetCurrentThread();
-}
-
 #ifdef _WIN32
 EXTERN_C UInt32_BOOL WINAPI RtuDllMain(HANDLE hPalInstance, uint32_t dwReason, void* pvReserved)


I do not see any place where this is used. Delete it?

I assumed RtuDllMain is used somehow when a NativeAOT dll is created. It is not used as-is.

I do not think it is used anywhere. It looks like a left-over from redhawk.

It looks like, at least in a case of .dll publishing, one would need to hookup InitDLL/UninitDLL, and on windows OnProcessExit.

Is that handled somehow or dynamic NativeAOT libraries are not supported?

I see we have a static lib sample, but no dlls.

I see we have a static lib sample, but no dlls.

The DLL sample is in the same location: https://github.com/dotnet/samples/tree/main/core/nativeaot/NativeLibrary#building-shared-libraries

Building DLLs is the only officially documented thing. Static lib is just the sample: https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/#build-native-libraries

Hmm I missed that, how does the .dll initialize the runtime? Calls RtuDllMain somehow, or calls RhInitialize on the first reverse pinvoke?
The latter should work (and perhaps more reliably) with this change.
Do we have a test?

I think we initialize on first reverse p/invoke. The test is here: https://github.com/dotnet/runtime/tree/main/src/tests/nativeaot/SmokeTests/SharedLibrary

src/coreclr/nativeaot/Runtime/threadstore.cpp

Co-authored-by: Jan Kotas <[email protected]>

jkotas · 2022-12-30T17:55:23Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-12-30T17:55:58Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2022-12-30T18:28:43Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-12-30T18:29:06Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas

LGTM once the runtime-extra-platforms tests pass.

VSadov · 2022-12-30T20:05:32Z

Build coreclr Pri0 Runtime Tests Run windows x86 checked failure looks like #75244

VSadov · 2022-12-30T22:52:11Z

There are some failures in Mono tests, but I do not think this PR shares anything with Mono. I see similar failures in #80035
NativeAOT tests have all passed.

VSadov · 2022-12-30T23:24:21Z

Thanks!!

VSadov added 8 commits December 27, 2022 11:26

remove m_pTEB

8bc902c

set g_threadPerformingShutdown in DllMain

0d1a6d3

more thread detach cleanup

0824a69

rationalizing DetachCurrentThread

5b7f73f

OnProcessExit

dd936b6

Remove g_SuspendEELock

ecbb0a4

fixes

6f40e54

comment tweak

792a5c2

VSadov requested a review from MichalStrehovsky as a code owner December 30, 2022 06:17

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Dec 30, 2022

ghost assigned VSadov Dec 30, 2022

VSadov commented Dec 30, 2022

View reviewed changes

src/coreclr/nativeaot/Bootstrap/main.cpp Show resolved Hide resolved

fix assert (thread could be detached before ending up performing shut…

33d6fde

…down)

jkotas reviewed Dec 30, 2022

View reviewed changes

src/coreclr/nativeaot/Runtime/threadstore.cpp Outdated Show resolved Hide resolved

jkotas reviewed Dec 30, 2022

View reviewed changes

src/coreclr/nativeaot/Runtime/threadstore.cpp Outdated Show resolved Hide resolved

jkotas reviewed Dec 30, 2022

View reviewed changes

src/coreclr/nativeaot/Runtime/threadstore.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

0096ff6

Co-authored-by: Jan Kotas <[email protected]>

remove unused RtuDllMain

cac113e

jkotas approved these changes Dec 30, 2022

View reviewed changes

build-analysis bot mentioned this pull request Dec 30, 2022

Test failure Loader\\classloader\\DictionaryExpansion\\DictionaryExpansion\\DictionaryExpansion.cmd #75244

Closed

VSadov merged commit 92e8eab into dotnet:main Dec 30, 2022

VSadov deleted the threading1 branch December 30, 2022 23:24

ghost locked as resolved and limited conversation to collaborators Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NativeAOT] Cleanup and rationalizing process/thread termination scenario #80063

[NativeAOT] Cleanup and rationalizing process/thread termination scenario #80063

VSadov commented Dec 30, 2022 •

edited

Loading

ghost commented Dec 30, 2022

jkotas commented Dec 30, 2022

azure-pipelines bot commented Dec 30, 2022

jkotas Dec 30, 2022

VSadov Dec 30, 2022

jkotas Dec 30, 2022

VSadov Dec 30, 2022

VSadov Dec 30, 2022

MichalStrehovsky Jan 1, 2023

VSadov Jan 2, 2023

MichalStrehovsky Jan 2, 2023

jkotas commented Dec 30, 2022

azure-pipelines bot commented Dec 30, 2022

jkotas commented Dec 30, 2022

azure-pipelines bot commented Dec 30, 2022

jkotas left a comment

VSadov commented Dec 30, 2022

VSadov commented Dec 30, 2022 •

edited

Loading

VSadov commented Dec 30, 2022

[NativeAOT] Cleanup and rationalizing process/thread termination scenario #80063

[NativeAOT] Cleanup and rationalizing process/thread termination scenario #80063

Conversation

VSadov commented Dec 30, 2022 • edited Loading

ghost commented Dec 30, 2022

jkotas commented Dec 30, 2022

azure-pipelines bot commented Dec 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Dec 30, 2022

azure-pipelines bot commented Dec 30, 2022

jkotas commented Dec 30, 2022

azure-pipelines bot commented Dec 30, 2022

jkotas left a comment

Choose a reason for hiding this comment

VSadov commented Dec 30, 2022

VSadov commented Dec 30, 2022 • edited Loading

VSadov commented Dec 30, 2022

VSadov commented Dec 30, 2022 •

edited

Loading

VSadov commented Dec 30, 2022 •

edited

Loading