Fix bugs with expression redirected to file (PowerShell#4847)

When handling file redirection for CommandExpression, we don't call 'DoComplete' on the underlying PipelineProcessor of the FileRedirection object, and thus the EndProcessing method is not called on Out-File, which causes different behaviors between <expr> > out.txt and <expr> | Out-File out.txt.

The fix is to make sure 'DoComplete' is called after the stream output has been written to the redirection pipe.

Also fix another issue

This PR also fixes an issue that could mess up restoring the original pipes. Here is the repro:

PS> 1 *> b.txt > a.txt; 123
Cannot perform operation because object "PipelineProcessor" has already been disposed
The root cause is that we don't always restore pipes in the correct order. Please see the code changes in Compiler.cs for more details.

Fix PowerShell#4812

Author: daxian-dbw

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<Expression> 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<Expression>(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 reverse 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

src/System.Management.Automation/engine/runtime/Operations/MiscOps.cs

@@ -1158,6 +1158,23 @@ internal Pipe GetRedirectionPipe(ExecutionContext context, PipelineProcessor par
             return new Pipe(context, PipelineProcessor);
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        /// <remark>
+        /// 'StartStepping' is called after creating the pipeline processor.
+        /// 'Step' is called when an object is added to the pipe created with the pipeline processor.
+        /// </remark>
+        internal void CallDoCompleteForExpression()
+        {
+            // The pipe returned from 'GetRedirectionPipe' could be a NullPipe
+            if (PipelineProcessor != null)
+            {
+                PipelineProcessor.DoComplete();
+            }
+        }
+
         private bool _disposed;
 
         public void Dispose()

test/powershell/Language/Parser/RedirectionOperator.Tests.ps1

@@ -92,3 +92,33 @@ 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" -Tags CI {
+    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 | ForEach-Object { $_.Trim() }
+        $errorContent = $errorContent -join ""
+        $errorContent | Should Match "CommandNotFoundException,Microsoft.PowerShell.Commands.GetCommandCommand"
+    }
+}