Skip to content
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

Add clean block to script block as a peer to begin, process, and end to allow easy resource cleanup #15177

Merged
merged 36 commits into from Oct 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
aabc7f8
:sparkles: Add cleanup block
vexx32 May 14, 2020
ed43b50
:recycle: Refactor & address code review
vexx32 May 15, 2020
b9cbf8b
:recycle: Refactor pipeline handling a bit
vexx32 May 16, 2020
dbc2b6c
:art: Linter fixes
vexx32 May 16, 2020
480eb01
:recycle: Refactor dispose in CommandProcessorBase
vexx32 May 16, 2020
e86422b
:wrench: Adjust scope and error handling
vexx32 May 18, 2020
506b3b2
:recycle: Avoid double type check for cleanup
vexx32 Aug 10, 2020
cb9491a
:recycle: Address Rob's feedback
vexx32 Aug 10, 2020
0d76834
:recycle: Move duplicated scope handling to method
vexx32 Aug 10, 2020
15b5485
:recycle: Refactor critical section logic
vexx32 Aug 10, 2020
47fd6d2
:recycle: Address Dongbo's review comments
vexx32 Mar 19, 2021
5e9f75f
Refactor the design of 'Clean' block implementation
daxian-dbw Apr 7, 2021
9704c7d
Revert a few unneeded changes
daxian-dbw Apr 7, 2021
376033a
Fix build and avoid using 'CriticalSection' code for now
daxian-dbw Apr 7, 2021
d9eac3b
Fix exception catching regression
daxian-dbw Apr 7, 2021
dc01c2b
Fix test failures
daxian-dbw Apr 7, 2021
aa57e6b
Fix pipeline regressions
daxian-dbw Apr 8, 2021
771dba7
Update the comments to make it more clear
daxian-dbw Apr 12, 2021
bab8f85
Complete a partial comment
daxian-dbw Apr 14, 2021
98339f5
Changes to SteppablePipeline to support 'Clean' block
daxian-dbw Apr 21, 2021
8d5e053
Clean up the steppable pipeline changes and add more comments
daxian-dbw Apr 21, 2021
5c8cc9a
Add lots of tests
daxian-dbw Jun 21, 2021
c9634e6
Add additional functional tests for steppable pipeline
daxian-dbw Jun 22, 2021
7f18442
Rename the new block from 'cleanup' to 'clean'
daxian-dbw Jul 4, 2021
35e466b
Fix 3 tests and a comment
daxian-dbw Jul 5, 2021
deadba7
Update a couple test comments
daxian-dbw Jul 8, 2021
e30736a
Add 2 more tests for 'exit' scenarios
daxian-dbw Jul 8, 2021
a2c9174
Address Ilya's comments
daxian-dbw Jul 10, 2021
7dbe15b
Add some more changes:
daxian-dbw Jul 14, 2021
aef7d95
Address new comments from Ilya
daxian-dbw Aug 13, 2021
451830d
Update comments to make the code easier to understand
daxian-dbw Aug 13, 2021
7ca9b4c
Update tests to use better names
daxian-dbw Aug 24, 2021
0060e0d
Address comments from Paul and Joel
daxian-dbw Aug 26, 2021
21538bb
Address Aditya's comment
daxian-dbw Aug 31, 2021
535584c
Revert changes in ImplicitRemotingCommands.cs because they are not ne…
daxian-dbw Sep 1, 2021
93305d4
Guard the 'clean' block behind an experimental feature
daxian-dbw Sep 7, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -2823,13 +2823,14 @@ private void GenerateHelperFunctions(TextWriter writer)

$clientSideParameters = Get-PSImplicitRemotingClientSideParameters $PSBoundParameters ${8}

$scriptCmd = {{ & $script:InvokeCommand `
@clientSideParameters `
-HideComputerName `
-Session (Get-PSImplicitRemotingSession -CommandName '{0}') `
-Arg ('{0}', $PSBoundParameters, $positionalArguments) `
-Script {{ param($name, $boundParams, $unboundParams) & $name @boundParams @unboundParams }} `
}}
$scriptCmd = {{
& $script:InvokeCommand `
@clientSideParameters `
-HideComputerName `
-Session (Get-PSImplicitRemotingSession -CommandName '{0}') `
-Arg ('{0}', $PSBoundParameters, $positionalArguments) `
-Script {{ param($name, $boundParams, $unboundParams) & $name @boundParams @unboundParams }} `
}}

$steppablePipeline = $scriptCmd.GetSteppablePipeline($myInvocation.CommandOrigin)
$steppablePipeline.Begin($myInvocation.ExpectingInput, $ExecutionContext)
Expand Down
7 changes: 7 additions & 0 deletions src/System.Management.Automation/engine/CommandBase.cs
Expand Up @@ -233,6 +233,13 @@ internal virtual void DoStopProcessing()
{
}

/// <summary>
/// When overridden in the derived class, performs clean-up after the command execution.
/// </summary>
internal virtual void DoCleanResource()
{
}

#endregion Override

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/CommandInfo.cs
Expand Up @@ -529,7 +529,7 @@ private void GetMergedCommandParameterMetadata(out MergedCommandParameterMetadat
processor = scriptCommand != null
? new CommandProcessor(scriptCommand, _context, useLocalScope: true, fromScriptFile: false,
sessionState: scriptCommand.ScriptBlock.SessionStateInternal ?? Context.EngineSessionState)
: new CommandProcessor((CmdletInfo)this, _context) { UseLocalScope = true };
daxian-dbw marked this conversation as resolved.
Show resolved Hide resolved
: new CommandProcessor((CmdletInfo)this, _context);

ParameterBinderController.AddArgumentsToCommandProcessor(processor, Arguments);
CommandProcessorBase oldCurrentCommandProcessor = Context.CurrentCommandProcessor;
Expand Down
21 changes: 20 additions & 1 deletion src/System.Management.Automation/engine/CommandMetadata.cs
Expand Up @@ -875,8 +875,11 @@ internal string GetProxyCommand(string helpComment, bool generateDynamicParamete

end
{{{5}}}

clean
{{{6}}}
<#
{6}
{7}
daxian-dbw marked this conversation as resolved.
Show resolved Hide resolved
#>
",
GetDecl(),
Expand All @@ -885,6 +888,7 @@ internal string GetProxyCommand(string helpComment, bool generateDynamicParamete
GetBeginBlock(),
GetProcessBlock(),
GetEndBlock(),
GetCleanBlock(),
CodeGeneration.EscapeBlockCommentContent(helpComment));

return result;
Expand Down Expand Up @@ -1063,6 +1067,11 @@ internal string GetBeginBlock()

internal string GetProcessBlock()
{
// The reason we wrap scripts in 'try { } catch { throw }' (here and elsewhere) is to turn
// an exception that could be thrown from .NET method invocation into a terminating error
// that can be propagated up.
// By default, an exception thrown from .NET method is not terminating, but when enclosed
// in try/catch, it will be turned into a terminating error.
daxian-dbw marked this conversation as resolved.
Show resolved Hide resolved
return @"
try {
$steppablePipeline.Process($_)
Expand Down Expand Up @@ -1113,6 +1122,16 @@ internal string GetEndBlock()
";
}

internal string GetCleanBlock()
{
// Here we don't need to enclose the script in a 'try/catch' like elsewhere, because
// 1. the 'Clean' block doesn't propagate up any exception (terminating error);
// 2. only one expression in the script, so nothing else needs to be stopped when invoking the method fails.
return @"
$steppablePipeline.Clean()
";
}

#endregion

#region Helper methods for restricting commands needed by implicit and interactive remoting
Expand Down
17 changes: 6 additions & 11 deletions src/System.Management.Automation/engine/CommandProcessor.cs
Expand Up @@ -309,13 +309,11 @@ internal override void DoBegin()
internal override void ProcessRecord()
{
// Invoke the Command method with the request object

if (!this.RanBeginAlready)
{
RanBeginAlready = true;
try
{
// NOTICE-2004/06/08-JonN 959638
using (commandRuntime.AllowThisCommandToWrite(true))
{
if (Context._debuggingMode > 0 && Command is not PSScriptCmdlet)
Expand All @@ -326,12 +324,9 @@ internal override void ProcessRecord()
Command.DoBeginProcessing();
}
}
// 2004/03/18-JonN This is understood to be
// an FXCOP violation, cleared by KCwalina.
catch (Exception e) // Catch-all OK, 3rd party callout.
catch (Exception e)
{
// This cmdlet threw an exception, so
// wrap it and bubble it up.
// This cmdlet threw an exception, so wrap it and bubble it up.
throw ManageInvocationException(e);
}
}
Expand Down Expand Up @@ -366,6 +361,7 @@ internal override void ProcessRecord()

// NOTICE-2004/06/08-JonN 959638
using (commandRuntime.AllowThisCommandToWrite(true))
using (ParameterBinderBase.bindingTracer.TraceScope("CALLING ProcessRecord"))
{
if (CmdletParameterBinderController.ObsoleteParameterWarningList != null &&
CmdletParameterBinderController.ObsoleteParameterWarningList.Count > 0)
Expand Down Expand Up @@ -400,14 +396,13 @@ internal override void ProcessRecord()
}
catch (LoopFlowException)
{
// Win8:84066 - Don't wrap LoopFlowException, we incorrectly raise a PipelineStoppedException
// Don't wrap LoopFlowException, we incorrectly raise a PipelineStoppedException
// which gets caught by a script try/catch if we wrap here.
throw;
}
// 2004/03/18-JonN This is understood to be
// an FXCOP violation, cleared by KCwalina.
catch (Exception e) // Catch-all OK, 3rd party callout.
catch (Exception e)
{
// Catch-all OK, 3rd party callout.
exceptionToThrow = e;
}
finally
Expand Down