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

Implement Cleanup { } Function Block #9900

Closed
wants to merge 11 commits into from
Expand Up @@ -2819,15 +2819,16 @@ private void GenerateHelperFunctions(TextWriter writer)

vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
vexx32 marked this conversation as resolved.
Show resolved Hide resolved
$positionalArguments.AddRange($args)

$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 }} `
}}
$clientSideParameters = Get-PSImplicitRemotingClientSideParameters $PSBoundParameters ${9}
Copy link
Member

Choose a reason for hiding this comment

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

The comment for private const string CommandProxyTemplate needs to be updated.


$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 All @@ -2840,6 +2841,8 @@ private void GenerateHelperFunctions(TextWriter writer)

End {{ {7} }}

Cleanup {{ {8} }}

# .ForwardHelpTargetName {1}
# .ForwardHelpCategory {5}
# .RemoteHelpRunspace PSSession
Expand All @@ -2865,7 +2868,8 @@ private static void GenerateCommandProxy(TextWriter writer, CommandMetadata comm
/* 5 */ commandMetadata.WrappedCommandType,
/* 6 */ ProxyCommand.GetProcess(commandMetadata),
/* 7 */ ProxyCommand.GetEnd(commandMetadata),
/* 8 */ commandMetadata.WrappedAnyCmdlet);
/* 8 */ ProxyCommand.GetCleanup(commandMetadata),
/* 9 */ commandMetadata.WrappedAnyCmdlet);
}

private static void GenerateCommandProxy(TextWriter writer, IEnumerable<CommandMetadata> listOfCommandMetadata)
Expand Down
17 changes: 16 additions & 1 deletion src/System.Management.Automation/engine/CommandMetadata.cs
Expand Up @@ -869,8 +869,11 @@ internal string GetProxyCommand(string helpComment, bool generateDynamicParamete

end
{{{5}}}

cleanup
{{{6}}}
<#
{6}
{7}
#>
",
GetDecl(),
Expand All @@ -879,6 +882,7 @@ internal string GetProxyCommand(string helpComment, bool generateDynamicParamete
GetBeginBlock(),
GetProcessBlock(),
GetEndBlock(),
GetCleanupBlock(),
CodeGeneration.EscapeBlockCommentContent(helpComment));

return result;
Expand Down Expand Up @@ -1107,6 +1111,17 @@ internal string GetEndBlock()
";
}

internal string GetCleanupBlock()
{
return @"
try {
$steppablePipeline.Dispose()
} catch {
throw
}
Comment on lines +1119 to +1121
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of the try/catch here if we just immediately throw?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea. I just copied the existing patterns for these generated strings. All of them have a try/catch where the catch does nothing other than throw.

I'm all for removing it, but didn't want to touch it in case there's some weird edge case that proxy commands need this to be here for. Even then, though, I'm pretty sure the weirdness around throw means this should instead be throw $_ (iirc there are some slight differences in the result, there's an old issue around it that nobody's been game to touch for years).

";
}

#endregion

#region Helper methods for restricting commands needed by implicit and interactive remoting
Expand Down
122 changes: 105 additions & 17 deletions src/System.Management.Automation/engine/CommandProcessorBase.cs
Expand Up @@ -366,7 +366,10 @@ internal void RestorePreviousScope()
{
OnRestorePreviousScope();

Context.EngineSessionState = _previousCommandSessionState;
if (_previousCommandSessionState != null)
{
Context.EngineSessionState = _previousCommandSessionState;
}

if (_previousScope != null)
{
Expand Down Expand Up @@ -645,13 +648,7 @@ internal void DoComplete()
_context.ShellFunctionErrorOutputPipe = oldErrorOutputPipe;
_context.CurrentCommandProcessor = oldCurrentCommandProcessor;

// Destroy the local scope at this point if there is one...
if (_useLocalScope && CommandScope != null)
{
CommandSessionState.RemoveScope(CommandScope);
}

// and the previous scope...
// Restore the previous scope...
if (_previousScope != null)
{
// Restore the scope but use the same session state instance we
Expand All @@ -668,6 +665,68 @@ internal void DoComplete()
}
}

protected virtual void HandleScopedAction<T>(Action<T> action, T state, string traceMessage)
{
Pipe oldErrorOutputPipe = Context.ShellFunctionErrorOutputPipe;
CommandProcessorBase oldCurrentCommandProcessor = Context.CurrentCommandProcessor;

try
{
// On V1 the output pipe was redirected to the command's output pipe only when it
// was already redirected. This is the original comment explaining this behaviour:
//
// NTRAID#Windows Out of Band Releases-926183-2005-12-15
// MonadTestHarness has a bad dependency on an artifact of the current implementation
// The following code only redirects the output pipe if it's already redirected
// to preserve the artifact. The test suites need to be fixed and then this
// the check can be removed and the assignment always done.
//
// However, this makes the hosting APIs behave differently than commands executed
// from the command-line host (for example, see bugs Win7:415915 and Win7:108670).
// The RedirectShellErrorOutputPipe flag is used by the V2 hosting API to force the
// redirection.
if (this.RedirectShellErrorOutputPipe || Context.ShellFunctionErrorOutputPipe != null)
{
Context.ShellFunctionErrorOutputPipe = CommandRuntime.ErrorOutputPipe;
}

Context.CurrentCommandProcessor = this;
using (CommandRuntime.AllowThisCommandToWrite(permittedToWriteToPipeline: true))
using (ParameterBinderBase.bindingTracer.TraceScope(traceMessage))
{
SetCurrentScopeToExecutionScope();
action(state);
}
}
catch (Exception e)
{
throw ManageInvocationException(e);
}
finally
{
Context.ShellFunctionErrorOutputPipe = oldErrorOutputPipe;
Context.CurrentCommandProcessor = oldCurrentCommandProcessor;

// Destroy the local scope at this point if there is one...
if (_useLocalScope && CommandScope != null)
{
CommandSessionState.RemoveScope(CommandScope);
}

RestorePreviousScope();
}
}

/// <summary>
/// Virtual method to be overridden by a command processor type that handles script functions, such as
/// <see cref="DlrScriptCommandProcessor"/>.
/// The base implementation does nothing.
/// </summary>
protected virtual void CleanupScriptCommand()
{
// Do nothing -- subclasses may use.
}

/// <summary>
/// For diagnostic purposes.
/// </summary>
Expand Down Expand Up @@ -940,21 +999,50 @@ public void Dispose()
private void Dispose(bool disposing)
{
if (_disposed)
{
return;
}

if (disposing)
try
{
// 2004/03/05-JonN Look into using metadata to check
// whether IDisposable is implemented, in order to avoid
// this expensive reflection cast.
IDisposable id = Command as IDisposable;
if (id != null)
if (disposing)
{
id.Dispose();
try
{
CommandRuntime.RemoveVariableListsInPipe();

// This method handles script commands' `cleanup {}` blocks.
// It is a no-op for compiled cmdlets.
CleanupScriptCommand();
}
finally
{
if (Command is PSScriptCmdlet scriptCmdlet)
{
bool isStopping = ExceptionHandlingOps.SuspendStoppingPipeline(Context);
try
{
HandleScopedAction(
(cmdlet) => cmdlet.Dispose(),
scriptCmdlet,
traceMessage: "CALLING Cleanup");
}
finally
{
ExceptionHandlingOps.RestoreStoppingPipeline(Context, isStopping);
}
}
else if (Command is IDisposable disposableCommand)
{
disposableCommand.Dispose();
}
}
}
}

_disposed = true;
finally
{
_disposed = true;
}
}

#endregion IDispose
Expand Down
5 changes: 5 additions & 0 deletions src/System.Management.Automation/engine/EventManager.cs
Expand Up @@ -1844,6 +1844,11 @@ public sealed class PSEngineEvent
/// </summary>
internal const string OnScriptBlockInvoke = "PowerShell.OnScriptBlockInvoke";

/// <summary>
/// Called during invocation of cleanup{} for script commands.
/// </summary>
internal const string OnScriptCommandCleanup = "PowerShell.OnScriptCommandCleanup";

/// <summary>
/// Called during scriptblock invocation.
/// </summary>
Expand Down
24 changes: 24 additions & 0 deletions src/System.Management.Automation/engine/ProxyCommand.cs
Expand Up @@ -247,6 +247,30 @@ public static string GetEnd(CommandMetadata commandMetadata)
return commandMetadata.GetEndBlock();
}

/// <summary>
/// This method constructs a string representing the dispose block of the command
/// specified by <paramref name="commandMetadata"/>. The returned string only contains the
/// script, it is not enclosed in "cleanup { }".
/// </summary>
/// <param name="commandMetadata">
/// An instance of CommandMetadata representing a command.
/// </param>
/// <returns>
/// A string representing the end block of the command.
/// </returns>
/// <exception cref="ArgumentNullException">
/// If <paramref name="commandMetadata"/> is null.
/// </exception>
public static string GetCleanup(CommandMetadata commandMetadata)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the scenario for exposing this method publicly?

Copy link
Collaborator Author

@vexx32 vexx32 May 15, 2020

Choose a reason for hiding this comment

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

I could be misremembering, but I think I just copied the existing access modifiers for the other blocks? If not we can make it private, it doesn't need to be different to the others here. IIRC this would be used by... New-ProxyCommand, I think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I was wondering if it's because PowerShell script needs it somewhere in that strange internal but exposed twilight zone...

{
if (commandMetadata == null)
{
throw PSTraceSource.NewArgumentNullException(nameof(commandMetadata));
}

return commandMetadata.GetCleanupBlock();
}

private static T GetProperty<T>(PSObject obj, string property) where T : class
{
T result = null;
Expand Down