dotnet / dotnet/msbuild

TaskAnalyzer: no diagnostic for unsynchronized shared mutable state in [MSBuildMultiThreadableTask] types

Open
#14,785 0 comments 0 reactions 0 assignees View on GitHub
Area: Analyzer Area: Multithreaded triaged
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.