Fix ForEach-Object -Parallel when passing in script block variable #16564

PaulHigin · 2021-12-07T01:04:04Z

PR Summary

This change fixes #16445, where using variables were not being evaluated correctly when the -Parallel script block is passed in as a variable.

PR Context

ForeEach-Object -Parallel using variable processing walks the script block AST to determine if the using variable was defined in the current scope, and does this by tracking the number of 'ForEach-Object -Parallel' commands in the call chain.
However, the provided script block is different, depending on whether it is passed in as a variable or via parameter binding.

$myScript = '"Hello!"'
1 | ForEach-Object -Parallel ([scriptblock]::Create($myScript))
# Script block Ast does not contain the 'ForEach-Object' command in the call chain

1 | ForEach-Object -Parallel { "Hello!" }
# Script block Ast does contain the 'ForEach-Object' command in the call chain

This change takes into account the two forms of script blocks

PR Checklist

src/System.Management.Automation/engine/hostifaces/PSTask.cs

src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs

…owerShell.cs Co-authored-by: Ilya <darpa@yandex.ru>

…gin/PowerShell into fix-foreachparallel-using

iSazonov · 2021-12-08T05:21:30Z

Could you please clarify why do we need to support $using if Foreach-Object execute the script block in current scope.

PaulHigin · 2021-12-08T16:53:37Z

@iSazonov The $using keyword is only supported for ForEach-Object and the -Parallel parameter set, in which case the script block is executed in multiple runspaces in parallel. Since runspaces are isolation units, the $using keyword is needed to pass in variables from the current session to each parallel runspace sessions, similar to how it is used for remoting. Otherwise they would need to be passed in via the argument list.

iSazonov · 2021-12-08T17:37:15Z

Do you mean Foreach-Object execute the scriptblock in current scope and Foreach-Object execute the scriptblock in another scope?

If so, we got a very confusing UX. :-(
Ex.: an user has large scriptblock, add -Parallel switch and now the scriptblock doesn't work.

PaulHigin · 2021-12-08T17:52:18Z

?? This is all discussed in the RFC created for this feature.

PowerShell/PowerShell-RFC#194

iSazonov · 2021-12-08T18:03:03Z

?? This is all discussed in the RFC created for this feature.

PowerShell/PowerShell-RFC#194

:-) Yes, and I objected to this $using support. I believe that the feedback now clearly shows the UX and performance issues. We should consider designing a new cmdlet to address these requests.

PaulHigin · 2021-12-10T16:44:14Z

@daxian-dbw Please review.

src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs

daxian-dbw · 2021-12-10T18:50:16Z

@iSazonov ForEach-Object -Parallel runs the given script block in a separate Runspsace, so of course the script block will not run in the current scope.

daxian-dbw · 2021-12-10T18:51:17Z

@PaulHigin Can you please also review #12378 and see if that can be closed after your fix.

…lock object

PaulHigin · 2021-12-10T20:06:59Z

@daxian-dbw #12378 is a different issue.

src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs

PaulHigin · 2021-12-14T00:33:38Z

@daxian-dbw Thanks for the review!

…owerShell#16564) ForeEach-Object -Parallel using variable processing walks the script block AST to determine if the using variable was defined in the current scope, and does this by tracking the number of 'ForEach-Object -Parallel' commands in the call chain. However, the provided script block is different, depending on whether it is passed in as a variable or via parameter binding. $myScript = '"Hello!"' 1 | ForEach-Object -Parallel ([scriptblock]::Create($myScript)) # Script block Ast does not contain the 'ForEach-Object' command in the call chain 1 | ForEach-Object -Parallel { "Hello!" } # Script block Ast does contain the 'ForEach-Object' command in the call chain This change takes into account the two forms of script blocks

ghost · 2022-02-24T23:32:58Z

🎉v7.3.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

TravisEz13 · 2022-03-02T19:44:14Z

@PowerShell/powershell-maintainers
We only take security fixes and regression to LTS. This is appropriate for 7.3. Rejecting back-port.

Allow some bake time and reconsider to verify that this is stable.

SeeminglyScience · 2022-03-14T23:31:07Z

@TravisEz13 Just to clarify this one was actually a regression (worked in 7.1.5, broke in 7.2, thank you @jborean93). Unsure if that was discussed in the meeting but if not, it may be worth another pass.

PaulHigin · 2022-03-29T19:57:35Z

The progressive changes include not only the regression, but also new behavior. So I feel backporting this to LTS branches is risky, and that we should at least wait for it to bake longer in preview versions.

PaulHigin · 2022-06-23T21:25:59Z

We should reconsider backporting this as it appears to be affecting users of 7.2.x.

#17571

adityapatwardhan · 2022-08-02T19:52:28Z

/backport to release/v7.2.6

github-actions · 2022-08-02T19:52:44Z

Started backporting to release/v7.2.6: https://github.com/PowerShell/PowerShell/actions/runs/2784832290

GitHub
Fix ForEach-Object -Parallel when passing in script block variable · PowerShell/PowerShell@0e4d2e4
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

github-actions · 2022-08-02T19:52:59Z

@adityapatwardhan backporting to release/v7.2.6 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: First try at fix
Applying: Fix scope function to correctly account for provided script block type.
Applying: Fix tests
Applying: Update src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs
Applying: PR comments
.git/rebase-apply/patch:26: trailing whitespace.
            
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs
CONFLICT (content): Merge conflict in src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 PR comments
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

iSazonov · 2022-08-05T02:57:01Z

@adityapatwardhan Friendly ping!

adityapatwardhan · 2022-08-05T04:46:30Z

This is in the backlog.. Currently working on fixing the build break in 7.2.6

adityapatwardhan · 2022-08-08T19:30:19Z

/backport to release/v7.2.6

adityapatwardhan · 2022-08-08T19:31:16Z

/backport to release/v7.2.6

…owerShell#16564) ForeEach-Object -Parallel using variable processing walks the script block AST to determine if the using variable was defined in the current scope, and does this by tracking the number of 'ForEach-Object -Parallel' commands in the call chain. However, the provided script block is different, depending on whether it is passed in as a variable or via parameter binding. $myScript = '"Hello!"' 1 | ForEach-Object -Parallel ([scriptblock]::Create($myScript)) # Script block Ast does not contain the 'ForEach-Object' command in the call chain 1 | ForEach-Object -Parallel { "Hello!" } # Script block Ast does contain the 'ForEach-Object' command in the call chain This change takes into account the two forms of script blocks

ghost · 2022-08-11T22:19:13Z

🎉v7.2.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

…16564) (#17860)

PaulHigin added 3 commits December 3, 2021 16:07

First try at fix

a012afb

Fix scope function to correctly account for provided script block type.

7e0d731

Fix tests

22d2b87

ghost assigned iSazonov Dec 7, 2021

pull-request-quantifier-deprecated bot added the Small label Dec 7, 2021

PaulHigin requested review from daxian-dbw and powercode December 7, 2021 01:04

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 7, 2021

iSazonov reviewed Dec 7, 2021

View reviewed changes

src/System.Management.Automation/engine/hostifaces/PSTask.cs Outdated Show resolved Hide resolved

iSazonov reviewed Dec 7, 2021

View reviewed changes

src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs Outdated Show resolved Hide resolved

PaulHigin and others added 3 commits December 7, 2021 10:11

Update src/System.Management.Automation/engine/runtime/ScriptBlockToP…

afa9120

…owerShell.cs Co-authored-by: Ilya <darpa@yandex.ru>

PR comments

b76969a

Merge branch 'fix-foreachparallel-using' of https://github.com/PaulHi…

d4aecb8

…gin/PowerShell into fix-foreachparallel-using

daxian-dbw added the Backport-7.2.x-Consider label Dec 7, 2021

daxian-dbw requested changes Dec 10, 2021

View reviewed changes

src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs Outdated Show resolved Hide resolved

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 10, 2021

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 10, 2021

Fix using scope check to account for other type of passed in script b…

2ae7a3f

…lock object

pull-request-quantifier-deprecated bot added Medium and removed Small labels Dec 10, 2021

daxian-dbw requested changes Dec 10, 2021

View reviewed changes

src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs Outdated Show resolved Hide resolved

src/System.Management.Automation/engine/runtime/ScriptBlockToPowerShell.cs Outdated Show resolved Hide resolved

iSazonov merged commit cadd576 into PowerShell:master Dec 14, 2021

segraef mentioned this pull request Feb 17, 2022

ForEach-Object -Parallel disallows script blocks via $using:, unlike Start-ThreadJob #12378

Closed

ghost mentioned this pull request Feb 24, 2022

ForEach-Object -Parallel does not accept a scriptblock variable containing a $using #16445

Closed

5 tasks

TravisEz13 removed the Backport-7.2.x-Consider label Mar 29, 2022

PaulHigin added the Backport-7.2.x-Consider label Jun 23, 2022

adityapatwardhan added Backport-7.2.x-Approved and removed Backport-7.2.x-Consider labels Aug 2, 2022

adityapatwardhan mentioned this pull request Aug 8, 2022

Fix ForEach-Object -Parallel when passing in script block variable (#16564) #17860

Merged

22 tasks

adityapatwardhan added Backport-7.2.x-Done and removed Backport-7.2.x-Approved labels Aug 8, 2022

adityapatwardhan added a commit that referenced this pull request Aug 12, 2022

Fix ForEach-Object -Parallel when passing in script block variable (#…

f153a64

…16564) (#17860)

DarkLite1 mentioned this pull request Mar 12, 2024

ForEach-Object -Parallel does not accept a scriptblock variable containing a $using #21332

Open

5 tasks

TravisEz13 mentioned this pull request Apr 16, 2024

WIP: add more detail to suspicious scriptblock logging #21475

Closed

TravisEz13 mentioned this pull request Jul 9, 2024

Add script to update SDK for release branches #24034

Merged

Fix ForEach-Object -Parallel when passing in script block variable #16564

Fix ForEach-Object -Parallel when passing in script block variable #16564

Uh oh!

Conversation

PaulHigin commented Dec 7, 2021 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Dec 8, 2021

Uh oh!

PaulHigin commented Dec 8, 2021

Uh oh!

iSazonov commented Dec 8, 2021

Uh oh!

PaulHigin commented Dec 8, 2021

Uh oh!

iSazonov commented Dec 8, 2021

Uh oh!

PaulHigin commented Dec 10, 2021

Uh oh!

Uh oh!

daxian-dbw commented Dec 10, 2021

Uh oh!

daxian-dbw commented Dec 10, 2021

Uh oh!

PaulHigin commented Dec 10, 2021

Uh oh!

Uh oh!

Uh oh!

PaulHigin commented Dec 14, 2021

Uh oh!

ghost commented Feb 24, 2022

Uh oh!

TravisEz13 commented Mar 2, 2022

Uh oh!

SeeminglyScience commented Mar 14, 2022

Uh oh!

PaulHigin commented Mar 29, 2022

Uh oh!

PaulHigin commented Jun 23, 2022

Uh oh!

adityapatwardhan commented Aug 2, 2022

Uh oh!

github-actions bot commented Aug 2, 2022 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 2, 2022

Uh oh!

iSazonov commented Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityapatwardhan commented Aug 5, 2022

Uh oh!

adityapatwardhan commented Aug 8, 2022

Uh oh!

adityapatwardhan commented Aug 8, 2022

Uh oh!

ghost commented Aug 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PaulHigin commented Dec 7, 2021 •

edited by daxian-dbw

Loading

github-actions bot commented Aug 2, 2022 •

edited by unfurl-links bot

Loading

iSazonov commented Aug 5, 2022 •

edited

Loading