Publish the stack-frame regex once instead of racing on lazy init - #10702
Publish the stack-frame regex once instead of racing on lazy init#10702Jakub Jareš (nohwnd) wants to merge 2 commits into
Conversation
GetFrameRegex could hand a different Regex to each concurrent caller on targets that don't use GeneratedRegex. The check and the assign were not atomic, and the method returned the field instead of the instance it had just built, so two threads racing on first use each got their own object and the second write replaced the first. Publish the first instance with Interlocked.CompareExchange and return it, so every caller shares one regex and the RegexOptions.Compiled construction is paid for once. The regex is still built lazily on first use. The MSBuild copy had the same shape and is fixed the same way. 🤖
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
✅ 22/22 dimensions clean — no findings.
The Volatile.Read + Interlocked.CompareExchange pattern is the correct lock-free lazy initialization idiom. The return expression Interlocked.CompareExchange(ref s_regex, regex, null) ?? regex correctly returns the winning instance regardless of which thread publishes first. The concurrency test adequately exercises the race window with Parallel.For across 32 lanes.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Atomically publishes one shared stack-frame regex across concurrent callers on non-generated-regex targets.
Changes:
- Uses volatile reads and
Interlocked.CompareExchangefor regex publication. - Applies the fix to platform and MSBuild helpers.
- Adds a concurrent-caller regression test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
StackTraceRegexHelperTests.cs |
Adds concurrency coverage. |
StackTraceHelper.cs |
Makes platform regex publication atomic. |
Microsoft.Testing.Platform.MSBuild/Tasks/StackTraceHelper.cs |
Applies equivalent MSBuild fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Racing callers on first use do each build a Regex, only the published one survives, so saying the construction is paid for once was wrong. The saving is that every call after publication skips it. 🤖
🧵 Parallel-safety audit — PR #10702Parallelization — test assembly audited:
Findings: A (global-state) The only changed test is a new method,
No unsafe call sites, no near-misses, no over-serialization. This is a case where a test was deliberately written to be safe under Advisory only — heuristic, non-blocking. Re-run with
|
🧪 Expert test review — PR #10702No new or modified test methods were identified in the changed regions Re-run with
|
| // assembly already initialized the regex, every caller here takes the fast path and the test passes trivially. | ||
| var instances = new Regex[32]; | ||
|
|
||
| Parallel.For(0, instances.Length, i => instances[i] = StackTraceHelper.GetFrameRegex()); |
There was a problem hiding this comment.
This does not reliably exercise the cold-start race. In a normal assembly run, another test or a product call can initialize s_regex first, so all 32 iterations take the fast path and this test still passes if the CompareExchange fix is later reverted. Even on a cold run, Parallel.For is allowed to execute the iterations serially and does not guarantee that callers overlap.
Could we make the regression deterministic? One option is to move the publication logic behind a helper that accepts a fresh storage location and coordinate callers with a Barrier, then assert that every caller receives the same published instance. Alternatively, using LazyInitializer.EnsureInitialized would remove the custom publication algorithm and make this test unnecessary. Resetting the production static through reflection would be less desirable because this assembly runs tests in parallel and other platform code can use the same field.
|
|
||
| // Racing callers on first use may each build an instance. Publish the first one and return it to all of them, | ||
| // so every caller observes the same object, and once it is published no caller builds the regex again. | ||
| return Interlocked.CompareExchange(ref s_regex, regex, null) ?? regex; |
There was a problem hiding this comment.
The same publication behavior is changing in the MSBuild copy, but its existing test only verifies that s_regex becomes non-null and has the expected timeout; it would not catch callers receiving different instances during first use. Could this implementation share the tested initialization primitive with the platform helper, or gain equivalent deterministic concurrency coverage? Otherwise a future edit can regress one copy while tests continue to cover only the other.
GetFrameRegex()could hand a differentRegexto each concurrent caller on targets that don't use[GeneratedRegex]. The check and the assign were not atomic, and the method returned the field instead of the instance it had just built, so two threads racing on first use each got their own object and the second write replaced the first. That is what madeGetFrameRegex_HasExplicitCapturesAndNoTimeoutfail on net462 in CI.Publish the first instance with
Interlocked.CompareExchangeand return it, so every caller gets the same regex. Callers racing on the very first use still each build one, only the published instance survives, and every call after that skips the construction. The regex is still built lazily on first use. The copy inMicrosoft.Testing.Platform.MSBuildhad the same shape and is fixed the same way.Verified: the new
GetFrameRegex_ConcurrentCallers_ShareASingleInstancetest fails 19 out of 20 runs against the old code and passes 20 out of 20 against the new one, run on its own so the regex is still cold. Ten consecutive full net462 runs ofMicrosoft.Testing.Platform.UnitTestsare green, andbuild.cmd -c Releasepasses.🤖