X Tutup
Skip to content

Unregister the event handler from 'AppDomain.CurrentDomain.ProcessExit' when uninitializing AMSI#10626

Merged
1 commit merged intoPowerShell:masterfrom
daxian-dbw:leak
Sep 26, 2019
Merged

Unregister the event handler from 'AppDomain.CurrentDomain.ProcessExit' when uninitializing AMSI#10626
1 commit merged intoPowerShell:masterfrom
daxian-dbw:leak

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Sep 25, 2019

PR Summary

Unregister the event handler from AppDomain.CurrentDomain.ProcessExit when uninitializing AMSI.

The event handler was registered when opening a Runspace, but not unregistered when closing.
In the hosting scenario, every time a Runspace is opened/closed, an instance of the EventHandler will be left behind linked to the AppDomain.CurrentDomain. In a dump file provided by a partner team, there are 588017 such EventHandler instances leaked.

PR Checklist

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Sep 25, 2019

I tried to add a test, but I cannot find a way to get the EventHandler methods from the event member AppDomain.ProcessExit ...

Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

I wonder that
internal static void WinCloseSession()
and
internal static void WinUninitialize()
is not under #if !UNIX ... #endif

Can we use a global static counter for test?

@iSazonov iSazonov added WG-Engine core PowerShell engine, interpreter, and runtime CL-Engine Indicates that a PR should be marked as an engine change in the Change Log and removed WG-Engine core PowerShell engine, interpreter, and runtime labels Sep 26, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.5 milestone Sep 26, 2019
@daxian-dbw
Copy link
Member Author

I can get the invocation method list in .NET, but not in .NET Core. The following works in Windows PowerShell

$f = [System.AppDomain].GetFields([System.Reflection.BindingFlags]::Instance -bor [System.Reflection.BindingFlags]::NonPublic -bor [System.Reflection.BindingFlags]::GetField) | ? name -eq '_processExit'
$v = $f.GetValue([System.AppDomain]::CurrentDomain)
$v.GetInvocationList()

Method                                                          Target
------                                                          ------
Void DisposeOnShutdown(System.Object, System.EventArgs)
Void CurrentDomain_ProcessExit(System.Object, System.EventArgs)
Void SingletonDomainUnload(System.Object, System.EventArgs)     {System.EventHandler}

So at least I can add a test when porting this change to Windows PowerShell.

@daxian-dbw daxian-dbw self-assigned this Sep 26, 2019
@daxian-dbw daxian-dbw added the AutoMerge informs the bot to automerge the PR label Sep 26, 2019
@ghost
Copy link

ghost commented Sep 26, 2019

Hello @daxian-dbw!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 24 hours, a condition that will be fulfilled in about 1 hour 59 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 239c29e into PowerShell:master Sep 26, 2019
@daxian-dbw daxian-dbw deleted the leak branch September 26, 2019 21:56
@iSazonov
Copy link
Collaborator

iSazonov commented Sep 27, 2019

@daxian-dbw I got it ("static" is key):

$f = [System.AppContext].GetFields([System.Reflection.BindingFlags]::Instance -bor [System.Reflection.BindingFlags]::NonPublic -bor [System.Reflection.BindingFlags]::GetProperty -bor [System.Reflection.BindingFlags]::Static) | ? name -eq 'ProcessExit'
$v = $f.GetValue([System.AppDomain]::CurrentDomain)
# or $f.GetValue([System.AppContext]::ProcessExit)

$v.GetInvocationList()

@daxian-dbw
Copy link
Member Author

@iSazonov This is very strange, and feels to me may randomly stop working in future .NET Core 😄

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 1, 2019

@daxian-dbw AppContext is public API and the property is static - it is not likely that this public API will be changed.

@daxian-dbw
Copy link
Member Author

Ahh, I thought it was [System.AppDomain]. Just realized you were using [System.AppContext].

$f.GetValue([System.AppDomain]::CurrentDomain)

This is wrong, should pass in $null as argument. I will submit a test for this change then.

@daxian-dbw
Copy link
Member Author

Hmm, I cannot write a Pester test for this, because the leak won't happen as long as a Runspace still opens. Need to do it in xUnit.

@iSazonov
Copy link
Collaborator

This is wrong, should pass in $null as argument.

I don't understand your thought. It seem works well in the example I posted.

@daxian-dbw
Copy link
Member Author

It's a static field, so should pass null when calling GetValue. Your code works but that may be because the dotnet implementation just ignored the passed in argument, which could change to check for null in future.

@iSazonov
Copy link
Collaborator

Sorry, if I noisy but I always wonder how in-depth you look.
Do you mean that [System.AppDomain]::CurrentDomain can be null? I see it is non-nullable and initialized with new AppDomain().
Also we can use $f.GetValue([System.AppContext]::ProcessExit) as I mentioned in example above.

@daxian-dbw
Copy link
Member Author

I was not clear in my above statement. Today, you can pass anything to $f.GetValue() when it's a static field. For example:

PS:32> $f = [System.AppContext].GetFields([System.Reflection.BindingFlags]::NonPublic -bor [System.Reflection.BindingFlags]::GetProperty -bor [System.Reflection.BindingFlags]::Static) | ? name -eq 'ProcessExit'
PS:33> $f.GetValue("fff")

Target Method
------ ------
       Void CurrentDomain_ProcessExit(System.Object, System.EventArgs)

It may be because today the .net implementation ignore the argument when the field is static, but this could change in future so that exception would be thrown if the passed in argument is not null.

@iSazonov
Copy link
Collaborator

Thanks for clarify!
It would be a breaking change which is incredible for dotnet runtime.

iSazonov pushed a commit that referenced this pull request Oct 11, 2019
Add a test for the even handler leaking fix addressed in #10626 #10626
The test is added to verify that PowerShell unregister the 'AppDomain.ProcessExit' event handler when a Runspace closes.
@ghost
Copy link

ghost commented Oct 23, 2019

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

kilasuit pushed a commit to kilasuit/PowerShell that referenced this pull request Nov 9, 2019
Add a test for the even handler leaking fix addressed in PowerShell#10626 PowerShell#10626
The test is added to verify that PowerShell unregister the 'AppDomain.ProcessExit' event handler when a Runspace closes.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge informs the bot to automerge the PR CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

X Tutup