Skip to content
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

Change CommandInfoCache to implement IDisposable and clean up the runspace pool #1335

Merged
merged 8 commits into from
Sep 12, 2019

Conversation

JamesWTruher
Copy link
Member

PR Summary

The Helper is recreated with every invocation of Invoke-ScriptAnalyzer which essentially results in a runspace which was not cleaned up for every invocation. I saw 100s of runspaces (ia get-runspace) in my session. I also gave the CommandInfoCache it's own runspace pool and implemented IDisposable in it. Lastly, I've added a test to catch this in the future.

PR Checklist

The Helper is recreated with every invocation of Invoke-ScriptAnalyzer which essentially resulted in a runspace which was not cleaned up for every invocation.
I saw 100s of runspaces (ia get-runspace) in my session. I also gave the CommandInfoCache it's own runspace pool and implemented IDisposable in it.
Lastly, I've added a test to catch this in the future.
Engine/Helper.cs Outdated Show resolved Hide resolved
It no longer needs to be IDisposable
Implement dispose in cache along suggested guidelines
@bergmeister
Copy link
Collaborator

bergmeister commented Sep 12, 2019

The new test failed on WMF4 because the Get-Runspace Cmdlet is not available in that PS version. We could either exclude this PS version for that test (given the fix is ps version agnostic I think that would be OK) or maybe there is a lower level method that one can call as an alternative?

This fix shows up implicitly in the build times, which are quite a bit faster when compared to the ones of master (1 minute), and the WMF4 one even 6 minutes. I even remember being realizing that the WMF4 builds started to take quite a bit longer at some point but I just thought that AppVeyor had reduced the VM sizes due it being a legacy image.

I added a few suggestions and have just one question around the maximum number of runspaces to assert against but mostly this PR looks great.

JamesWTruher and others added 4 commits September 12, 2019 11:51
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, can be merged if the build is green (except for the Ubuntu build)

@JamesWTruher JamesWTruher changed the title Change helper to implement IDisposable and clean up the runspace pool Change CommandInfoCache to implement IDisposable and clean up the runspace pool Sep 12, 2019
@bergmeister bergmeister merged commit 91eafaf into PowerShell:master Sep 12, 2019
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.

None yet

3 participants