Skip to content

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

Merged
merged 4 commits into from
Mar 24, 2025

Conversation

jborean93
Copy link
Collaborator

@jborean93 jborean93 commented Nov 26, 2024

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.

Function Test-FunctionWithCancellation {
    [CmdletBinding()]
    param ()

    [MyType]::SomeMethodWithCancellation($PSCmdlet.PipelineStopToken)
}

Before this was only really possible in a binary cmdlet and it required extra boilerplate. The binary cmdlet can now look like

[Cmdlet(VerbsDiagnostic.Test, "CmdletWIthCancellation")]
public sealed class TestCmdletWithCancellation : PSCmdlet
{
    protected override void EndProcessing()
    {
        MyType.SomeMethodWithCancellation(PipelineStopToken);
    }
}

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Dec 4, 2024
@SeeminglyScience SeeminglyScience added WG-Engine core PowerShell engine, interpreter, and runtime WG-NeedsReview Needs a review by the labeled Working Group labels Dec 4, 2024
@kasini3000

This comment was marked as off-topic.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jan 6, 2025
@SeeminglyScience SeeminglyScience added CommunityDay-Small A small PR that the PS team has identified to prioritize to review WG-Reviewed A Working Group has reviewed this and made a recommendation and removed WG-NeedsReview Needs a review by the labeled Working Group labels Jan 13, 2025
@daxian-dbw daxian-dbw self-assigned this Jan 13, 2025
Copy link
Member

@daxian-dbw daxian-dbw left a 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!

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Jan 25, 2025
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.
@jborean93
Copy link
Collaborator Author

@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:

  • Should the token be cancelled before or after StopProcessing is called on each command in the pipeline
    • Currently it will cancel it after
  • Should the stop token be added the ICommandRuntime interface through a new ICommandRuntime3 and that is used as the check
    • Currently it just verifies the CommandRuntime of the PSCmdlet is MshCommandRuntime
    • 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

@iSazonov
Copy link
Collaborator

/azp run

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jan 28, 2025
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@alx9r
Copy link

alx9r commented Jan 28, 2025

Should the token be cancelled before or after StopProcessing is called on each command in the pipeline

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 $PSCmdlet.PipelineStopToken.IsCancellationRequested in user code to avert undesired behavior and extraneous errors arising from that condition.

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 $result | ConvertTo-SomeOtherFormat is executed when Invoke() is canceled. In that scenario, the engine currently completes assignment to $result despite the pipeline already being in the Stopping state, but does not execute $result | ConvertTo-SomeOtherFormat. (The behavior of assignments to PowerShell variables from .Net methods while stopping is contemplated in #24658. There are empirical test results demonstrating the behavior in this answer).

If, on the other hand, the token is cancelled before StopProcessing, then $result would have the return value of a cancelled call to Invoke() assigned to it. Let's say that value is $null. That then means that the author of Invoke-SomeCancellableWorkload has to consider the possibility of $result being $null when the line $result | ConvertTo-SomeOtherFormat is executed after token cancellation but before StopProcessing. That scenario is apt to cause at least an extraneous parameter binding error that would likely be observed by users whenever that unfortunate thread scheduling occurs. But the author might also deem that inputting $null to ConvertTo-SomeOtherFormat is unnacceptable because that results in unknown, undesired, or destructive behavior. Then the author would probably have to resort to something like the following:

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.

@jborean93
Copy link
Collaborator Author

@alx9r in the script based functions it won't matter because script functions cannot implement StopProcessing, so whether the token is cancelled before or after the effectively no-op StopProcessing function is called won't make a different. The question is more important for compiled cmdlets as they can have their own StopProcessing implementation. Even then I don't think it matters too much.

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.

@alx9r
Copy link

alx9r commented Jan 28, 2025

@jborean93

in the script based functions it won't matter because script functions cannot implement StopProcessing, so whether the token is cancelled before or after the effectively no-op StopProcessing function is called won't make a different.

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 clean{}? I was referring to that as StopProcessing because in all testing I have done, the invocation of StopProcessing on a compiled Cmdlet corresponds to the same timing as flow of control jump to clean{} (and $PSCmdlet.Stopping changing from false to true). Whatever that event is called, the argument I made here for StopProcessing surely applies to that event. In other words, whatever you call that event, there is a significant difference when authoring Advanced Functions whether that event happens before or after $PSCmdlet.PipelineStopToken is canceled.

@jborean93
Copy link
Collaborator Author

I think you might be focusing too much on this cancellation/cleanup side. This PR enables two things for end users:

  • Removes the need for common boilerplate done in compiled cmdlets that manually setup a CancellationToken
  • Exposes the same functionality present in a compiled function into a script function
    • They can now call async functions with a cancellation token and do a blocking wait and it will respond to stop events
    • Currently you need to do a loop that frees up the engine every few ms to respond to a cancellation event

The talk about when clean is called in relation to stop is unrelated to this PR.

@alx9r
Copy link

alx9r commented Jan 28, 2025

The talk about when clean is called in relation to stop is unrelated to this PR.

My concern is not about when clean{} is called in relation to stop. My concern is about when the $PSCmdlet.PipelineStopToken available in an Advanced Function is canceled in relation to when pipeline stopping affects the execution of the PowerShell code of that Advanced Function.

@jborean93
Copy link
Collaborator Author

My concern is about when the $PSCmdlet.PipelineStopToken available in an Advanced Function is canceled in relation to when pipeline stopping affects the execution of the PowerShell code of that Advanced Function.

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 CancellationToken will raise the OperationCanceledException if set. For example

& {
    [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 CancellationToken to the PSCmdlet to allow cmdlets and now script functions to use that. The former removing the boilerplate needed, the latter actually making it possible.

@alx9r
Copy link

alx9r commented Jan 29, 2025

@jborean93

I think we are still talking past one another. I'll try a different approach. With $PSCmdlet.PipelineStopToken there are now two separate events that can affect the execution of user code when stopping an Advanced Function:

  • cancellation of the token
  • interruption of executing PowerShell code

Which event happens first?

@jborean93
Copy link
Collaborator Author

jborean93 commented Jan 29, 2025

The pipeline is marked as stopped before the pipeline calls StopProcessing on each cmdlet and thus sets the token to be cancelled. This PR doesn't change any of that and this is why I'm trying to say this is off topic and just adds noise to the PR.

internal void Stop()
{
PipelineProcessor[] copyStack;
lock (_syncRoot)
{
if (_stopping)
{
return;
}
_stopping = true;
copyStack = _stack.ToArray();
}
// Propagate error from the toplevel operation through to enclosing the LocalPipeline.
if (copyStack.Length > 0)
{
PipelineProcessor topLevel = copyStack[copyStack.Length - 1];
if (topLevel != null && _localPipeline != null)
{
_localPipeline.SetHadErrors(topLevel.ExecutionFailed);
}
}
// Note: after _stopping is set to true, nothing can be pushed/popped
// from stack and it is safe to call stop on PipelineProcessors in stack
// outside the lock
// Note: you want to do below loop outside the lock so that
// pipeline execution thread doesn't get blocked on Push and Pop.
// Note: A copy of the stack is made because we "unstop" a stopped
// pipeline to execute finally blocks. We don't want to stop pipelines
// in the finally block though.
foreach (PipelineProcessor pp in copyStack)
{
pp.Stop();
}
}

In Stop() the _stopping field is set to true before pp.Stop() (what called StopProcessing on each cmdlet) is called.

@alx9r
Copy link

alx9r commented Jan 30, 2025

The pipeline is marked as stopped before the pipeline calls StopProcessing on each cmdlet and thus sets the token to be cancelled.
...
In Stop() the _stopping field is set to true before pp.Stop() (what called StopProcessing on each cmdlet) is called.

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:

  1. _stopping is set to true. This has the effect that flow of user PowerShell code of the pipeline undergoing stopping will subsequently be controlled according to the stopping rules.
  2. PipelineProcessor.Stop() is invoked which calls .Cancel() on the CancellationTokenSource from which $PSCmdlet.PipelineStopToken was produced.

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.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Feb 6, 2025
@powercode powercode added WG-Reviewed A Working Group has reviewed this and made a recommendation and removed Review - Needed The PR is being reviewed WG-Reviewed A Working Group has reviewed this and made a recommendation labels Feb 12, 2025
@daxian-dbw
Copy link
Member

I'm assuming here that token cancellation and StopProcessing both happen on a different thread from the execution of the PowerShell instance being stopped.
... that causes flow of control of an advanced function to jump to clean{}? I was referring to that as StopProcessing because ...

@alx9r If I understand correctly, you refer to clean {} as StopProcessing. clean {} will always be invoked on the pipeline thread, which is the same thread that runs begin {}, process {}, and end {}. When pipeline is being stopped, clean {} runs after all running scripts are actually stopped. PipelineProcessor.Stop, on the other hand, runs on a separate thread.

Can you confirm that this is correct?

The order is correct.

@@ -914,6 +922,8 @@ internal void Stop()
continue;
}
}

_pipelineStopTokenSource.Cancel();
Copy link
Member

@daxian-dbw daxian-dbw Feb 19, 2025

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 new ICommandRuntime3 and that is used as the check
    • Currently it just verifies the CommandRuntime of the PSCmdlet is MshCommandRuntime
    • 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?

Copy link
Collaborator Author

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.

Copy link

@alx9r alx9r Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daxian-dbw

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.

Copy link
Member

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!

Copy link
Collaborator

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()
}

Copy link
Collaborator Author

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.

Copy link
Collaborator

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!

Copy link
Collaborator Author

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.

@rkeithhill rkeithhill added WG-Triaged The issue has been triaged by the designated WG. and removed WG-Triaged The issue has been triaged by the designated WG. labels Feb 22, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Mar 1, 2025
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @jborean93!

@daxian-dbw daxian-dbw merged commit 93e053c into PowerShell:master Mar 24, 2025
33 checks passed
@github-project-automation github-project-automation bot moved this from PR In Progress to Done in PowerShell Team Reviews/Investigations Mar 24, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Mar 24, 2025
@daxian-dbw daxian-dbw added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Mar 24, 2025
@jborean93 jborean93 deleted the cancellation-token branch March 24, 2025 21:41
@jborean93
Copy link
Collaborator Author

Thanks for the reviews and comments on this one!

Sysoiev-Yurii pushed a commit to Sysoiev-Yurii/PowerShell that referenced this pull request May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-Engine Indicates that a PR should be marked as an engine change in the Change Log CommunityDay-Small A small PR that the PS team has identified to prioritize to review WG-Engine core PowerShell engine, interpreter, and runtime WG-Reviewed A Working Group has reviewed this and made a recommendation
Projects
Development

Successfully merging this pull request may close these issues.

Add a PipelineStopToken cancellation token property to PSCmdlet
8 participants