Skip to content

Make Parameters property of CommandInfo thread safe#12865

Closed
bergmeister wants to merge 1 commit intoPowerShell:masterfrom
bergmeister:CommandInfo_Parameters_ThreadSafety
Closed

Make Parameters property of CommandInfo thread safe#12865
bergmeister wants to merge 1 commit intoPowerShell:masterfrom
bergmeister:CommandInfo_Parameters_ThreadSafety

Conversation

@bergmeister
Copy link
Copy Markdown
Contributor

@bergmeister bergmeister commented Jun 1, 2020

PR Summary

Fixes PowerShell/PSScriptAnalyzer#1516

PR Context

PSSA is multi-threaded and keeps a cache of CommandInfo objects. Those objects might be accessed concurrently and PowerShell threw exceptions, see my issue comment for more details. I attached the debugger to PowerShell, I looked deeper and it seems the culprit is the BindableParameters property of MergedCommandParameterMetadata. 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 _bindableParameters be a possible and acceptable solution?

PR Checklist

@ghost ghost assigned anmenaga Jun 1, 2020
@SeeminglyScience
Copy link
Copy Markdown
Contributor

Part of the problem is that GetMergedCommandParameterMetadataSafely isn't actually safe due to #4003. Since getting parameter metadata can cause PowerShell code to be invoked (dynamic params), if you just make the dictionary itself thread safe you're more than likely going to see a lot more harder to track state corruption.

A simplish fix would be for PSSA to maintain it's own ConcurrentDictionary<CommandInfo, Dictionary<string, ParameterMetadata>>. Though iirc that API doesn't actually ensure that two "create" delegates won't run concurrently, just that the actual dictionary add won't. If that's correct then you'd probably have to roll your own version with a lock.

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.

@bergmeister
Copy link
Copy Markdown
Contributor Author

bergmeister commented Jun 12, 2020

@SeeminglyScience Thanks for the valuable input and specific references. :-)
I remember talking to @daxian-dbw about this issue in a general way where it happened in a different scenario (Test-ModuleManifest was not thread safe and could be fixed in PSSA by limiting its concurrency in PowerShell/PSScriptAnalyzer#1258). In this scenario the parameters property comes from the CommandInfo cache of PSSA where limiting concurrency is not only hard from a consumers point of view but also comes with performance implications (a lot of the perf improvements in recent version were done by getting rid of fat locks and using .Net's built in and thread safe ConcurrentDictionary)...
A suggestion was also to try to force evaluation of the property in the runspace that originally created it. I tried to call the .Parameters property right after Get-Command gets queried the first time and do something with so that the compiler doesn't optimize the call away and this did not help. All I can think of now is to add the parameters to the cache as well (bad in terms of memory usage) or not use the cache in this instance.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 16, 2020
@ghost ghost added the Stale label Jul 1, 2020
@ghost
Copy link
Copy Markdown

ghost commented Jul 1, 2020

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.

@bergmeister
Copy link
Copy Markdown
Contributor Author

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.
Since the other referenced PowerShell issue where the root cause lies, is still open, I'll close the PR then

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.

Collection was modified; enumeration operation may not execute when running Invoke-ScriptAnalyzer

4 participants