Skip to content

Function 'GetCommandInfo' does not add CommandInfo to cache #1030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Blackbaud-ShaydeNofziger opened this issue Jun 28, 2018 · 4 comments
Open

Comments

@Blackbaud-ShaydeNofziger
Copy link
Contributor

Blackbaud-ShaydeNofziger commented Jun 28, 2018

In Helper.cs, the function GetCommandInfo checks the commandInfoCache for a given command, and - if it does not exist in the cache - retrieves the info from a powershell session. Unlike in the deprecated function GetCommandInfoLegacy, however the command info is never added to the commandInfoCache dictionary. (Compare lines 750 and 776)

I'd like to open a PR to correct this, assuming this is indeed a bug.

This should help marginally with performance issues due to contentions from the locks in the GetCommandInfo and GetCommandInfoLegacy functions.

@bergmeister
Copy link
Collaborator

@Blackbaud-ShaydeNofziger PR #953 already attempted to fix this but we struggled due to concurrency/locking bugs in the existing code base.
This new-ish method was being added for a new function where we could not populate the cache due to those bugs.
We would appreciate it if you can come up with a solution where all tests still pass. Feel free to open a WIP PR so that you get test results against all systems to allow you to get easy and quick feedback but from my experience just getting it in Windows PowerShell 5.1 to work was hard enough and it seemed that resolving the locking issues (locks are being held too long and in inappropriate places) would probably help tackle this issue.

@Blackbaud-ShaydeNofziger
Copy link
Contributor Author

@bergmeister Agreed that the locking issues are the primary hiccup here. Apologies for not seeing the existing PR. I would really like to help address the locking issues, so I'll dig into that.

@bergmeister
Copy link
Collaborator

Awesome, thanks, we really appreciate your help. Those concurrency issues in PSSA are really bad as they can also lead to sporadic failures (that I have even seen myself in the CI pipeline that I maintain at work)

@Blackbaud-ShaydeNofziger
Copy link
Contributor Author

😮 I thought it was just us getting those sporadic failures!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants