dotnet / dotnet/sdk

dotnetup: Mutex/async correctness in ScopedMutex and InstallerOrchestratorSingleton

Open
#53,744 0 comments 0 reactions 1 assignee Claimed by @nagilson View on GitHub
Area-dotnetup
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.