dotnetup: Mutex/async correctness in ScopedMutex and InstallerOrchestratorSingleton
- Dominant language
- C#
- Stars
- 3.2k
- Forks
- 1.3k
- PR merge metrics
- PR metrics pending
Description
## Context
PR [#53464](https://github.com/dotnet/sdk/pull/53464) review identified that `ScopedMutex` does not work correctly with async code. In theory, (not confirmed) mutexes in .NET may not be acquired and released on different threads, which may break across `async`/`await` boundaries.
### Related PR Comments
- [InstallerOrchestratorSingleton.cs L39](https://github.com/dotnet/sdk/pull/53464#discussion_r3042474917) — @dsplaisted: "Mucking with the mutex like this feels dangerous to me."
- @nagilson: "Let's leave this (the callback only sends a message) but track it in an issue."
- [InstallerOrchestratorSingleton.cs L100](https://github.com/dotnet/sdk/pull/53464#discussion_r3042505540) — @dsplaisted: "Mutexes don't work across async. Our ScopedMutex class is just broken."
- @nagilson: "Let's leave this and file an issue to consider."
- [ScopedMutex.cs L34](https://github.com/dotnet/sdk/pull/53464#discussion_r3046058624) — @dsplaisted: "Doesn't `s_processActiveHolds` mean we shouldn't need `SuppressWaitingCallback`?"
## Description
The `ScopedMutex` class uses `System.Threading.Mutex`, which has thread affinity — the thread that acquires the mutex must be the same thread that releases it. In async code paths, `await` may resume on a different thread, causing:
1. `AbandonedMutexException` or undefined behavior on release
2. `SuppressWaitingCallback` used as a workaround via static state, which is fragile
Additionally, `InstallerOrchestratorSingleton` modifies mutex behavior through static state (`SuppressWaitingCallback`), which is error-prone.
## Acceptance Criteria
- [ ] Investigate whether `SemaphoreSlim` or a file-based lock is more appropriate for cross-process async synchronization, note it has to work inter / cross process and intraprocess
- [ ] Remove the need for `SuppressWaitingCallback` static state
- [ ] Ensure `s_processActiveHolds` correctly prevents redundant locking within the same process
- [ ] Consult with a concurrency expert on the correct locking pattern for dotnetup's use case
- [ ] Add tests that exercise concurrent install scenarios
## Relevant Code
- `src/Installer/Microsoft.Dotnet.Installation/Internal/ScopedMutex.cs`
- `src/Installer/dotnetup/InstallerOrchestratorSingleton.cs`
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.