Lazy init of ThreadWaitInfo in Thread.Mono.cs to prevent crash on external attached threads. #51483
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commit dotnet@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 constructur 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 indentical. After above commit this is no longer true, meaning that all managed thread ojbects 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.
vargaz
approved these changes
Apr 19, 2021
vargaz
reviewed
Apr 19, 2021
jkotas
reviewed
Apr 19, 2021
src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs
Outdated
Show resolved
Hide resolved
construct under CoreClr on x64 gives the following assembler on property access: lea rax,[rcx+8] mov rax,qword ptr [rax] test rax,rax jne 00007FFAD41908C1 call 00007FFAD41904D0 mov eax,3B9ACA00h Since its x64 fast path will be a lea/mov and conditional branch on property access, all inlined in caller.
jkotas
reviewed
Apr 19, 2021
src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Apr 19, 2021
src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs
Outdated
Show resolved
Hide resolved
jkotas
approved these changes
Apr 19, 2021
src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs
Outdated
Show resolved
Hide resolved
CoffeeFlux
approved these changes
Apr 19, 2021
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 thread 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.
//CC @CoffeeFlux