Conversation
…l tests Remove StringBuilder overrides from OptimizedConsoleInterceptor. The overrides called ToString() which traverses the entire internal chunk linked list — if the caller's StringBuilder is mutated concurrently, ToString() throws ArgumentOutOfRangeException(chunkLength). The base TextWriter implementation iterates chunks via GetChunks() and copies each one independently via Write(ReadOnlySpan<char>), which the interceptor already overrides. Also move StringBuilder.Length checks inside the ReaderWriterLockSlim in Context.GetStandardOutput/GetErrorOutput — Length is not thread-safe. Fixes #5411
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Code Review
This PR addresses a real race condition and is generally well-reasoned. Here are my findings:
Correctness of the fix
Context.cs — moving .Length check inside the lock
The change in GetStandardOutput() / GetErrorOutput() is correct. StringBuilder.Length is not documented as thread-safe when accessed concurrently with mutations, so reading it outside the ReaderWriterLockSlim was a data race. Moving the check inside the read lock is the right fix. The ternary form is slightly more compact than before, which is fine.
OptimizedConsoleInterceptor.cs — removing StringBuilder overrides
The root cause diagnosis is accurate: ASP.NET Core's console logger passes a pooled StringBuilder via TextWriter.Write(StringBuilder), and after the call returns the StringBuilder may be recycled. Calling ToString() on it after the caller has logically released it opens a window for the chunk linked-list to be mutated mid-traversal, causing the ArgumentOutOfRangeException.
Falling back to the base TextWriter implementation is a clean way to avoid the problem — the base implementation uses GetChunks() rather than ToString(), so it never takes a snapshot of the whole buffer atomically.
Subtle concern worth noting
The base TextWriter.Write(StringBuilder) iterates via GetChunks() and calls Write(ReadOnlySpan<char>) per chunk:
// .NET runtime TextWriter
public virtual void Write(StringBuilder? value)
{
if (value != null)
{
foreach (ReadOnlyMemory<char> chunk in value.GetChunks())
Write(chunk.Span);
}
}OptimizedConsoleInterceptor.Write(ReadOnlySpan<char>) materialises a new string(buffer) and calls Write(string). So each chunk becomes a separate, independent Write(string) call into the line buffer — the chunks are written non-atomically relative to each other. For the described scenario (ASP.NET Core's pooled logger writing a single log line), this is benign: the chunks are still written in order and the ConsoleLineBuffer is itself locked. However, if a caller mutates the StringBuilder mid-iteration (between chunks), the emitted content could be garbled (e.g., some chunks from the old content, some from the new). This is no worse than what any other TextWriter (such as StreamWriter) would produce in the same situation, and it is strictly better than ArgumentOutOfRangeException. Worth documenting explicitly in the comment or issue, even if no code change is needed.
Similarly, the async WriteAsyncCore path awaits between chunks, so chunks from interleaved writers can mix. Again, same behaviour as StreamWriter — acceptable.
ConcurrentStringWriter — related concern, not addressed by this PR
ConcurrentStringWriter (in Context.cs) is also a TextWriter subclass. It does not override Write(ReadOnlySpan<char>), so the base TextWriter will handle it by calling Write(char value) in a loop — one acquire/release of the write lock per character. This is thread-safe but potentially slow for large spans. Not a regression from this PR, but a pre-existing inefficiency that could be improved independently.
Testing
The new ContextThreadSafetyTests exercises concurrent reads and writes on OutputWriter/ErrorOutputWriter and verifies that no exception is thrown. This directly covers the Context.cs lock-ordering fix.
The interceptor fix itself is harder to reproduce in a unit test (it requires a pooled StringBuilder from an external logger under high concurrency). The PR note acknowledges this and relies on CI. That is reasonable.
One enhancement worth considering: add a test that passes a new StringBuilder("hello") to Console.Write(StringBuilder) while the interceptor is active, to at least verify the fall-through path produces the correct output at the unit-test level.
Summary
| Area | Verdict |
|---|---|
| Root cause analysis | Accurate |
Context.cs lock fix |
Correct and necessary |
Removing StringBuilder overrides |
Correct; non-atomic chunk iteration is an acceptable trade-off identical to StreamWriter's behaviour |
| Comment quality | Good — the #if NET block comment explains the intent |
| Tests | Adequate for the locking fix; interceptor fix relies on CI |
Overall this is a good, targeted fix. The approach of deferring to the base TextWriter's chunk-iteration strategy is the right call and matches the pattern used by the BCL's own StreamWriter. No blocking issues; the subtle chunk-interleaving note above is informational rather than a request to change the implementation.
Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.23.7 to 1.28.7. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.28.7 <!-- Release notes generated using configuration in .github/release.yml at v1.28.7 --> ## What's Changed ### Other Changes * fix: prevent StringBuilder race in console interceptor during parallel tests by @thomhurst in thomhurst/TUnit#5414 ### Dependencies * chore(deps): update tunit to 1.28.5 by @thomhurst in thomhurst/TUnit#5415 **Full Changelog**: thomhurst/TUnit@v1.28.5...v1.28.7 ## 1.28.5 <!-- Release notes generated using configuration in .github/release.yml at v1.28.5 --> ## What's Changed ### Other Changes * perf: eliminate redundant builds in CI pipeline by @thomhurst in thomhurst/TUnit#5405 * perf: eliminate store.ToArray() allocation on mock behavior execution hot path by @thomhurst in thomhurst/TUnit#5409 * fix: omit non-class/struct constraints on explicit interface mock implementations by @thomhurst in thomhurst/TUnit#5413 ### Dependencies * chore(deps): update tunit to 1.28.0 by @thomhurst in thomhurst/TUnit#5406 **Full Changelog**: thomhurst/TUnit@v1.28.0...v1.28.5 ## 1.28.0 <!-- Release notes generated using configuration in .github/release.yml at v1.28.0 --> ## What's Changed ### Other Changes * fix: resolve build warnings in solution by @thomhurst in thomhurst/TUnit#5386 * Perf: Optimize MockEngine hot paths (~30-42% faster) by @thomhurst in thomhurst/TUnit#5391 * Move Playwright install into pipeline module by @thomhurst in thomhurst/TUnit#5390 * perf: optimize solution build performance by @thomhurst in thomhurst/TUnit#5393 * perf: defer per-class JIT via lazy test registration + parallel resolution by @thomhurst in thomhurst/TUnit#5395 * Perf: Generate typed HandleCall<T1,...> overloads to eliminate argument boxing by @thomhurst in thomhurst/TUnit#5399 * perf: filter generated attributes to TUnit-related types only by @thomhurst in thomhurst/TUnit#5402 * fix: generate valid mock class names for generic interfaces with non-built-in type args by @thomhurst in thomhurst/TUnit#5404 ### Dependencies * chore(deps): update tunit to 1.27.0 by @thomhurst in thomhurst/TUnit#5392 * chore(deps): update dependency path-to-regexp to v8 by @thomhurst in thomhurst/TUnit#5378 **Full Changelog**: thomhurst/TUnit@v1.27.0...v1.28.0 ## 1.27.0 <!-- Release notes generated using configuration in .github/release.yml at v1.27.0 --> ## What's Changed ### Other Changes * Fix Dependabot security vulnerabilities in docs site by @thomhurst in thomhurst/TUnit#5372 * fix: use 0.0.0-scrubbed sentinel version in snapshot scrubber to avoid false Dependabot alerts by @thomhurst in thomhurst/TUnit#5374 * Speed up Engine.Tests by removing ProcessorCount parallelism cap by @thomhurst in thomhurst/TUnit#5379 * ci: add concurrency groups to cancel redundant workflow runs by @thomhurst in thomhurst/TUnit#5373 * Add scope-aware initialization and disposal OpenTelemetry spans to trace timeline and HTML report by @Copilot in thomhurst/TUnit#5339 * Add WithInnerExceptions() for fluent AggregateException assertion chaining by @thomhurst in thomhurst/TUnit#5380 * Drop net6.0 and net7.0 TFMs, keep net8.0+ and netstandard2.x by @thomhurst in thomhurst/TUnit#5387 * Remove all [Obsolete] members and migrate callers by @thomhurst in thomhurst/TUnit#5384 * Add AssertionResult.Failed overload that accepts an Exception by @thomhurst in thomhurst/TUnit#5388 ### Dependencies * chore(deps): update dependency mockolate to 2.3.0 by @thomhurst in thomhurst/TUnit#5370 * chore(deps): update tunit to 1.25.0 by @thomhurst in thomhurst/TUnit#5371 * chore(deps): update dependency minimatch to v9.0.9 by @thomhurst in thomhurst/TUnit#5375 * chore(deps): update dependency path-to-regexp to v0.2.5 by @thomhurst in thomhurst/TUnit#5376 * chore(deps): update dependency minimatch to v10 by @thomhurst in thomhurst/TUnit#5377 * chore(deps): update dependency picomatch to v4 by @thomhurst in thomhurst/TUnit#5382 * chore(deps): update dependency svgo to v4 by @thomhurst in thomhurst/TUnit#5383 * chore(deps): update dependency path-to-regexp to v1 [security] by @thomhurst in thomhurst/TUnit#5385 **Full Changelog**: thomhurst/TUnit@v1.25.0...v1.27.0 ## 1.25.0 <!-- Release notes generated using configuration in .github/release.yml at v1.25.0 --> ## What's Changed ### Other Changes * Fix missing `default` constraint on explicit interface implementations with unconstrained generics by @thomhurst in thomhurst/TUnit#5363 * feat(mocks): add ReturnsAsync typed factory overload with method parameters by @thomhurst in thomhurst/TUnit#5367 * Fix Arg.IsNull<T> and Arg.IsNotNull<T> to support nullable value types by @thomhurst in thomhurst/TUnit#5366 * refactor(mocks): use file-scoped types for generated implementation details by @thomhurst in thomhurst/TUnit#5369 * Compress HTML report JSON data and minify CSS by @thomhurst in thomhurst/TUnit#5368 ### Dependencies * chore(deps): update tunit to 1.24.31 by @thomhurst in thomhurst/TUnit#5356 * chore(deps): update dependency mockolate to 2.2.0 by @thomhurst in thomhurst/TUnit#5357 * chore(deps): update dependency polyfill to 9.24.1 by @thomhurst in thomhurst/TUnit#5365 * chore(deps): update dependency polyfill to 9.24.1 by @thomhurst in thomhurst/TUnit#5364 **Full Changelog**: thomhurst/TUnit@v1.24.31...v1.25.0 ## 1.24.31 <!-- Release notes generated using configuration in .github/release.yml at v1.24.31 --> ## What's Changed ### Other Changes * Fix Aspire 13.2.0+ timeout caused by ProjectRebuilderResource being awaited by @Copilot in thomhurst/TUnit#5335 * chore(deps): update dependency polyfill to 9.24.0 by @thomhurst in thomhurst/TUnit#5349 * Fix nullable IParsable type recognition in source generator and analyzer by @Copilot in thomhurst/TUnit#5354 * fix: resolve race condition in HookExecutionOrderTests by @thomhurst in thomhurst/TUnit#5355 * Fix MaxExternalSpansPerTest cap bypass when Activity.Parent chain is broken by @Copilot in thomhurst/TUnit#5352 ### Dependencies * chore(deps): update tunit to 1.24.18 by @thomhurst in thomhurst/TUnit#5340 * chore(deps): update dependency stackexchange.redis to 2.12.14 by @thomhurst in thomhurst/TUnit#5343 * chore(deps): update verify to 31.15.0 by @thomhurst in thomhurst/TUnit#5346 * chore(deps): update dependency polyfill to 9.24.0 by @thomhurst in thomhurst/TUnit#5348 **Full Changelog**: thomhurst/TUnit@v1.24.18...v1.24.31 ## 1.24.18 <!-- Release notes generated using configuration in .github/release.yml at v1.24.18 --> ## What's Changed ### Other Changes * feat(mocks): shorter, more readable generated mock type names by @thomhurst in thomhurst/TUnit#5334 * Fix DisposeAsync() ordering for nested property injection by @Copilot in thomhurst/TUnit#5337 ### Dependencies * chore(deps): update tunit to 1.24.13 by @thomhurst in thomhurst/TUnit#5331 **Full Changelog**: thomhurst/TUnit@v1.24.13...v1.24.18 ## 1.24.13 <!-- Release notes generated using configuration in .github/release.yml at v1.24.13 --> ## What's Changed ### Other Changes * perf(mocks): optimize MockEngine for lower allocation and faster verification by @thomhurst in thomhurst/TUnit#5319 * Remove defunct `UseTestingPlatformProtocol` reference for vscode by @erwinkramer in thomhurst/TUnit#5328 * perf(aspnetcore): prevent thread pool starvation during parallel WebApplicationTest server init by @thomhurst in thomhurst/TUnit#5329 * fix TUnit0073 for when type from from another assembly by @SimonCropp in thomhurst/TUnit#5322 * Fix implicit conversion operators bypassed in property injection casts by @Copilot in thomhurst/TUnit#5317 * fix(mocks): skip non-virtual 'new' methods when discovering mockable members by @thomhurst in thomhurst/TUnit#5330 * feat(mocks): IFoo.Mock() discovery with generic fallback and ORP resolution by @thomhurst in thomhurst/TUnit#5327 ### Dependencies * chore(deps): update tunit to 1.24.0 by @thomhurst in thomhurst/TUnit#5315 * chore(deps): update aspire to 13.2.1 by @thomhurst in thomhurst/TUnit#5323 * chore(deps): update verify to 31.14.0 by @thomhurst in thomhurst/TUnit#5325 ## New Contributors * @erwinkramer made their first contribution in thomhurst/TUnit#5328 **Full Changelog**: thomhurst/TUnit@v1.24.0...v1.24.13 ## 1.24.0 <!-- Release notes generated using configuration in .github/release.yml at v1.24.0 --> ## What's Changed ### Other Changes * perf: optimize TUnit.Mocks hot paths by @thomhurst in thomhurst/TUnit#5304 * fix: resolve System.Memory version conflict on .NET Framework (net462) by @thomhurst in thomhurst/TUnit#5303 * fix: resolve CS0460/CS0122/CS0115 when mocking concrete classes from external assemblies by @thomhurst in thomhurst/TUnit#5310 * feat(mocks): parameterless Returns() and ReturnsAsync() for async methods by @thomhurst in thomhurst/TUnit#5309 * Fix typo in NUnit manual migration guide by @aa-ko in thomhurst/TUnit#5312 * refactor(mocks): unify Mock.Of<T>() and Mock.OfPartial<T>() into single API by @thomhurst in thomhurst/TUnit#5311 * refactor(mocks): clean up Mock API surface by @thomhurst in thomhurst/TUnit#5314 * refactor(mocks): remove generic/untyped overloads from public API by @thomhurst in thomhurst/TUnit#5313 ### Dependencies * chore(deps): update tunit to 1.23.7 by @thomhurst in thomhurst/TUnit#5305 * chore(deps): update dependency mockolate to 2.1.1 by @thomhurst in thomhurst/TUnit#5307 ## New Contributors * @aa-ko made their first contribution in thomhurst/TUnit#5312 **Full Changelog**: thomhurst/TUnit@v1.23.7...v1.24.0 Commits viewable in [compare view](thomhurst/TUnit@v1.23.7...v1.28.7). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
OptimizedConsoleInterceptor— the overrides calledToString()which traverses the entire internal chunk linked list atomically. If the caller'sStringBuilderis mutated concurrently (e.g. pooledStringBuilderfrom ASP.NET Core logging under parallel test execution),ToString()throwsArgumentOutOfRangeException(chunkLength). The baseTextWriterimplementation iterates chunks viaGetChunks()and copies each one independently viaWrite(ReadOnlySpan<char>), which the interceptor already overrides — this eliminates the race window rather than catching the exception after the fact.StringBuilder.Lengthchecks insideReaderWriterLockSliminContext.GetStandardOutput()/GetErrorOutput()—StringBuilder.Lengthis not thread-safe and was being read outside synchronization.Why this differs from #5412
PR #5412 catches the
ArgumentOutOfRangeExceptionand swallows it (losing the log entry). This PR prevents the race entirely by not callingToString()— instead letting the baseTextWriterhandleStringBuilderarguments chunk-by-chunk, which is the same approach used byStreamWriter(the normalConsole.Out).Test plan
ContextThreadSafetyTestspass across all target frameworksFixes #5411