Skip to content

[Server] Restore CurrentCulture/CurrentUICulture at end of each request#13756

Closed
JanProvaznik wants to merge 1 commit into
dotnet:mainfrom
JanProvaznik:prototype/msbuild-server-culture-restore
Closed

[Server] Restore CurrentCulture/CurrentUICulture at end of each request#13756
JanProvaznik wants to merge 1 commit into
dotnet:mainfrom
JanProvaznik:prototype/msbuild-server-culture-restore

Conversation

@JanProvaznik
Copy link
Copy Markdown
Member

@JanProvaznik JanProvaznik commented May 13, 2026

Server: Restore CurrentCulture/CurrentUICulture at end of each request

Honest framing (post-review feedback): this is defense-in-depth hygiene, not a fix for any observable bug today. See "Why the failure mode is weak" below before deciding whether to merge.

Snapshot Thread.CurrentThread.CurrentCulture and CurrentUICulture once at server startup (in OutOfProcServerNode.Run, before the listener loop), then restore them in a try/finally around each HandleServerNodeBuildCommand invocation.

Why the failure mode is weak

Each request explicitly sets Thread.CurrentThread.CurrentCulture = command.Culture (OutOfProcServerNode.cs:386-387) before any culture-sensitive build work runs. Build work runs synchronously on the same Task.Run-spawned ThreadPool thread, so the per-request set covers the whole request. Walking the lifecycle:

  1. Request N → Task.Run picks a ThreadPool thread → sets culture to N's → build runs → returns.
  2. Thread goes back to the pool with culture still set to N's.
  3. Request N+1 → Task.Run picks some ThreadPool thread (maybe the same one) → sets culture to N+1's → build runs.

Whichever thread N+1's Task.Run picks, the very first thing it does is overwrite the culture. So the prior request's culture cannot leak into N+1's build. The narrow scenarios where the leftover culture could matter are all weak:

Scenario Real?
Listener / packet-pump thread reads CurrentCulture for trace formatting Pump thread never has command.Culture set, so it uses the process default — unaffected by this PR
Finalizer / GC callback reads CurrentCulture Runs on dedicated finalizer/GC threads, never the request handler — unaffected
Async continuation on a recycled handler thread between requests Continuations on ThreadPool threads don't preserve thread-local culture; whatever the picked thread happens to have is non-deterministic regardless
Fire-and-forget work outliving the request Bad practice in tasks; doesn't actually capture Thread.CurrentThread.CurrentCulture for use later
Debugger / heap-dump inspector observing idle server thread cultures Real but cosmetic, not a build correctness issue

What the test actually proves (and doesn't)

MSBuildServer_Tests.ServerBuildsUseCultureFromEachRequest runs back-to-back en-US then fr-FR builds and asserts each build's task observes its own culture and that the server PID is reused. The test passes with or without the restore code in this PR, because the explicit set in HandleServerNodeBuildCommand already guarantees correctness for what the test observes.

So the test is a useful regression for the existing per-request set behavior, but it does NOT validate that this PR fixes any concrete bug.

Why merge anyway (defense-in-depth case)

  1. Symmetry with the existing reset patterns: HandleShutdown already resets the working directory; CWD restore was added for the same "between-requests cleanliness" reason. This PR extends that pattern to culture.
  2. Future-proofing: if MSBuild ever adds between-request work (a periodic stats emitter, a passive cache eviction timer, a server-startup background task) that would otherwise see stale culture from the last request handler thread, it would be predictable instead of "whatever the last build set it to".
  3. Idle-server inspection: a debugger attached to a long-running idle server sees baseline cultures rather than the last build's request culture.
  4. Cost is tiny — ~20 lines, no risk to existing scenarios, no performance impact.

What this PR is NOT

It is not a fix for:

Implementation

  • Snapshot _originalCulture / _originalUICulture in Run() before the listener loop.
  • Wrap the body of HandleServerNodeBuildCommand in a try/finally that restores both.
  • Two files changed: OutOfProcServerNode.cs (+19 lines), MSBuildServer_Tests.cs (+86 lines for the regression test + helper task).

Recommendation

Merge if the maintainers value defense-in-depth hygiene + symmetry with the existing CWD reset; close as "premature optimization" if not. I have no strong preference but lean toward merge given the very low cost.

Investigation context: #9379 Thread G item 3.

Snapshot Thread.CurrentThread.CurrentCulture and CurrentUICulture once at server
startup (in OutOfProcServerNode.Run, before the listener loop begins), then restore
them in a try/finally around each HandleServerNodeBuildCommand invocation.

Without this, each request mutates the server thread's culture from command.Culture
and command.UICulture but never reverts. A subsequent request whose client provides
identical cultures sees no observable problem, but if the server enters a state-read
code path between requests (lazy initializers, finalizers, GC callbacks reading
CultureInfo.CurrentCulture), it sees the prior request's culture rather than the
server's process-startup baseline. Symmetric snapshot/restore matches the existing
working-directory restore pattern in HandleShutdown.

Investigation context: investigation.md Thread G item 3 in the broader MSBuild
Server default-on analysis (dotnet#9379).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JanProvaznik
Copy link
Copy Markdown
Member Author

iterated that this is not a realistic failure mode, not worth fixing, but I'm glad the investigation was done

@JanProvaznik
Copy link
Copy Markdown
Member Author

Closing rationale (post-review): this PR fixes a leak that only a contract-violating task or logger could cause. MSBuild tasks aren't supposed to touch Thread.CurrentThread.CurrentCulture directly — they have IBuildEngine for I/O. The framework's per-request Thread.CurrentThread.CurrentCulture = command.Culture already covers the legitimate case in OutOfProcServerNode.cs:386-387.

I was building justification for a fix in search of a problem. Confirming the close was correct; same logic applied to #13757 which I just closed.

The other two server PRs from the same investigation stand on their own merits:

See #9379 for the consolidated default-on analysis.

@JanProvaznik JanProvaznik deleted the prototype/msbuild-server-culture-restore branch May 14, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant