FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix using variable for nested foreach parallel calls by PaulHigin · Pull Request #14548 · PowerShell/PowerShell · GitHub

Fix using variable for nested foreach parallel calls - #14548

Merged
Andrew (anmenaga) merged 5 commits into
PowerShell:masterfrom
PaulHigin:fix-foreach-nested-using
Feb 2, 2021
Merged

Fix using variable for nested foreach parallel calls#14548
Andrew (anmenaga) merged 5 commits into
PowerShell:masterfrom
PaulHigin:fix-foreach-nested-using

Conversation

Copy link
Copy Markdown
Contributor

PR Summary

This fixes a bug in ForEach-Object -Parallel where a using variable in a nested ForEach-Object -Parallel throws an error even when the variable is defined in the correct scope (Issue #11817).

PR Context

This error was occurring because, when ForEach-Object -Parallel was assembling the user variable map, it was searching all nested scriptblocks within the ForEach scriptblock, with the result of finding nested using variables where the variable had not yet been defined. Since the nested scriptblock using variable had not been defined in the current scope, a mapping error was thrown.

Fix is to change the using variable map function to not search nested scriptblocks in the ForEach -Parallel case. This way foreach -parallel using variable mapping is always performed only for the current scope.

Many thanks to Michael Klement (@mklement0) for pointing out the fix.

PR Checklist

Copy link
Copy Markdown
Contributor Author

Dongbo Wang (@daxian-dbw) Can you please review these changes?

Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One comment, otherwise looks fine to me

ghost added the Review - Needed The PR is being reviewed label Jan 21, 2021

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Copy link
Copy Markdown

Restarted CIs for what looks like unrelated failures in Update-Help tests.

ghost removed the Review - Needed The PR is being reviewed label Jan 28, 2021

Copy link
Copy Markdown

Paul Higinbotham (@PaulHigin) it would be good to fix CodeFactor violations in the new code that is added; thank you.

Copy link
Copy Markdown

Restarted CIs again.

Copy link
Copy Markdown

One more time restarted CIs.

Andrew (anmenaga) merged commit 0039807 into PowerShell:master Feb 2, 2021

Copy link
Copy Markdown

🎉v7.2.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

Copy link
Copy Markdown
Member

Paul Higinbotham (@PaulHigin) - Should this be backported to 7.1.x?

Copy link
Copy Markdown
Contributor Author

Aditya Patwardhan (@adityapatwardhan) I think the impact of this change is small, since if involves nested foreach calls. So I would say it does not need to be backported as it probably does not affect a lot of users.

Copy link
Copy Markdown
Member

Removed the back port label as per comment from Paul Higinbotham (@PaulHigin)

Copy link
Copy Markdown

This is not unique to parallel foreach, it applies to nested start-job and nested invoke-command calls (double-hop) or any combination of those, like using invoke-command and then running start-job in the remote session, etc. Does this fix work for these scenarios as well (everywhere $using is supported) or was this somehow scoped to parallel foreach?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL