From 88c8aa54f0e7590e305b8705125b5ac973cf77a6 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 21 Apr 2021 09:30:16 -0700 Subject: [PATCH] Changes to SteppablePipeline to support 'Clean' block --- .../engine/CommandMetadata.cs | 14 +- .../engine/lang/scriptblock.cs | 39 +++-- .../engine/pipeline.cs | 145 +++++++++--------- 3 files changed, 110 insertions(+), 88 deletions(-) diff --git a/src/System.Management.Automation/engine/CommandMetadata.cs b/src/System.Management.Automation/engine/CommandMetadata.cs index b29f784b448db..bfe5ca5f7a924 100644 --- a/src/System.Management.Automation/engine/CommandMetadata.cs +++ b/src/System.Management.Automation/engine/CommandMetadata.cs @@ -1067,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. return @" try { $steppablePipeline.Process($_) @@ -1119,12 +1124,11 @@ internal string GetEndBlock() internal string GetCleanupBlock() { + // 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 @" - try { - $steppablePipeline.Dispose() - } catch { - throw - } + $steppablePipeline.Clean() "; } diff --git a/src/System.Management.Automation/engine/lang/scriptblock.cs b/src/System.Management.Automation/engine/lang/scriptblock.cs index 0c6c1d5dff1a7..e74fada27e908 100644 --- a/src/System.Management.Automation/engine/lang/scriptblock.cs +++ b/src/System.Management.Automation/engine/lang/scriptblock.cs @@ -1280,7 +1280,32 @@ public Array End() { // then pop this pipeline and dispose it... _context.PopPipelineProcessor(true); - _pipeline.Dispose(); + Dispose(); + } + } + + /// + /// Clean resources for script commands of this steppable pipeline. + /// + public void Clean() + { + if (_pipeline.Commands is null) + { + // The pipeline commands have been disposed. In this case, + // 'Clean' should have already been called. + return; + } + + try + { + _context.PushPipelineProcessor(_pipeline); + _pipeline.Clean(); + } + finally + { + // then pop this pipeline and dispose it... + _context.PopPipelineProcessor(true); + Dispose(); } } @@ -1293,23 +1318,13 @@ public Array End() /// When this object is disposed, the contained pipeline should also be disposed. /// public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - private void Dispose(bool disposing) { if (_disposed) { return; } - if (disposing) - { - _pipeline.Dispose(); - } - + _pipeline.Dispose(); _disposed = true; } diff --git a/src/System.Management.Automation/engine/pipeline.cs b/src/System.Management.Automation/engine/pipeline.cs index a1ff65b2cba7d..7f2d67c9ac96f 100644 --- a/src/System.Management.Automation/engine/pipeline.cs +++ b/src/System.Management.Automation/engine/pipeline.cs @@ -66,7 +66,6 @@ internal class PipelineProcessor : IDisposable public void Dispose() { Dispose(true); - GC.SuppressFinalize(this); } private void Dispose(bool disposing) @@ -689,9 +688,9 @@ private void DoCompleteCore(CommandProcessorBase commandRequestingUpstreamComman /// /// Clean up resources for script commands in this pipeline processor. /// - private void Clean() + internal void Clean() { - if (_commands is null) + if (!_executionStarted || _commands is null) { return; } @@ -752,6 +751,12 @@ private void Clean() /// The results of the execution. internal Array DoComplete() { + if (!_executionStarted) + { + throw PSTraceSource.NewInvalidOperationException( + PipelineStrings.PipelineNotStarted); + } + try { if (Stopping) @@ -759,12 +764,6 @@ internal Array DoComplete() throw new PipelineStoppedException(); } - if (!_executionStarted) - { - throw PSTraceSource.NewInvalidOperationException( - PipelineStrings.PipelineNotStarted); - } - ExceptionDispatchInfo toRethrowInfo; try { @@ -779,7 +778,7 @@ internal Array DoComplete() // By rethrowing the exception outside of the handler, we allow the CLR on X64/IA64 to free from the stack // the exception records related to this exception. - // The only reason we should get here is if an exception should be rethrown. + // The only reason we should get here is an exception should be rethrown. Diagnostics.Assert(toRethrowInfo != null, "Alternate protocol path failure"); toRethrowInfo.Throw(); @@ -788,36 +787,42 @@ internal Array DoComplete() } finally { + Clean(); DisposeCommands(); } } /// - /// This routine starts the stepping process. It is optional to - /// call this but can be useful if you want the begin clauses - /// of the pipeline to be run even when there may not be any input - /// to process as is the case for I/O redirection into a file. We - /// still want the file opened, even if there was nothing to write to it. + /// This routine starts the stepping process. It is optional to call this but can be useful + /// if you want the begin clauses of the pipeline to be run even when there may not be any + /// input to process as is the case for I/O redirection into a file. We still want the file + /// opened, even if there was nothing to write to it. /// /// True if you want to write to this pipeline. internal void StartStepping(bool expectInput) { + bool startSucceeded = false; try { Start(expectInput); + startSucceeded = true; // Check if this pipeline is being stopped asynchronously. ThrowFirstErrorIfExisting(logException: false); } - catch (PipelineStoppedException) + catch (Exception e) { + Clean(); DisposeCommands(); - // The error we want to report is the first terminating error - // which occurred during pipeline execution, regardless - // of whether other errors occurred afterward. - ThrowFirstErrorIfExisting(logException: false); + if (!startSucceeded && e is PipelineStoppedException) + { + // We want to report the first terminating error which occurred during pipeline execution, + // regardless of whether other errors occurred afterward. + ThrowFirstErrorIfExisting(logException: false); + } + // If we haven't recorded the first terminating error, then re-throw this exception. throw; } } @@ -832,23 +837,26 @@ internal void Stop() // Only call StopProcessing if the pipeline is being stopped // for the first time - if (!RecordFailure(new PipelineStoppedException(), null)) + if (!RecordFailure(new PipelineStoppedException(), command: null)) + { return; + } // Retain copy of _commands in case Dispose() is called List commands = _commands; - if (commands == null) + if (commands is null) + { return; + } // Call StopProcessing() for all the commands. - for (int i = 0; i < commands.Count; i++) + foreach (CommandProcessorBase commandProcessor in commands) { - CommandProcessorBase commandProcessor = commands[i]; - if (commandProcessor == null) { throw PSTraceSource.NewInvalidOperationException(); } + try { commandProcessor.Command.DoStopProcessing(); @@ -900,36 +908,30 @@ internal void Stop() /// internal Array Step(object input) { - if (Stopping) - { - DisposeCommands(); - throw new PipelineStoppedException(); - } - + bool injectSucceeded = false; try { Start(true); Inject(input, enumerate: false); + injectSucceeded = true; // Check if this pipeline is being stopped asynchronously. ThrowFirstErrorIfExisting(logException: false); - return RetrieveResults(); } - catch (PipelineStoppedException) + catch (Exception e) { + Clean(); DisposeCommands(); - // The error we want to report is the first terminating error - // which occurred during pipeline execution, regardless - // of whether other errors occurred afterward. - ThrowFirstErrorIfExisting(logException: false); + if (!injectSucceeded && e is PipelineStoppedException) + { + // We want to report the first terminating error which occurred during pipeline execution, + // regardless of whether other errors occurred afterward. + ThrowFirstErrorIfExisting(logException: false); + } - throw; - } - catch (Exception) - { - DisposeCommands(); + // If we haven't recorded the first terminating error, then re-throw this exception. throw; } } @@ -1296,41 +1298,40 @@ private void DisposeCommands() LogToEventLog(); - if (_commands != null) + if (_commands is not null) { - for (int i = 0; i < _commands.Count; i++) + foreach (CommandProcessorBase commandProcessor in _commands) { - CommandProcessorBase commandProcessor = _commands[i]; - if (commandProcessor != null) + if (commandProcessor is null) + { + continue; + } + + // If Dispose throws an exception, record it as a pipeline failure and continue disposing cmdlets. + try { - // If Dispose throws an exception, record it as a pipeline failure and continue disposing cmdlets. - try + commandProcessor.CommandRuntime.RemoveVariableListsInPipe(); + commandProcessor.Dispose(); + } + catch (Exception e) + { + // The only vaguely plausible reason for a failure here is an exception in 'Command.Dispose'. + // As such, this should be covered by the overall exemption. + InvocationInfo myInvocation = commandProcessor.Command?.MyInvocation; + + if (e is ProviderInvocationException pie) { - commandProcessor.CommandRuntime.RemoveVariableListsInPipe(); - commandProcessor.Dispose(); + e = new CmdletProviderInvocationException(pie, myInvocation); } - // The only vaguely plausible reason for a failure here is an exception in Command.Dispose. - // As such, this should be covered by the overall exemption. - catch (Exception e) + else { - InvocationInfo myInvocation = null; - if (commandProcessor.Command != null) - myInvocation = commandProcessor.Command.MyInvocation; - - if (e is ProviderInvocationException pie) - { - e = new CmdletProviderInvocationException(pie, myInvocation); - } - else - { - e = new CmdletInvocationException(e, myInvocation); - - // Log a command health event - MshLog.LogCommandHealthEvent(commandProcessor.Command.Context, e, Severity.Warning); - } - - RecordFailure(e, commandProcessor.Command); + e = new CmdletInvocationException(e, myInvocation); + + // Log a command health event + MshLog.LogCommandHealthEvent(commandProcessor.Command.Context, e, Severity.Warning); } + + RecordFailure(e, commandProcessor.Command); } } } @@ -1348,7 +1349,9 @@ private void DisposeCommands() } // Clean resources for script commands. - // This method catches and handles all exceptions inside, so it will never throw. + // It is possible (though very unlikely) that the call to 'Step' on the redirection pipeline failed. + // In such a case, 'Clean' would have run and the 'pipelineProcessor' would have been disposed. + // Therefore, calling 'Clean' again will simply return, because '_commands' was already set to null. redirPipe.Clean(); // The complicated logic of disposing the commands is taken care