Add CmdletInfo to Command Clone() #12301
Conversation
|
Will we fix #12295 here too? |
|
I can, but I wasn't sure if that change was worthy of making servicing. But I did think this one was worthy. |
There was a problem hiding this comment.
This looks good. Before I sign off, I want to directly ask if this introduces the possibility of cloning a command, running on a different thread and then corrupting command info state somehow due to thread safety issues? I've read through the discussion above, but want to get a better understanding of whether there are implications for leaking a CommandInfo reference with a clone, specifically if there's a behaviour that we're confident today is threadsafe and may not be in future
The answer is: yes, it could cause thread safe issue when using However, the public API Given that, I'm fine making this change, but hesitating if this change should be serviced (will leave that decision to @SteveL-MSFT) |
|
Just to clarify, I'm totally fine also adding a Most |
That won't solve the problem. You will have to create a script block because |
|
if this doesn't go back to servicing I'll maybe try to add #12295 to it. It'd be a shame to have the silly workaround in PSES for PS7 though... |
Right, that's what I was thinking and just wanted to check that my understanding was ok. In that case, I'm happy. |
Do you mean that API proposed in the PR will fixed too? |
@TylerLeonhardt Please don't. This PR has been reviewed and let's keep the scope as is. You can submit a new PR for that work. |
|
@iSazonov The issue can be exposed in other ways beyond those APIs. |
|
@PoshChan please retry macos |
|
@daxian-dbw, successfully started retry of |
|
🎉 Handy links: |
|
@iSazonov Please don't mark something for backport without discussing using the teams channel. |
|
@TravisEz13 I expect that "Consider" in "7.0.x-Servicing-Consider" says that the label is for the discussion. And "approved" in label is for already discussed commits. Yes? |
|
@iSazonov But, you have been marking issue that clearly don't need to be back ported and often we don't have much time to triage once a release has been declared. I though using the teams channel would be a good way to discuss the bar. On this one, it's for PSES and they don't need it back ported. |
|
I'm saying no to the backport because @daxian-dbw showed concern here |
PR Summary
fixes #12297
This adds copying
CmdletInfoto theCommandclass'sClone(). I also added a few tests for thePSCommandtype (more should be added for coverage as it seems that this type is not tested at all it seems...) which includes a test that covers the code change.PR Context
PowerShell Editor Services uses the
PSCommandtype everywhere. For whatever reason, there's anPowerShell.AddCommand(CmdletInfo cmdletInfo)but not aPSCommand.AddCommand(CmdletInfo cmdletInfo). To work around this, we have an extension method that does:PowerShell.AddCommand(new Command(cmdletInfo))... however, this exposed another bug as we take thePSCommandtype and set it directly on thePowerShellinstance:This would trigger cmdlet discovery for commands regardless of if they had a
CmdletInfoassociated with it because deep down, theCmdletInfoproperty on the underlyingCommandobject would not get cloned...This PR makes sure it also gets cloned. Take a look at the very last test added which fails before this change, but passes after it.
PSES could take advantage of this if it was backported to PS 7.0. In the meantime, we have to copy commands manually... (naturally this is what we will have to do for Windows PowerShell... but for PS7 we can use this.
cc @SeeminglyScience
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.