TaskAnalyzer: no diagnostic for the non-atomic GetRegisteredTaskObject/RegisterTaskObject cache pattern
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
### Summary
`IBuildEngine4.GetRegisteredTaskObject` / `RegisterTaskObject` is a natural caching idiom, and the read/write pair is not atomic. Under multithreaded execution two instances of the same task in one node can both miss and both populate. Whether that is benign depends entirely on what the cached computation does, and there is no diagnostic prompting the author to make that call.
This is distinct from #14785, which covers `static` mutable fields. Here the shared state lives in the engine, so no static field appears and nothing looks suspicious.
### Evidence
Three tasks in dotnet/arcade use the pattern, and they land on opposite sides of the line.
**Benign** — `LocateDotNet` and `CheckRequiredDotNetVersion`. The cached computation is pure and deterministic for a given key, so the loser of the race overwrites an identical entry:
```csharp
var cachedResult = (CacheEntry)BuildEngine4.GetRegisteredTaskObject(s_cacheKey, RegisteredTaskObjectLifetime.Build);
...
BuildEngine4.RegisterTaskObject(s_cacheKey, new CacheEntry(...), RegisteredTaskObjectLifetime.Build, allowEarlyCollection: true);
```
(`CheckRequiredDotNetVersion` still degrades slightly: deduplicating error reporting is part of what the cache buys, so a failing check can log twice.)
**Not benign** — `SingleError`, whose entire purpose is to emit exactly one error for a build. The same race lets two instances both observe "not yet reported" and both log, defeating the task's only reason to exist. We removed `[MSBuildMultiThreadableTask]` from it rather than ship that.
Identical API shape; opposite conclusions. That is precisely the situation where a diagnostic earns its keep.
### Proposed rule
In an annotated task, flag a `GetRegisteredTaskObject` whose result feeds a conditional that guards a later `RegisterTaskObject` with the same key.
Severity: Info, or Warning behind opt-in. The point is not that the pattern is wrong — it is that it is a decision the author must consciously make, and today nothing asks them to.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by locating the analyzer entry point for annotated tasks and any existing diagnostic tests covering GetRegisteredTaskObject or RegisterTaskObject. Use the proposed rule and the LocateDotNet, CheckRequiredDotNetVersion, and SingleError examples as behavioral references; done means the matching conditional cache pattern produces the intended diagnostic without treating every cache use as an error.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100