Fix coordinator timer disposal race - #14765
Conversation
Track in-flight server timer callbacks and defer synchronization-state disposal until RunAsync and callbacks have completed. Reuse the shutdown timer and add deterministic concurrent-disposal coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DustinCampbell
left a comment
DustinCampbell
left a comment
There was a problem hiding this comment.
In general, I'm not sure I understand all of the changes in this PR. Some of it looks solid, but other code looks a bit more desperate. There's a lot of code that "hardens" the CoodinatorServer against cases that don't happen in production and should probably be fixed in the test infrastructure.
There are some strange changes added without explanation, such as wrapping a single _output.WriteLine(...) call in try/finally? Is that because the TestCoordinatorDebugOutput was being called after the test had finished running? If that's the case, why doesn't the test wait until the server is disposed? I recognize that there's a problem with timers not being shutdown properly, but that should be handled in the same way that the CoordinatorClient's HeartbeatTimer does.
| lock (_lifecycleLock) | ||
| { | ||
| ObjectDisposedException.ThrowIf(_disposeState != NotDisposed, this); | ||
|
|
||
| _output.WriteLine($"CoordinatorServer: Accept loop started on pipe '{_pipeName}' (budget={_settings.TotalNodeBudget})"); | ||
| if (_runStarted) | ||
| { | ||
| throw new InvalidOperationException("The coordinator server can only be run once."); | ||
| } | ||
| _runStarted = true; | ||
| } |
There was a problem hiding this comment.
This looks suspicious to me. In production, there's only a single caller of RunAsync(...) in Program.cs. Is this just for tests? If so, could we fix this in the test infrastructure rather than complicating the server code for cases that can't happen in production?
| while (!token.IsCancellationRequested) | ||
| { | ||
| NamedPipeServerStream? pipeStream = await WaitForClientAsync(token); | ||
| NamedPipeServerStream? pipeStream = await WaitForClientAsync(token).ConfigureAwait(false); |
There was a problem hiding this comment.
Why was ConfigureAwait(false) added? Is there a reason we don't want to continue in the current context in the server?
| { | ||
| try | ||
| { | ||
| await Task.WhenAll(remainingTasks).ConfigureAwait(false); |
There was a problem hiding this comment.
Why is ConfigureAwait(false) used here?
| try | ||
| { | ||
| _output.WriteLine("CoordinatorServer: Accept loop exiting"); | ||
| } |
There was a problem hiding this comment.
Why is it necessary to protect a call to _output.WriteLine(...) in a try/finally?
| lock (_lifecycleLock) | ||
| { | ||
| _runCompleted = true; | ||
| } |
There was a problem hiding this comment.
Again, this is very suspicious to me. It really feels like changing production code rather than test infrastruture.
2 participants
Context
The Windows net472 x86 Coordinator test process in #14725 crashed after all 168 tests passed because a queued heartbeat timer callback entered
CheckHeartbeatsafterCoordinatorServer.Dispose()had disposed_clientsLock. The priority scheduling change did not modify timer or lock disposal; this race exists independently onmain.Fix
_ctsand_clientsLockonly afterRunAsyncclient cleanup and all entered timer callbacks completeDispose()callsValidation
build.cmd -rebuild -configuration Release -projects src\MSBuild.Coordinator.UnitTests\MSBuild.Coordinator.UnitTests.csproj -v quiet