From e890bac692e7d86586de2d89f57c6b5c5f72d2a2 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 15 Sep 2017 16:32:42 -0700 Subject: [PATCH 1/5] Call 'DoComplete' when handling FileRedirection for CommandExpression --- .../engine/parser/Compiler.cs | 86 ++++++++++++++----- .../engine/runtime/Operations/MiscOps.cs | 14 +++ .../Parser/RedirectionOperator.Tests.ps1 | 29 +++++++ 3 files changed, 107 insertions(+), 22 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index 62462ad53af..bed1a593759 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -181,6 +181,8 @@ internal static class CachedReflectionInfo internal static readonly MethodInfo FileRedirection_BindForExpression = typeof(FileRedirection).GetMethod(nameof(FileRedirection.BindForExpression), instanceFlags); + internal static readonly MethodInfo FileRedirection_CallDoCompleteForExpression = + typeof(FileRedirection).GetMethod(nameof(FileRedirection.CallDoCompleteForExpression), instanceFlags); internal static readonly ConstructorInfo FileRedirection_ctor = typeof(FileRedirection).GetConstructor(instanceFlags, null, CallingConventions.Standard, new Type[] { typeof(RedirectionStream), typeof(bool), typeof(string) }, null); @@ -3186,6 +3188,7 @@ private Expression GetRedirectedExpression(CommandExpressionAst commandExpr, boo exprs.Add(Expression.Call(oldPipe, CachedReflectionInfo.Pipe_SetVariableListForTemporaryPipe, s_getCurrentPipe)); } + List extraFileRedirectExprs = null; // We must generate the code for output redirection to a file before any merging redirections // because merging redirections will use funcContext._outputPipe as the value to merge to, so defer merging // redirections until file redirections are done. @@ -3194,35 +3197,55 @@ private Expression GetRedirectedExpression(CommandExpressionAst commandExpr, boo // This will simply return a Linq.Expression representing the redirection. var compiledRedirection = VisitFileRedirection(fileRedirectionAst); - // For non-output streams (error, warning, etc.) we must save the old stream so it can be restored. - // The savedPipe variable is used only for setting funcContext._outputPipe for redirecting Output to file. - // The savedPipes variable is used for restoring non-output streams (error, warning, etc.). - var savedPipes = NewTemp(typeof(Pipe[]), "savedPipes"); - temps.Add(savedPipes); + if (extraFileRedirectExprs == null) + { + extraFileRedirectExprs = new List(commandExpr.Redirections.Count); + } + // Hold the current 'FileRedirection' instance for later use var redirectionExpr = NewTemp(typeof(FileRedirection), "fileRedirection"); temps.Add(redirectionExpr); exprs.Add(Expression.Assign(redirectionExpr, (Expression)compiledRedirection)); - /* - if (fileRedirectionAst.FromStream != RedirectionStream.Output && !(redirectionExpr is ConstantExpression)) - { - // We'll be reusing redirectionExpr, it's not constant, so save it in a temp. - var temp = Expression.Variable(redirectionExpr.Type); - temps.Add(temp); - exprs.Add(Expression.Assign(temp, redirectionExpr)); - redirectionExpr = temp; - } - */ + + // We must save the old streams so they can be restored later. + var savedPipes = NewTemp(typeof(Pipe[]), "savedPipes"); + temps.Add(savedPipes); exprs.Add(Expression.Assign( savedPipes, Expression.Call(redirectionExpr, CachedReflectionInfo.FileRedirection_BindForExpression, _functionContext))); - finallyExprs.Add(Expression.Call(redirectionExpr.Cast(typeof(CommandRedirection)), - CachedReflectionInfo.CommandRedirection_UnbindForExpression, - _functionContext, - savedPipes)); + + // We need to call 'DoComplete' on the file redirection pipeline processor after writing the stream output to redirection pipe, + // so that the 'EndProcessing' method of 'Out-File' would be called as expected. + // Expressions for this purpose are kept in 'extraFileRedirectExprs' and will be used later. + extraFileRedirectExprs.Add(Expression.Call(redirectionExpr, CachedReflectionInfo.FileRedirection_CallDoCompleteForExpression)); + + // The 'UnBind' and 'Dispose' operations on 'FileRedirection' objects must be done in the reversed order of 'Bind' operations. + // Namely, it should be done is this order: + // try { + // // The order is A, B + // fileRedirectionA = new FileRedirection(..) + // fileRedirectionA.BindForExpression() + // fileRedirectionB = new FileRedirection(..) + // fileRedirectionB.BindForExpression() + // ... + // } finally { + // // The oder must be B, A + // fileRedirectionB.UnBind() + // fileRedirectionB.Dispose() + // fileRedirectionA.UnBind() + // fileRedirectionA.Dispose() + // } + // + // Otherwise, the original pipe might not be correctly restored in the end. For example, + // '1 *> b.txt > a.txt; 123' would result in the following error when evaluating '123': + // "Cannot perform operation because object "PipelineProcessor" has already been disposed" + finallyExprs.Insert(0, Expression.Call(redirectionExpr.Cast(typeof(CommandRedirection)), + CachedReflectionInfo.CommandRedirection_UnbindForExpression, + _functionContext, + savedPipes)); // In either case, we must dispose of the redirection or file handles won't get released. - finallyExprs.Add(Expression.Call(redirectionExpr, - CachedReflectionInfo.FileRedirection_Dispose)); + finallyExprs.Insert(1, Expression.Call(redirectionExpr, + CachedReflectionInfo.FileRedirection_Dispose)); } Expression result = null; @@ -3287,10 +3310,29 @@ private Expression GetRedirectedExpression(CommandExpressionAst commandExpr, boo } } + if (extraFileRedirectExprs != null) + { + // Now that the redirection is done, we need to call 'DoComplete' on the file redirection pipeline processors. + // Exception may be thrown when running 'DoComplete', but we don't want the exception to affect the cleanup + // work in 'finallyExprs'. We generate the following code to make sure it does what we want. + // try { + // try { + // exprs + // } finally { + // extraFileRedirectExprs + // } + // finally { + // finallyExprs + // } + var wrapExpr = Expression.TryFinally(Expression.Block(exprs), Expression.Block(extraFileRedirectExprs)); + exprs.Clear(); + exprs.Add(wrapExpr); + } + if (oldPipe != null) { // If a temporary pipe was created at the beginning, we should restore the original pipe in the - // very end of the finally block. Otherwise, _getCurrentPipe may be messed up by the following + // very end of the finally block. Otherwise, s_getCurrentPipe may be messed up by the following // file redirection unbind operation. // For example: // function foo diff --git a/src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs b/src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs index 0cba45b8eec..c28fe992003 100644 --- a/src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs +++ b/src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs @@ -1158,6 +1158,20 @@ internal Pipe GetRedirectionPipe(ExecutionContext context, PipelineProcessor par return new Pipe(context, PipelineProcessor); } + /// + /// After file redirection is done, we need to call 'DoComplete' on the pipeline processor, + /// so that 'EndProcessing' of Out-File can be called to wrap up the file write operation. + /// + /// + /// 'StartStepping' is called after creating the pipeline processor. + /// 'Step' is called when an object is added to the pipe created with the pipeline processor. + /// + internal void CallDoCompleteForExpression() + { + Diagnostics.Assert(PipelineProcessor != null, "PipelineProcessor should never be null when running this method"); + PipelineProcessor.DoComplete(); + } + private bool _disposed; public void Dispose() diff --git a/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 b/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 index 20b35867651..b50095a2617 100644 --- a/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 +++ b/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 @@ -92,3 +92,32 @@ Describe "File redirection mixed with Out-Null" -Tags CI { Get-Content $TestDrive\out.txt | Should Be "some more text" } } + +Describe "File redirection should have 'DoComplete' called on the underlying pipeline processor" { + It "File redirection should result in the same file as Out-File" { + $object = [pscustomobject] @{ one = 1 } + $redirectFile = Join-Path $TestDrive fileRedirect.txt + $outFile = Join-Path $TestDrive outFile.txt + + $object > $redirectFile + $object | Out-File $outFile + + $redirectFileContent = Get-Content $redirectFile -Raw + $outFileContent = Get-Content $outFile -Raw + $redirectFileContent | Should Be $outFileContent + } + + It "File redirection should not mess up the original pipe" { + $outputFile = Join-Path $TestDrive output.txt + $otherStreamFile = Join-Path $TestDrive otherstream.txt + + $result = & { $(Get-Command NonExist; 1234) > $outputFile *> $otherStreamFile; "Hello" } + $result | Should Be "Hello" + + $outputContent = Get-Content $outputFile -Raw + $outputContent.Trim() | Should Be '1234' + + $errorContent = Get-Content $otherStreamFile -Raw + $errorContent | Should Match "CommandNotFoundException,Microsoft.PowerShell.Commands.GetCommandCommand" + } +} From f6978d7ff453dbaf590b511e28ddb70d445084a4 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 15 Sep 2017 16:52:12 -0700 Subject: [PATCH 2/5] Add '-tags CI' for tests --- test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 b/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 index b50095a2617..8eeb099fdc5 100644 --- a/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 +++ b/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 @@ -93,7 +93,7 @@ Describe "File redirection mixed with Out-Null" -Tags CI { } } -Describe "File redirection should have 'DoComplete' called on the underlying pipeline processor" { +Describe "File redirection should have 'DoComplete' called on the underlying pipeline processor" -Tags CI { It "File redirection should result in the same file as Out-File" { $object = [pscustomobject] @{ one = 1 } $redirectFile = Join-Path $TestDrive fileRedirect.txt From 131f33a7e7466d21af48f910a47dc8e702515cc9 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 15 Sep 2017 17:57:36 -0700 Subject: [PATCH 3/5] Call 'DoComplete' only if it's not NullPipe --- .../engine/runtime/Operations/MiscOps.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs b/src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs index c28fe992003..9559c61d8db 100644 --- a/src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs +++ b/src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs @@ -1168,8 +1168,11 @@ internal Pipe GetRedirectionPipe(ExecutionContext context, PipelineProcessor par /// internal void CallDoCompleteForExpression() { - Diagnostics.Assert(PipelineProcessor != null, "PipelineProcessor should never be null when running this method"); - PipelineProcessor.DoComplete(); + // The pipe returned from 'GetRedirectionPipe' could be a NullPipe + if (PipelineProcessor != null) + { + PipelineProcessor.DoComplete(); + } } private bool _disposed; From 86afb9f798c5ebc0a5882eff7c9b1b9e62c436b2 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 15 Sep 2017 23:27:25 -0700 Subject: [PATCH 4/5] [Feature] Fix a test issue --- test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 b/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 index 8eeb099fdc5..b492cb7580a 100644 --- a/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 +++ b/test/powershell/Language/Parser/RedirectionOperator.Tests.ps1 @@ -117,7 +117,8 @@ Describe "File redirection should have 'DoComplete' called on the underlying pip $outputContent = Get-Content $outputFile -Raw $outputContent.Trim() | Should Be '1234' - $errorContent = Get-Content $otherStreamFile -Raw + $errorContent = Get-Content $otherStreamFile | ForEach-Object { $_.Trim() } + $errorContent = $errorContent -join "" $errorContent | Should Match "CommandNotFoundException,Microsoft.PowerShell.Commands.GetCommandCommand" } } From d73e1e1d625f50d1f3b0ecc67cc809724c9bbf9f Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 20 Sep 2017 13:18:04 -0700 Subject: [PATCH 5/5] Fix a typo --- src/System.Management.Automation/engine/parser/Compiler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/parser/Compiler.cs b/src/System.Management.Automation/engine/parser/Compiler.cs index bed1a593759..bddddf1c734 100644 --- a/src/System.Management.Automation/engine/parser/Compiler.cs +++ b/src/System.Management.Automation/engine/parser/Compiler.cs @@ -3219,7 +3219,7 @@ private Expression GetRedirectedExpression(CommandExpressionAst commandExpr, boo // Expressions for this purpose are kept in 'extraFileRedirectExprs' and will be used later. extraFileRedirectExprs.Add(Expression.Call(redirectionExpr, CachedReflectionInfo.FileRedirection_CallDoCompleteForExpression)); - // The 'UnBind' and 'Dispose' operations on 'FileRedirection' objects must be done in the reversed order of 'Bind' operations. + // The 'UnBind' and 'Dispose' operations on 'FileRedirection' objects must be done in the reverse order of 'Bind' operations. // Namely, it should be done is this order: // try { // // The order is A, B