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

[WIP] Fix using variable error for nested foreach parallel by PaulHigin · Pull Request #11829 · PowerShell/PowerShell · GitHub

[WIP] Fix using variable error for nested foreach parallel - #11829

Closed
Paul Higinbotham (PaulHigin) wants to merge 9 commits into
PowerShell:masterfrom
PaulHigin:fix-foreachparallel-using
Closed

[WIP] Fix using variable error for nested foreach parallel#11829
Paul Higinbotham (PaulHigin) wants to merge 9 commits into
PowerShell:masterfrom
PaulHigin:fix-foreachparallel-using

Conversation

Paul Higinbotham (PaulHigin) commented Feb 11, 2020
edited
Loading

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
Collaborator

Paul Higinbotham (@PaulHigin) would this fix also affect the use of nested iterations of Start-ThreadJob and similar $using: variable usage there?

…owerShell.cs

Co-Authored-By: Ilya <darpa@yandex.ru>
…owerShell.cs

Co-Authored-By: Ilya <darpa@yandex.ru>
…owerShell.cs

Co-Authored-By: Ilya <darpa@yandex.ru>
…owerShell.cs

Co-Authored-By: Ilya <darpa@yandex.ru>

Copy link
Copy Markdown
Contributor Author

Rain Sallow (/u/ta11ow) (@vexx32) No, ThreadJob will also need to change. Please create an issue for it.

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Feb 12, 2020
Paul Higinbotham (PaulHigin) changed the title Fix using variable error for nested foreach parallel [WIP] Fix using variable error for nested foreach parallel Feb 13, 2020

Copy link
Copy Markdown
Collaborator

Just double checked it, and looks like Start-ThreadJob doesn't exhibit the same issue, interestingly enough. 🎉

Copy link
Copy Markdown

Will this be added to RC3?

Copy link
Copy Markdown
Member

Przemysław Kłys (@PrzemyslawKlys) I don't think so, as the RC3 is already locked down and we have a very high bar for GA. But this fix will be included in a servicing release later.

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.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

Copy link
Copy Markdown

What is the status of this PR?

Copy link
Copy Markdown
Contributor Author

AFAIK there is no reason not to take this PR. Dongbo Wang (@daxian-dbw) can we include this in the next preview release?

Copy link
Copy Markdown
Collaborator

Paul Higinbotham (@PaulHigin) Please rebase to pass CIs.

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

Copy link
Copy Markdown
Contributor Author

Closing this PR since it is so old, and created a new one (#14548)

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL