-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Add PipelineStopToken to PSCmdlet #24620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jborean93 for making this change. Love the new property!
test/powershell/Language/Scripting/PipelineStoppedToken.Tests.ps1
Outdated
Show resolved
Hide resolved
Adds the PipelineStopToken property to a PSCmdlet instance that makes it easier for cmdlets and advanced functions to integrate a CancellationToken in their code for calling .NET methods that support them.
f5d5dfb
to
daf63c8
Compare
@daxian-dbw thanks for the review, I've just rebased on the latest master and made the change to move the cancellation token to the pipeline processor. There are probably a few things we want to decide on:
|
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
Cancelling the token after StopProcessing would result in fewer branches in user PowerShell code to achieve correct results, I think. That would be better ergonomics for authors of Advanced Functions. I'm assuming here that token cancellation and StopProcessing both happen on a different thread from the execution of the PowerShell instance being stopped. If StopProcessing occurs after cancelling the token then the PowerShell thread can continue to execute user PowerShell code after token cancellation but before StopProcessing whenever unfortunate thread scheduling occurs. That would mean PowerShell authors have to consider cases where the PowerShell engine has canceled the token but not yet stopped the pipeline. I expect that would necessarily lead to checks of Consider the following user code: function Invoke-SomeCancellableWorkload {
[CmdletBinding()]
param()
$result = [MyCancellableWorkload]::Invoke($PSCmdlet.PipelineStopToken)
$result | ConvertTo-SomeOtherFormat
} If the token is cancelled after StopProcessing, then the PowerShell engine will always interrupt PowerShell execution before If, on the other hand, the token is cancelled before StopProcessing, then function Invoke-SomeCancellableWorkload {
[CmdletBinding()]
param()
$result = [MyCancellableWorkload]::Invoke($PSCmdlet.PipelineStopToken)
. {}
if ($PSCmdlet.PipelineStopToken.IsCancellationRequested) {
throw [StopTokenCanceledBeforeStopProcessingException]::new()
return
}
$result | ConvertTo-SomeOtherFormat
} I think the former is more readable. But the latter would be necessary if cancellation occurs before StopProcessing. |
@alx9r in the script based functions it won't matter because script functions cannot implement Please keep in mind this PR is not trying to deal with any scenarios like variable assignment or the like. It is focused specifically on just exposing a CancellationToken for functions or cmdlets to use when calling methods that support a CancellationToken so it can stop its execution. In the majority of cases the cancellation will result in an OperationCanceledException meaning callers don't have to worry about checking the cancellation status of the token anyway. |
I think I might be miscommunicating, then. For script functions, how do you refer to the point in execution on the thread that invokes stopping that causes flow of control of an advanced function to jump to |
I think you might be focusing too much on this cancellation/cleanup side. This PR enables two things for end users:
The talk about when clean is called in relation to stop is unrelated to this PR. |
My concern is not about when |
It's exactly the same as it is today, the only difference is that now there is a way to call .NET functions that accept a CancellationToken and have it respond to the stop request. Normally this works just fine because a task that accepts a & {
[CmdletBinding()] param()
[System.Threading.Tasks.Task]::Delay(-1, $PSCmdlet.PipelineStopToken).GetAwaiter().GetResult()
[Console]::WriteLine("Will not be called")
} If the method does not raise the exception or you catch it yourself then it'll continue to run until it reaches a point where the engine checks if it is stopped. For example when calling a cmdlet, entering a loop. & {
[CmdletBinding()] param()
try {
[System.Threading.Tasks.Task]::Delay(-1, $PSCmdlet.PipelineStopToken).GetAwaiter().GetResult()
}
catch {
[Console]::WriteLine("Exception $_")
}
[Console]::WriteLine("Will be called")
Write-Host test # PowerShell checks here if it is stopped or not
[Console]::WriteLine("Will not be called")
}
# Exception Exception calling "GetResult" with "0" argument(s): "A task was canceled."
# Will be called If the API doesn't use that token properly then you need to handle that accordingly. This works exactly the same as in a compiled cmdlet, if you don't release control back to the engine somehow then things will continue to run. But once again, this is off topic to this PR. This is not about the stop implementation but rather about adding the |
I think we are still talking past one another. I'll try a different approach. With
Which event happens first? |
The pipeline is marked as stopped before the pipeline calls PowerShell/src/System.Management.Automation/engine/hostifaces/LocalPipeline.cs Lines 1361 to 1398 in 4e79421
In |
Based on this and my attempt to trace through the source I think the thread invoking stopping completes the events in question in the following order:
Can you confirm that this is correct? If that is correct, then my concern is addressed since that is the order that avoids the races I was concerned about. |
@alx9r If I understand correctly, you refer to
The order is correct. |
@@ -914,6 +922,8 @@ internal void Stop() | |||
continue; | |||
} | |||
} | |||
|
|||
_pipelineStopTokenSource.Cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jborean93 Sorry for my delay on getting back to this PR.
- Should the token be cancelled before or after
StopProcessing
is called on each command in the pipeline
- Currently it will cancel it after
I prefer to moving it before the calls to DoStopProcessing()
, so as to signal the cancellation sooner. With that, commands in pipeline that use the cancellation token could have started the cleanup while we are running StopProcess
for those that don't.
- Should the stop token be added the
ICommandRuntime
interface through a newICommandRuntime3
and that is used as the check
- Currently it just verifies the
CommandRuntime
of thePSCmdlet
isMshCommandRuntime
- This should always be the case so it's a simple Debug assertion but maybe that will change in the future
- A new interface seems like a drastic change so I didn't go ahead with it
I agree that a new interface is overkill. But this property should be available to commands that derive from Cmdlet
too. There are many cmdlets whose implementation actually derives from Cmdlet
, such as ConvertFrom-Json
, Get/Wait/Stop/Debug-Process
, and quite some service cmdlets in PowerShell. When running in PowerShell, I believe we will assign an MshCommandRuntime
instance to them. Also, Cmdlet
has the public property Stopping
, which make it reasonable for it to have the PipelineStopToken
property as well.
Maybe we should consider adding the PipelineStopToken
property to the abstract base class InternalCommand
(IsStopping
is defined in it, which is returned from Cmdlet.Stopping
). Something like the following:
// in InternalCommand class
// internal bool IsStopping ...
internal CancellationToken StopToken => commandRuntime is MshCommandRuntime mcr
? mcr.PipelineProcessor.PipelineStopToken
: CancellationToken.Default;
// in Cmdlet class
// public bool Stopping ...
public CancellationToken PipelineStopToken => StopToken;
@SeeminglyScience thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can definitely move the cancellation to before DoStopProcessing
is called on each pipeline command. I cannot think of a race condition here that would affect existing code so I think it is safe to do so.
I can certainly move it to Cmdlet
(accessed by InternalCommand
as suggested). I just assumed we wanted things to use PSCmdlet
over Cmdlet
and this would be a nice carrot to do so but I'll wait to see what @SeeminglyScience thinks.
I'll also need to dig in a big deeper into the CI failures as it seems like the PipelineProcessor
was disposed before the cancellation token was stopped so I'm not 100% sure what is happening there yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, you refer to clean {} as StopProcessing.
If I referred to clean {}
as StopProcessing
that was a mistake and I certainly didn't intend that.
I wrote
I'm assuming here that token cancellation and StopProcessing both happen on a different thread from the execution of the PowerShell instance being stopped.
because if they aren't on the thread executing user PowerShell then a race can arise from the time that elapses between _stopping
getting set to true and $PSCmdlet.PipelineStopToken
being cancelled. In particular the thread that does the stopping/cancelling can be preempted between setting _stopping
and cancelling $PSCmdlet.PipelineStopToken
while the thread executing user PowerShell code continues.
Can you confirm that this is correct?
The order is correct.
Thank you for confirming. This sounds good to me. I expect that order to work good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SeeminglyScience can you please review @jborean93 and my comments in this thread and share your thoughts? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to moving it before the calls to DoStopProcessing(), so as to signal the cancellation sooner. With that, commands in pipeline that use the cancellation token could have started the cleanup while we are running StopProcess for those that don't.
Hmm my only worry with this is script based commands with a try finally. Since the cancellation token will be triggered in a different thread I wonder how likely the timing is for PSScriptCmdlet.StopProcessing
to start occuring while the script command has just entered the finally block.
Something like
New-Item temp:\sometempfile
$tcpClient = [Net.Sockets.TcpClient]::new()
try {
# will throw OperationCancelled on ctrl c
$tcpClient.ConnectAsync($PSCmdlet.PipelineStopToken).GetAwaiter().GetResult()
# do stuff with tcpclient
} finally {
# StopProcessing will probably be called somewhere in this `if` block
if ((Test-Path temp:\sometempfile) {
Remove-Item temp:\sometempfile
}
# is this likely to run?
($tcpClient)?.Dispose()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does StopProcessing
do anything for script functions? IIRC last time I checked it was used for CDXML cmdlets but that's about it. It didn't seem to be user configurable hence this PR being opened.
At this point in time the pipeline has been marked as stopped so if the finally block was to call something which checks this state and also re-throws then the stop token being triggered before or after won't make a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep pretty sure you're 100% correct there lol
No objections from me then!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming, I've pushed a commit that moved the cancellation to before DoStopProcessing
and also exposed the CancellationToken on the Cmdlet
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @jborean93!
Thanks for the reviews and comments on this one! |
…ipeline is stopping (PowerShell#24620)
PR Summary
Adds the PipelineStopToken property to a PSCmdlet instance that makes it easier for cmdlets and advanced functions to integrate a CancellationToken in their code for calling .NET methods that support them.
PR Context
Fixes: #17444
Slight alternative to: #19685
I've opened an RFC to cover this approach PowerShell/PowerShell-RFC#382. This PR shows how it could be implemented and that it is not too intrusive.
This now makes it possible to do the following and have it signal the
CancellationToken
when the engine is set to stop.Before this was only really possible in a binary cmdlet and it required extra boilerplate. The binary cmdlet can now look like
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).