Repository navigation
Fix crash when stopping pipeline with stop token - #28300
Jordan Borean (jborean93) wants to merge 2 commits into
Conversation
There is a race condition where `PipelineProcess.Stop()` can be called after `Dispose()` has already disposed the token. This causes the `Stop()` call to raise `ObjectDisposedException` crashing the process. An explicitly registered action on `$PSCmdlet.PipelineStopToken.Register()` can also throw an exception which is not handled in the stop call causing a similar crash. The fix is to ignore `AggregateException` which contains any exceptions raised by the user defined delegates on the stop token and exit early if the token has been disposed. We exit early because `Dispose()` has already done the work and there is nothing left to stop.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not reliably exercise cancellation of a disposed token source or the AggregateException handler, leaving the original crash without dependable regression coverage.
2 open issues
What changed in this PR
This PR hardens pipeline cancellation to prevent shell crashes when stopping races with disposal or a registered stop-token callback throws, addressing #28299.
Changes:
- Return safely when the cancellation source has already been disposed.
- Catch callback exceptions so command stopping can continue.
- Add a callback-failure test and adjust test readiness signals.
| File | Description |
|---|---|
test/powershell/Language/Scripting/PipelineStoppedToken.Tests.ps1 |
Adds callback-failure coverage and moves readiness signals into test scripts. |
src/System.Management.Automation/engine/pipeline.cs |
Handles disposed cancellation sources and exceptions from stop-token callbacks. |
Review effort: Balanced
| { | ||
| _pipelineStopTokenSource.Cancel(); | ||
| } | ||
| catch (ObjectDisposedException) |
There was a problem hiding this comment.
Not sure how this would be possible without adding test only hooks that lock the state to a particular set of steps. This would be very complicated and means we start testing an implementation detail. Ultimately this is a race condition that I don't believe we can really test against.
Ilya (iSazonov)
added
the
CL-General
microsoft-github-policy-service
Bot
added
the
Review - Needed
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
PR Summary
There is a race condition where
PipelineProcess.Stop()can be called afterDispose()has already disposed the token. This causes theStop()call to raiseObjectDisposedExceptioncrashing the process. An explicitly registered action on$PSCmdlet.PipelineStopToken.Register()can also throw an exception which is not handled in the stop call causing a similar crash.The fix is to ignore
AggregateExceptionwhich contains any exceptions raised by the user defined delegates on the stop token and exit early if the token has been disposed. We exit early becauseDispose()has already done the work and there is nothing left to stop.PR Context
Fixes: #28299
It also fixes a related problem where if the delegate registered by a caller could throw which is raised as an
AggregateException. In C# this wouldn't typically happen but if someone in PowerShell provided any ScriptBlock delegate the cancellation will most likely be triggered somewhere that doesn't have a runspace. For example:The fix is to ignore
AggregateExceptionso that this exception, any anything else a registered delegate a C# cmdlet would register, do not bring down the process.This should be considered for backporting as well. While it is a race condition, the issue from #28299 shows that it can crash the whole process.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header