Support in-process named mutexes in managed implementation #55199
Conversation
|
Tagging subscribers to this area: @mangod9 Issue Details
This follows the suggestion in #48720 (comment) Note that the current managed Mutex implementation considers a mutex abandoned not only if the thread holding it terminates, but also when the SafeEventHandle refering to it is closed. This is clearly not correct for named mutexes, which can have multiple handles refering to it. In this patch, I've chosen to consider a named mutex abandoned once all handles to it are closed -- an alternate approach would be to just not have that special case at all and only abandon the mutex on thread exit (because as long as the mutex is held, it would in principle be possible to re-open another handle to it by name). Let me know if that alternate approach would be preferable.
|
|
I see some of the runs show failures in System.Threading.Tests.MutexTests.AbandonExisting, this is most likely this problem: #55198 which should be merged first (or if you prefer merged into this PR). |
src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/Mutex.Unix.cs
Outdated
Show resolved
Hide resolved
Is the SDK going to work reliably with this partial implementation? |
This is a good question, and I do not know a definite answer to that. What I can say is:
I guess a full solution would be preferable, but that seems a much larger effort, and I'm not even sure how to do that in a pure managed implementation. Unless we want to link the Mono runtime also against the PAL layer? |
Ok. So this makes it as broken as it always was.
I do not see why it would be needed. src/libraries/native PAL layer can be extended as necessary, without coupling it with the Mono runtime. |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@jkotas Do you know if |
It is coupled with the rest of the CoreCLR PAL. I do not think we would want to move it over wholesale, instead cherry pick just the pieces relevant for the shared mutex implementation. It should not be much. |
|
I think a managed in-proc implementation is still useful - it's a better fit for WASM and iOS+Android. We don't get a lot of freedom to launch new processes on mobile, and in any case relying on shared FS access is probably the wrong thing. |
| @@ -24,7 +29,9 @@ private void CreateMutexCore(bool initiallyOwned, string? name, out bool created | |||
|
|
|||
| private static OpenExistingResult OpenExistingWorker(string name, out Mutex? result) | |||
| { | |||
| throw new PlatformNotSupportedException(SR.PlatformNotSupported_NamedSynchronizationPrimitives); | |||
| OpenExistingResult status = WaitSubsystem.OpenNamedMutex(name, out var safeWaitHandle); | |||
There was a problem hiding this comment.
| OpenExistingResult status = WaitSubsystem.OpenNamedMutex(name, out var safeWaitHandle); | |
| OpenExistingResult status = WaitSubsystem.OpenNamedMutex(name, out SafeWaitHandle? safeWaitHandle); |
There was a problem hiding this comment.
Sorry, I missed those. Now fixed.
src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.WaitableObject.Unix.cs
Outdated
Show resolved
Hide resolved
| /// Dictionary to look up named waitable objects. This implementation only supports in-process | ||
| /// named waitable objects. Currently only named mutexes are supported. | ||
| /// </summary> | ||
| private static readonly Dictionary<string, WaitableObject> s_namedObjects = new Dictionary<string, WaitableObject>(); |
There was a problem hiding this comment.
Create it on demand in the one place that does Add so that the Dictionary won't be created in apps that do not used named mutexes?
src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.WaitableObject.Unix.cs
Outdated
Show resolved
Hide resolved
* Partially addresses #48720 * Use a dictionary to perform the name to object lookup * Allow multiple handles to refer to a single waitable object * Abandon mutex only when all handles refering to it are closed * Re-enable test cases disabled due to the above issue
ghost
locked as resolved and limited conversation to collaborators
Partially addresses Support inter-process named mutexes in managed Mutex implementation #48720
Use a dictionary to perform the name to object lookup
Allow multiple handles to refer to a single waitable object
Abandon mutex only when all handles refering to it are closed
Re-enable test cases disabled due to the above issue
This follows the suggestion in #48720 (comment)
Note that the current managed Mutex implementation considers a mutex abandoned not only if the thread holding it terminates, but also when the SafeEventHandle refering to it is closed. This is clearly not correct for named mutexes, which can have multiple handles refering to it. In this patch, I've chosen to consider a named mutex abandoned once all handles to it are closed -- an alternate approach would be to just not have that special case at all and only abandon the mutex on thread exit (because as long as the mutex is held, it would in principle be possible to re-open another handle to it by name). Let me know if that alternate approach would be preferable.