-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Move CoreCLR over to the managed wait subsystem #117788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @mangod9 |
25446d0
to
63dc9af
Compare
06de3b2
to
63a1aa6
Compare
// Declared nullable even though it is initialized in Create | ||
// as Roslyn doesn't see that it's set in Create and Create is called from all constructors. | ||
private WaitSubsystem.WaitableObject? _semaphore; | ||
|
||
private void Create(int maximumSignalCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use [MemberNotNull(nameof(_semaphore))]
?
63a1aa6
to
2b610bf
Compare
src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetPageSize.cs
Outdated
Show resolved
Hide resolved
4a27a27
to
ac08ce6
Compare
// as we're running on the finalizer thread right now. | ||
PREPARE_NONVIRTUAL_CALLSITE(METHOD__THREAD__ON_THREAD_EXITING); | ||
DECLARE_ARGHOLDER_ARRAY(args, 1); | ||
args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(((Thread*)iter.GetElement())->GetExposedObject()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's keeping the Threads in the temporary list alive?
(The lifetime management of the native Threads is a mess, see e.g. #107473.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These managed thread objects can't be GCd until they are finalized, and (from what I can tell) they can't be put on the finalizer queue until after the native thread is marked as detached.
Since we're doing this processing on the finalizer thread after detaching the native threads but before processing any additional objects on the finalizer queue, they can't have been finalized yet. At worst they're currently on the finalizer queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the native Thread objects, those are kept alive until after this function runs after the managed thread object has been finalized. As we can't run the managed thread object's finalizer while we're in this function as we're on the finalizer thread, we know that the native thread object hasn't been deleted yet.
8838c10
to
9af82b0
Compare
@dotnet/samsung Could you please take a look? These changes may be related to riscv64. |
…nents of mutexes from the CoreCLR PAL.
…as already hardened the cases that this makes weird, so we can reliably do it with CoreCLR
…he managed thread object is finalized.
… wait" worker as the only handles it gets on Unix are PAL handles, not wait subsystem handles (and for Windows, we don't need to call back for handles that come through here)
9af82b0
to
b18871c
Compare
Use the shared managed wait subsystem for CoreCLR's managed code instead of the Win32 PAL
Also, remove the named mutex support from the CoreCLR PAL as nothing uses it now.
TODO: Once #117688 is in, we can remove Mutex support from the Win32 PAL entirely
Unblocks #115685
Depends on #117635