Support backgrounding pipelines with ampersand by BrucePay · Pull Request #3360 · PowerShell/PowerShell

BrucePay · 2017-03-17T20:10:33Z

This implements support for backgrounding pipelines with &. Putting & at the end of a pipeline will cause the pipeline to be run as a PowerShell job. When a pipeline is backgrounded a job object is returned. Once the pipeline is running as a job, all of the normal job cmdlets can be used to manage the job. Variables (ignoring process-specific variables) used in the pipeline are automatically copied to the job so

copy $foo $bar &

just works. The job is also run in the current directory instead of the user's home directory as is the case with Start-Job.

Addresses #716

lzybkr · 2017-03-17T20:13:24Z

Please rebase on master so commits that aren't yours go away.

lzybkr · 2017-03-18T00:23:38Z

The api is public so this is a breaking change. We need to introduce a new constructor.

Should we maybe inherit another class from PipelineAst instead? Something like BackgroundPipelineAst.

I think a new Ast is too much.

lzybkr · 2017-03-18T00:24:03Z

Another public api - so this is breaking as well.

lzybkr · 2017-03-18T00:28:08Z

I see no tests to cover this case - are you sure it's correct?

A test would look like:

1..10 | % { $_ } &

I'll add the proposed test (which does pass).

lzybkr · 2017-03-18T00:28:56Z

Use Utils.EmptyArray<Expression>() instead of new Expression[0].

lzybkr · 2017-03-18T00:31:37Z

Are you sure the expression is evaluated only once and in the background.

From my quick review, I'm concerned it's evaluated twice, once in the foreground (and thrown away), and then again in the background.

Something like $(Sleep -Seconds 10; 2+3) & would make it obvious I think.

I've verified that it only executes once in the background task.

lzybkr · 2017-03-18T00:32:55Z

We should monitor how long this test takes, maybe move it to Nightly before merging if it's too slow from creating too many background jobs.

It takes > 10 s so nightly is probably right.

lzybkr · 2017-03-18T00:36:42Z

You generally don't want to use Extent.Text except for error messages or other ui purposes, it might include more than you think, e.g. ${pid} - it includes the braces.

Instead, you should use the VariablePath property.

lzybkr · 2017-03-18T00:37:39Z

This regex won't work if a scope was used, like global:pid. You probably also want to skip constants like $null, $true, and $false.

lzybkr · 2017-03-18T00:40:14Z

This is pretty inefficient in that it creates a bunch of strings.

It would be better to start with a copy of the script block body in a StringBuilder and insert using as appropriate.

lzybkr · 2017-03-18T00:41:56Z

If something fails in starting the job, you won't report the actual position of the background invocation.
I think you probably also want to ensure we call PowerShell's Start-Job, not any random command called Start-Job.

I'm changing it to report the pipeline extent.

lzybkr · 2017-03-18T00:44:45Z

I think this would be cleaner to just build the CommandProcessor directly instead of creating an Ast to build the CommandProcessor.

I think of the Ast as mapping directly to source as much as possible, and this use of the Ast doesn't seem necessary.

lzybkr

We definitely need to fix the breaking change and have more tests.

It feels like we already have similar code rewriting the script block, but I can't recall where. If you do, it would be nice to unify that code if possible.

lzybkr · 2017-03-21T16:16:47Z

@BrucePay - please fix the build break in appveyor - it looks like it catches a breaking change in the api.

iSazonov · 2017-03-22T03:07:41Z

What is 'cd' here? Alias? If so we should use full name.

iSazonov · 2017-03-22T03:18:16Z

This title is difficult to understand. Maybe better "Background job don't overwrite the parent process 'Pid'"

Reworded for clarity.

iSazonov · 2017-03-22T03:19:43Z

The same about title

Reworded for clarity.

iSazonov · 2017-03-22T03:24:29Z

Please use "Should Not BeExactly".

iSazonov · 2017-03-22T03:24:43Z

Please use "Should Not BeExactly".

iSazonov · 2017-03-22T03:25:35Z

Extra line.

iSazonov · 2017-03-25T16:33:55Z

Please replace alias with full name. Below the alias is used too.

iSazonov · 2017-03-25T16:34:59Z

Please replace alias foreach.

iSazonov · 2017-03-25T16:40:36Z

Please replace alias and add parameter names.

Also please correct register Should Be here and above -Wait, Write-Output, titles and so on.

Please recheck - I dont see the fixes.

Still open.

lzybkr · 2017-03-26T04:12:18Z

Just a general comment about the design - do we have a good feel for how:

people use & today?
how we think they'll use it in the future?

There was the hypothesis that & is mostly used to launch windowed applications. If this is true, launching PowerShell to launch the application will feel slow because launching PowerShell is not fast. If we could avoid launching PowerShell first, that would be ideal, though that can only work if the pipeline is only running external commands.

On the other hand, if the primary scenario really is background jobs, then this PR will be a great syntactic shortcut.

BrucePay · 2017-03-28T23:11:04Z

The primary scenario really is to background pipelines. The need to launch blocking apps was simply a forcing function.

lzybkr · 2017-04-02T02:17:50Z

I don't see why you couldn't use pipelineElementAsts[0].Parent here instead of parsing the pipeline again.

I need the actual offsets in the string to do the variable substitution. The parent AST will have the offsets in the parent script not the scriptblock.

Isn't that as simple as subtracting pipeElementAsts[0].Parent.Extent.StartOffset ?

lzybkr · 2017-04-02T02:19:04Z

You might as well use scriptblockBodyString.Length + length of the set-location text as the initial capacity to minimize allocations.

Since I'm inserting $using: for each variable, scriptblockBody.Length is guaranteed to be too short forcing an allocation.

Yes, but StringBuilder starts with 16 characters and adds chunks by the larger of how much you are appending and how big the buffer is already.

With many small appends, you'll probably end up with multiple chunks, whereas starting with the minimum needed will result in probably 1 extra allocation, or you could just double the minimum needed if it's "small" (say less than 40kb or so) to avoid a single allocation going into the LOH.

lzybkr · 2017-04-02T02:24:20Z

I don't have a good feeling about not copying any variables starting with PS. I understand the intent, but I confidently predict it will trip up some folks and their scenario will seem perfectly reasonable.

lzybkr · 2017-04-02T02:25:32Z

You can remove the resource string - this is the only reference.

JamesWTruher · 2017-04-05T22:53:43Z

TokenKind [](start = 29, length = 9)

what do the following emit?
$A = "bar" & "foo"
or
$A = $("bar" & echo "foo")
and we should probably have tests for them

& is a statement separator like in ksh so in your examples, you'd get an array of two objects: the job object and "foo".

lzybkr · 2017-04-20T20:46:08Z

This comment has not been addressed.

This should be fixed now.

iSazonov · 2017-04-24T07:27:18Z

We can use C# 7.0 pattern here.

We aren't targeting C# 7.

We are now, but you didn't change this code, just indented it, so don't bother changing it now.

iSazonov · 2017-04-24T07:29:17Z

Maybe "BeExactly" ?

iSazonov · 2017-04-24T07:30:10Z

Maybe "BeExactly" ?

iSazonov · 2017-04-24T07:32:48Z

Please use Receive-Job $j -Wait as above.

This is unnecessary churn.

iSazonov · 2017-04-24T07:33:52Z

Still open.

…he end of a pipeline will cause the pipeline to be run as a PowerShell job. When a pipeline is backgrounded a job object is returned. Once the pipeline is running as a job, all of the normal job cmdlets can be used to manage the job. Variables (ignoring process-specific variables) used in the pipeline are automatically copied to the job so copy $foo $bar & just works. The job is also run in the current directory instead of the user's home directory as is the case with Start-Job.Implement Marked background jobs as SLOW Removed call to Parser.Parse in processing the background job string

lzybkr · 2017-05-16T05:45:50Z

-                    firstCommandExpr != null ? ExpressionCache.FalseConstant : ExpressionCache.TrueConstant,
-                    Expression.NewArrayInit(typeof(CommandParameterInternal[]), pipelineExprs),
+                    firstCommandExpr != null && !pipelineAst.BackgroundProcess ? ExpressionCache.FalseConstant : ExpressionCache.TrueConstant,
+                    Expression.NewArrayInit(typeof(CommandParameterInternal[]), 


After looking more closely, I wonder if it makes sense to avoid basically this whole method (Compiler.VisitPipeline) and generate code to call PipelineOps.InvokePipeline directly.

Of particular concern - the pipelineExprs are ignored - so they shouldn't be generated in the first place. You never know what sort of side effects generating those expressions might have, it's better to avoid that entirely.

Refactored the code.

lzybkr · 2017-05-16T05:46:41Z

+                    Expression.NewArrayInit(typeof(CommandParameterInternal[]), 
+                        !pipelineAst.BackgroundProcess ? pipelineExprs : Utils.EmptyArray<Expression>()),
                    Expression.Constant(pipeElementAsts),
                    redirectionExpr,


I don't see any tests for redirections. I assume the redirections are also done in the background, and hence should not be generated in the foreground.

Added tests for redirection.

lzybkr · 2017-05-16T05:48:53Z

+        /// <exception cref="PSArgumentException">
+        /// If <paramref name="pipelineElements"/> is null or is an empty collection.
+        /// </exception>
+        public PipelineAst(IScriptExtent extent, IEnumerable<CommandBaseAst> pipelineElements) :this (extent, pipelineElements, false)


backgroundProcess: false

lzybkr · 2017-05-16T05:49:00Z

+        /// <exception cref="PSArgumentNullException">
+        /// If <paramref name="extent"/> or <paramref name="commandAst"/> is null.
+        /// </exception>
+        public PipelineAst(IScriptExtent extent, CommandBaseAst commandAst) :this (extent, commandAst, false)


backgroundProcess: false

lzybkr · 2017-05-16T05:49:25Z


-            var pipeLineAst = new PipelineAst(this.Extent, cmdAst);
+            var pipeLineAst = new PipelineAst(this.Extent, cmdAst, false);
            var funcStatements = ConfigurationExtraParameterStatements.Select(statement => (StatementAst)statement.Copy()).ToList();


backgroundProcess: false

lzybkr · 2017-05-16T05:49:30Z


-            var returnPipelineAst = new PipelineAst(this.Extent, setItemCmdlet);
+            var returnPipelineAst = new PipelineAst(this.Extent, setItemCmdlet, false);



backgroundProcess: false

lzybkr · 2017-05-16T05:49:34Z

            cmdAst.DefiningKeyword = Keyword;
-            _commandCallPipelineAst = new PipelineAst(FunctionName.Extent, cmdAst);
+            _commandCallPipelineAst = new PipelineAst(FunctionName.Extent, cmdAst, false);
            return _commandCallPipelineAst;


backgroundProcess: false

lzybkr · 2017-05-16T05:53:36Z

We are now, but you didn't change this code, just indented it, so don't bother changing it now.

lzybkr · 2017-05-16T05:54:43Z


-                var cmdletInfo = commandProcessor?.CommandInfo as CmdletInfo;
-                if (cmdletInfo?.ImplementingType == typeof(OutNullCommand))
+                var pipelineAst = (PipelineAst)pipeElementAsts[0].Parent;


After thinking more about the code in Compiler.VisitPipeline and looking at this code, I think it makes sense to add a new method InvokePipelineInBackground that accepts the PipelineAst and avoid touching this method completely. They really are very different operations.

I've refactored the code as suggested.

lzybkr · 2017-05-16T05:58:18Z

+                        {
+                            updatedScriptblock.Append(scriptblockBodyString.Substring(position, v.Extent.StartOffset - pipelineOffset - position));
+                            updatedScriptblock.Append("${using:");
+                            updatedScriptblock.Append(vName);


Annoying corner case - you need to escape } in vName. CodeGeneration.EscapeVariableName will handle that for you.

…on tests

SteveL-MSFT · 2017-05-18T19:25:19Z

@lzybkr it looks like all the feedback has been addressed?

lzybkr · 2017-05-18T19:37:22Z

-                    redirectionExpr,
-                    _functionContext);
+                Expression invokePipe;
+                if (pipelineAst.BackgroundProcess)


I was thinking this would go at the top of this method to avoid needlessly compiling any of the body. That would also eliminate the need for some of the tests that were added above. Is that possible?

lzybkr · 2017-05-18T19:39:17Z

        }

+        internal static void InvokePipelineInBackground(
+                                            CommandBaseAst[] pipeElementAsts,


You can pass the PipelineAst instead of the elements because that's all you use here.

lzybkr · 2017-05-18T23:26:03Z

+                var firstCommandExpr = (pipeElements[0] as CommandExpressionAst);
+    
+                if (firstCommandExpr != null && pipeElements.Count == 1 && ! pipelineAst.BackgroundProcess)
                {


You can revert this change now - it can't be a background process.

lzybkr · 2017-05-18T23:26:11Z

+                    int i, commandsInPipe;
+
+                    if (firstCommandExpr != null  && !pipelineAst.BackgroundProcess)
+                    {


You can revert this change now - it can't be a background process.

lzybkr · 2017-05-18T23:28:20Z

+        /// <summary>
+        /// Indicates that this pipeline should be run in the background.
+        /// </summary>
+        public bool BackgroundProcess { get; private set; }


BackgroundProcess [](start = 20, length = 17)

Maybe the property should not include Process, e.g. if in the future, we support in-process jobs, then it's just in the background, not necessarily in another process.

Change BackgroundProcess to just be Background.

lzybkr · 2017-05-18T23:28:51Z

            cmdAst.DefiningKeyword = Keyword;
-            _commandCallPipelineAst = new PipelineAst(FunctionName.Extent, cmdAst);
+            _commandCallPipelineAst = new PipelineAst(FunctionName.Extent, cmdAst, false);
            return _commandCallPipelineAst;


false [](start = 83, length = 5)

backgroundProcess: false

lzybkr · 2017-05-18T23:29:09Z

                        endErrorStatement = fileNameExpr.Extent;
                        condition = new PipelineAst(fileNameExpr.Extent,
-                                                    new CommandExpressionAst(fileNameExpr.Extent, fileNameExpr, null));
+                                                    new CommandExpressionAst(fileNameExpr.Extent, fileNameExpr, null), false);


false [](start = 119, length = 5)

backgroundProcess: false

BrucePay assigned lzybkr Mar 17, 2017

BrucePay requested review from JamesWTruher and lzybkr March 17, 2017 20:10

msftclas added the cla-not-required label Mar 17, 2017

SteveL-MSFT changed the title ~~Brucepay ampersand~~ Support backgrounding pipelines with ampersand Mar 17, 2017

BrucePay force-pushed the brucepay-ampersand branch from 81b84fb to 564cc3f Compare March 17, 2017 23:07

lzybkr reviewed Mar 18, 2017

View reviewed changes

lzybkr suggested changes Mar 18, 2017

View reviewed changes

BrucePay force-pushed the brucepay-ampersand branch from b4d2822 to 30642fc Compare March 21, 2017 23:17

iSazonov reviewed Mar 22, 2017

View reviewed changes

iSazonov added the Review - Waiting on Author label Mar 23, 2017

BrucePay force-pushed the brucepay-ampersand branch from 30642fc to 80e4547 Compare March 24, 2017 20:25

iSazonov added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Mar 25, 2017

iSazonov reviewed Mar 25, 2017

View reviewed changes

lzybkr suggested changes Apr 2, 2017

View reviewed changes

JamesWTruher reviewed Apr 5, 2017

View reviewed changes

BrucePay force-pushed the brucepay-ampersand branch 2 times, most recently from 379aff2 to 06ae76d Compare April 18, 2017 22:51

lzybkr reviewed Apr 20, 2017

View reviewed changes

BrucePay force-pushed the brucepay-ampersand branch from 8905083 to 80041b2 Compare April 23, 2017 23:53

iSazonov reviewed Apr 24, 2017

View reviewed changes

SteveL-MSFT added this to the 6.0.0-beta1 milestone May 1, 2017

SteveL-MSFT modified the milestones: 6.0.0-beta, 6.0.0-beta1 May 10, 2017

BrucePay force-pushed the brucepay-ampersand branch from 80041b2 to 52b8460 Compare May 15, 2017 18:47

Removed duplicate tests, minor cleanups

eaa69a1

lzybkr suggested changes May 16, 2017

View reviewed changes

Refactored background pipeline code into two methods, added redirecti…

a5e49f2

…on tests

lzybkr reviewed May 18, 2017

View reviewed changes

cleanup of background process code

0f21b5f

lzybkr reviewed May 18, 2017

View reviewed changes

renamed backgroundProcess to just background

4c67db1

lzybkr approved these changes May 19, 2017

View reviewed changes

lzybkr merged commit dfb3866 into PowerShell:master May 19, 2017

BrucePay mentioned this pull request May 19, 2017

Implement the use of & to background a command #716

Closed

BrucePay deleted the brucepay-ampersand branch June 21, 2018 21:59

joeyaiello mentioned this pull request Oct 29, 2018

Docs needed for the Backgrounding of jobs using the postpositional & (ampersand control operator) MicrosoftDocs/PowerShell-Docs#3094

Closed


		var returnPipelineAst = new PipelineAst(this.Extent, setItemCmdlet);
		var returnPipelineAst = new PipelineAst(this.Extent, setItemCmdlet, false);

Conversation

BrucePay commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lzybkr commented Mar 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

lzybkr commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

BrucePay commented Mar 17, 2017 •

edited

Loading