Use FLS detach as thread termination notification on windows. #110589
Conversation
src/coreclr/vm/ceemain.cpp
Outdated
| if (threadFromCurrentFiber != NULL) | ||
| { | ||
| _ASSERTE(!"Multiple threads encountered from a single fiber"); | ||
| COMPlusThrowWin32(); |
There was a problem hiding this comment.
This should be fail fast or ASSERTE_ALL_BUILDS. I do think we want to run any exception handling here.
There was a problem hiding this comment.
ASSERTE_ALL_BUILDS would be the right choice if it did what the name implies, but is it just a regular assert?
There was a problem hiding this comment.
Actually it looks like ASSERTE_ALL_BUILDS is a failfast in CoreClr. It is in the NativeAOT where it is just a regular debug-only assert.
There was a problem hiding this comment.
I saw the pattern used in threads.cpp. So I figured it might be the convention for this kind of code.
runtime/src/coreclr/vm/threads.cpp
Lines 1051 to 1055 in ca5cf41
runtime/src/coreclr/vm/threads.cpp
Lines 1138 to 1139 in ca5cf41
. . . bunch of other cases . . .
I do agree that _ASSERTE_ALL_BUILDS intuitively seems more appropriate.
There was a problem hiding this comment.
It is a convention to throw an exception when there is a (transient) error during runtime initialization.
This case is different - it is not runtime initialization: It means that something is really wrong, e.g. the process uses fibers that .NET runtime does not support.
There was a problem hiding this comment.
Right. In this case these are situations that we cannot recover from, so should be a failfast or an equivalent.
|
I suspect |
src/coreclr/vm/ceemain.cpp
Outdated
| // thread - thread to detach | ||
| // Return: | ||
| // true if the thread was detached, false if there was no attached thread | ||
| bool OsDetachThread(void* thread) |
There was a problem hiding this comment.
We never check the return value. Should it return void?
There was a problem hiding this comment.
This came from NativeAOT. I just mostly wanted to keep code similar.
In NativeAOT returning false indicates that the thread was initialized (i.e the Thread object that is a C++ TLS object has meaningful data and it is m_ThreadStateFlags != TSF_Unknown), but is not attached (i.e. not inserted in the thread store - basically not a managed thread yet). Returning false here means that the rest of detachment steps like removing from threadstore are unnecessary.
I am not sure the state is real. Once we initialize the thread we immediately try to attach it to the threadstore, but these are distinct states that we can handle in detach.
On CoreCLR the call to SetThread(NULL) seems to be more ad-hoc. It is called from 5 different places. It seems more like "just unsubscribe from OS notifications". Noone checks the result.
SetThread(NULL) itself would clear appdomain regardless of whether the thread was attached to the runtime or not.
Yes, I think we could return void. We evidently do not check the result.
|
The win-x86 failure appears to be #110285 |
|
Thanks! |
|
/backport to release/9.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/12285157933 |
…#110589) * Use FLS detach as thread termination notification on windows. * use _ASSERTE_ALL_BUILDS * one more case * InitFlsSlot throws per convention used in threading initialization * OsDetachThread could be void * Update src/coreclr/vm/ceemain.cpp --------- Co-authored-by: Jan Kotas <[email protected]>
|
I am sorry - I have to revert this to unblock WinForms codeflow (#110801). |
Fixes: #110350
Switches the mechanism for OS notification about thread termination to use FLS (Fiber Local Storage) - the same mechanism as used on NativeAOT.
The advantage of FLS notification is that it does not run while holding the Loader Lock, thus it does not require that the termination callback never calls or waits for an OS call that may need to acquire that same Loader Lock. (It is hard for us to guarantee the latter)