-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Does SteppablePipeline.Clean() have be invoked from a compiled cmdlet for clean{} to run reliably? #11643
Comments
Hope the bold part makes it's easier to understand :)
But it's possible that the proxy function fails, say, before calling public sealed class InvokeSteppableCommand : PSCmdlet, IDisposable
{
...
protected override void ProcessRecord(){ steppable.Process(InputObject); }
protected override void EndProcessing() { throw new WhateverException(); steppable.End(); }
...
} The cmdlet will fail when throwing out the Yes, it could cause the
The |
Thank you very much for the informative reply @daxian-dbw. I understand this much better now.
Calling (I'm using this updated implementation of Invoke-Steppable which traces method calls for the following example.)using System;
using System.Management.Automation;
using System.Collections.Generic;
[Cmdlet(VerbsLifecycle.Invoke, "Steppable")]
public sealed class InvokeSteppableCommand : PSCmdlet, IDisposable
{
private SteppablePipeline steppable;
[Parameter(ValueFromPipeline = true)]
public object InputObject { get; set; }
[Parameter(Mandatory = true, Position = 0)]
public ScriptBlock ScriptBlock { get; set; }
[Parameter()]
public List<string> Log {get;set;}
protected override void BeginProcessing() {
Log.Add("BeginProcessing()");
steppable = ScriptBlock
.Create(string.Format("param($__sb) . $__sb"))
.GetSteppablePipeline(
CommandOrigin.Internal,
new object[] { ScriptBlock });
steppable.Begin(this);
}
protected override void ProcessRecord(){
Log.Add("ProcessRecord()");
steppable.Process(InputObject);
}
protected override void EndProcessing() {
Log.Add("EndProcessing()");
steppable.End();
}
public void Dispose() {
Log.Add("Dispose()");
steppable.Dispose();
}
} $log = [System.Collections.Generic.List[string]]::new()
try {
1..2 |
. {
process {
if ($_ -eq 2) { throw 'something'}
$_
}
} |
Invoke-Steppable -Log $log {
begin { $log.Add('command begin{}' ) }
process { $log.Add("command process{} $_" ); $_ }
end { $log.Add('command end{}' ) }
clean { $log.Add('command clean{}' ) }
}
}
catch {}
$log That outputs
which shows that
I think that is the better choice here. Adding
Removing the
For what it's worth I ended up tracing a wide range of pipeline scenarios looking for equivalence class partitions. The test fixture is a bit involved, but I've put the test report here for reference. test reportThe Traces table below shows traces of the blocks and methods invoked in various pipeline scenarios. The following code is representative of the test setup for the prototypical "Normal Upstream" pipeline shown in the table: 1..2 |
# upstream
. {
begin { $log.Add("upstream begin{}") }
process { $log.Add("upstream process{$_}"); $_ }
end { $log.Add("upstream end{}") }
clean { $log.Add("upstream clean{}") }
} |
Invoke-Steppable -Log $log {
begin { $log.Add('command begin{}' ) }
process { $log.Add("command process{$_}"); $_ }
end { $log.Add('command end{}' ) }
clean { $log.Add('command clean{}' ) }
} |
. {
begin { $log.Add("downstream begin{}") }
process { $log.Add("downstream process{$_}"); $_ }
end { $log.Add("downstream end{}") }
clean { $log.Add("downstream clean{}") }
}
# downstream Each column in the Traces table is a trace of a distinct pipeline scenario. Upstream and downstream in the column name refers to a command added to the prototypical pipeline at the Traces table
With that report it's easy to see, for example, that
Many of the results in that test report are counterintuitve to me. But it all looks manageable for my goals. |
That is very thorough testing! I think there is one more case that
This is an interesting point. Maybe we should explore and see if |
Thank you @daxian-dbw. FWIW, the fixture isn't nearly as complicated as I thought it would be. In hindsight I should have done that long ago.
Right. I added those scenarios to the test fixture. I'm using Stop test report
I was expecting this to work this way too. But calling
The |
The Since you have the fixture handy, can you please also add the following scenarios and see if it works as expected in those cases:
We should open a PowerShell issue about the unexpected behavior in those "ctrl+c/Stop" scenarios. |
@daxian-dbw Those results are indeed interesting. updated report
If you think it would help, I could try to extract this test fixture and publish it somewhere. |
Thank you! So, the unexpected behavior -- command
Yes please! That'd be very helpful. Also, I think we should create a new issue to track this bug in PowerShell repo. |
My pleasure @daxian-dbw.
I agree.
I will try to do so.
I agree. I guess we need a portable repro for that though. We'll either get that by me publishing this test fixture, or I'll cobble together another repro. |
Related to PowerShell/PowerShell#24679 |
I'll do that next. Test FixtureI have attached a PowerShell module in You should just be able to expand that archive and invoke I think the examples I put in Get-ControlFlowTestParameters |
Test-ControlFlow |
? {-not $_.CleanupOk} will test a number of scenarios and output the culprits that we discussed above (and a few others that turned up). That will output objects like Name : stop upstream process{}
Parameters : {[LogParameterName, Log],
[WaitPoint, stop upstream process{}],
[CommandName, Invoke-Steppable],
[DryRun, False]} whose Parameters property you can use to craft focused repros like Trace-ControlFlow -CommandName Invoke-Steppable -WaitPoint 'stop upstream process{}' -DryRun $false | % Log which produces the trace log upstream begin{}
command begin{}
downstream begin{}
upstream process{1}
command process{1}
downstream process{1}
upstream clean{}
downstream clean{} which, I think, you're already familiar with from above. I documented the public parts of the module in the usual PowerShell help places. There's not much explanation of how the internals work, though. Hopefully it'll be clear enough for anyone stepping through it. The In any case, I think this will streamline the repros for future testing of Thoughts on documenting
|
That is now PowerShell/PowerShell#24782. |
@alx9r Please add this discussion to PowerShell/PowerShell#24782. |
@sdwheeler Ah...good point. I have quoted it here. |
Links
Summary
The remarks for
SteppablePipeline.Clean()
state to invoke.Clean()
from theclean{}
block of a PowerShell function. Those remarks do not state when to invoke.Clean()
from a compiledCmdlet
if at all. Because there is not really aCmdlet
method that corresponds toclean{}
, it's not obvious when to invoke.Clean()
from a compiledCmdlet
.FWIW, I would call this an omission rather than a "bug".
Details
Below is a pattern that often arises when authoring commands involving user script blocks and .Net objects where implementation directly from PowerShell is difficult.
This cmdlet, for example, synthesizes a pipeline stopping cancellation token uses this pattern. I use this pattern to implement
Use-Transaction
(i.e. for sqlite) andUse-Object
in a manner that is reliable despite the complications described in #24679, # 23786, and #24658.The use of
SteppablePipeline
is the only technique I know of that results in idiomatic control of flow when invoking a user script block from a compiledCmdlet
. Specifically, the codetraces nearly identical flow of control as when
Invoke-Steppable
is replaced by.
as shown in the following table:Invoke-Steppable
.
Only the
clean{}
block is called at a different point. (I can imagine some scenarios where that might have consequences, but those scenarios all seem avoidable.)I did not expect
clean{}
to be invoked at all byInvoke-Steppable
because the documentation forSteppablePipeline.Clean()
suggests that.Clean()
would have to be invoked in order forclean{}
to run:Based on these remarks I expected to have to call
.Clean()
in order forclean{}
to run.For commands written in PowerShell the remarks state that
SteppablePipeline.Clean()
should be called from theclean{}
block. CompiledCmdlet
s, however, do not have a method that obviously corresponds toclean{}
, and it's not obvious to me whether calling.Clean()
is necessary for compiledCmdlet
s.I have tried to summarize the contenders for where
.Clean()
could be called from a compiledCmdlet
in the following table:EndProcessing()
StopProcessing()
Dispose()
IDisposable.Dispose()
of aCmdlet
that implementsIDisposable
(likeInvoke-Steppable
above).`Notes
1. An exception thrown upstream from the a compiled
Cmdlet
prevents theEndProcessing()
from being called. This is the same control of flow when a script block is substituted for theCmdlet
. That is shown in "repro 1" below.repro 1
outputs
Note that
end{}
never runs.The table above really leaves only
Dispose()
as a viable contender for callingclean{}
. But it seems likeclean{}
is already called whenSteppablePipeline.Dispose()
is called.Is calling
.Clean()
required forclean{}
to run reliably?Suggested Fix
Once an answer to the underlying question is determined, I think it should be added to the
SteppablePipeline.Clean()
method's remarks section.The text was updated successfully, but these errors were encountered: