TaskAnalyzer: no diagnostic for unsynchronized shared mutable state in [MSBuildMultiThreadableTask] types
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
### Issue Description
`[MSBuildMultiThreadableTask]` is an assertion by the task author that the task is safe to run concurrently with other tasks in the same process. The analyzer verifies the *ambient process state* half of that contract (environment, CWD, `Process.Start`, temp files), but nothing verifies the *shared mutable state* half.
Unsynchronized static state is not mentioned anywhere in #14772. It is, however, the failure mode that produces genuinely nondeterministic corruption rather than a deterministic wrong answer, and it caused three real races in the Arcade migration ([dotnet/arcade#17381](https://github.com/dotnet/arcade/pull/17381)).
### Steps to Reproduce
```csharp
[MSBuildMultiThreadableTask]
public sealed class SharedStateTask : Task, IMultiThreadableTask
{
public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;
private static readonly Dictionary s_cache = new Dictionary();
private static readonly Random s_random = new Random();
private static HttpClient s_client;
public override bool Execute()
{
s_cache["k"] = "v"; // unsynchronized write to a static Dictionary
s_client = new HttpClient(); // overwrites a process-wide field
Log.LogMessage(s_random.Next().ToString() + s_cache.Count + s_client.Timeout);
return true;
}
}
```
Build with `msbuild_task_analyzer.scope = multithreadable_only` and `--no-incremental`.
### Expected Behavior
At minimum a diagnostic on the `s_cache["k"] = "v"` write: mutating a non-thread-safe static collection from a type asserted to be multithreadable. Ideally also on the static field assignment and on `Random`, which is documented as not thread-safe for instance members.
### Actual Behavior
No diagnostic. The only thing reported for this type is `MSBuildTask0011` (constructor injection).
### Real-World Cases
All three were found by manual review, not by the analyzer, and all three are in code that the analyzer otherwise reported as clean:
1. **`TargetFrameworkResolver.CreateOrGet`** — a `static readonly Dictionary` used as a memoization cache, read and written with no synchronization. Concurrent `Execute` calls can tear the bucket array. Fixed by switching to `ConcurrentDictionary.GetOrAdd`.
2. **`GetPackageDescription`** — shared a non-thread-safe NuGet object across invocations via a cached index.
3. **`GetCompatiblePackageTargetFrameworks`** — same shape.
Case 1 is representative of a very common idiom in build tasks: a `static` memoization cache added at some point for performance, in code that was single-threaded-by-construction for its entire life until the attribute was applied.
### Suggested Fix
A new diagnostic, reported only on types carrying `[MSBuildMultiThreadableTask]` (so it is opt-in and cannot regress existing task code), covering:
1. Writes to `static` fields, and mutating calls on `static` fields of known non-thread-safe types (`Dictionary<,>`, `List<>`, `HashSet<>`, `StringBuilder`, `Random`, `XmlDocument`, …), when not obviously guarded by a `lock` in the same method.
2. Writes to `static` fields of any type from within `Execute` or anything it reaches.
3. Optionally, a "possibly-shared state" informational diagnostic on `static` fields of types from referenced assemblies, which cannot be judged locally.
Suppression is easy for the intentional cases (`lock`, `Lazy`, `ConcurrentDictionary`, `Interlocked`, immutable collections, `readonly` value types), so the rule should be tractable without excessive noise.
Even a limited version restricted to point 1 — mutating a static `Dictionary`/`List`/`HashSet` outside a `lock` — would have caught all three Arcade cases.
This may be considered out of scope for a *task-authoring* analyzer, since it is a general concurrency concern. Filing it because the attribute is exactly the signal that makes the rule cheap and precise: it tells the analyzer which types the author has promised are concurrency-safe, and therefore which types are worth checking.
### Versions & Configurations
- `Microsoft.Build.TaskAuthoring.Analyzer` `18.11.0-1.26420.118`
- `Microsoft.Build.Utilities.Core` `18.8.2`
- SDK `11.0.0-preview.6.26359.118`
### Related
- #14772, #14779, #14780
- [dotnet/arcade#17378](https://github.com/dotnet/arcade/issues/17378), [dotnet/arcade#17381](https://github.com/dotnet/arcade/pull/17381)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reading the TaskAnalyzer handling for [MSBuildMultiThreadableTask] types and the existing MSBuildTask0011 diagnostic, then reproduce the issue with msbuild_task_analyzer.scope = multithreadable_only and --no-incremental. Done means the analyzer reports the specified unsynchronized static-state cases while allowing the listed guarded or concurrent-safe patterns.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system, tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100