Make Parameters property of CommandInfo thread safe#12865
Make Parameters property of CommandInfo thread safe#12865bergmeister wants to merge 1 commit intoPowerShell:masterfrom
Conversation
src/System.Management.Automation/engine/MergedCommandParameterMetadata.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/MergedCommandParameterMetadata.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/MergedCommandParameterMetadata.cs
Show resolved
Hide resolved
|
Part of the problem is that A simplish fix would be for PSSA to maintain it's own As for fixing the problem in PowerShell itself, I think fixing #4003 is the only option. That said, there's a reason it's sat open for almost three years, it's difficult to solve without a probably sizable (but sort of unknowable) break. |
|
@SeeminglyScience Thanks for the valuable input and specific references. :-) |
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
|
Closing as PSSA implemented the workarouns to catch the exception when it happens for a command from the cache and just ask for a fresh new object. |
PR Summary
Fixes PowerShell/PSScriptAnalyzer#1516
PR Context
PSSA is multi-threaded and keeps a cache of
CommandInfoobjects. Those objects might be accessed concurrently and PowerShell threw exceptions, see my issue comment for more details. I attached the debugger toPowerShell, I looked deeper and it seems the culprit is theBindableParametersproperty ofMergedCommandParameterMetadata. Therfore the easy (but wrong) fix is to make it use a thread safe dictionary. I am aware that Jason @lzybkr previously said in a similar PR (but different area - the module system) that most of the PS engine deliberately doesn't use locking. In this case, this proposed change is probably not the right change (and probably break other things as indicated by the failing tests) because the class calls.Clear()and.Add()internally for internal resetting/updating in various scenarios. This PR is rather the starting point of a discussion at which layer we can/should intervene to prevent such errors. Would a slim read lock around_bindableParametersbe a possible and acceptable solution?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.