-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Support unloadability in DispatchProxy.
#62095
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
Support unloadability in DispatchProxy.
#62095
Conversation
And create them with AssemblyBuilderAccess.RunAndCollect.
f5d1489 to
62afbe1
Compare
d066z
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
|
The two tests I added fail on Browser because |
|
I would recomend to early out from the tests if the Location is empty string. It will make the test pass in all situations when the assembly is not physically available on the disk (e.g. #43079). |
|
Thanks for the feedback @jkotas, that was my first guess, thought there might be a more elegant way. I updated the tests. |
d066hie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change
|
Hello @d066hie, would you like to explain in more detail what to change in this PR? |
src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
Show resolved
Hide resolved
Use a different name for the default ALC's proxy assembly and make it collectible only if its associated ALC is.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
buyaa-n
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @teo-tsirpanis, LGTM
|
Outerloop test failures are unrelated and reported as needed (#62305, #60753 (comment), #58616 (comment), #56165 (comment)). Thank you for your review @jkotas @vitek-karas if you have no more comments/concerns the PR is ready for merge. |
|
Thanks a lot @teo-tsirpanis !! |
The
System.Reflection.DispatchProxytype used to be incompatible with unloadability, because theAssemblyBuilderwith the generated proxy types is created withAssemblyBuilderAccess.Runand because all proxy types were associated with a single one.This PR creates a separate
AssemblyBuilder, one for eachAssemblyLoadContextof the base type's assembly, and passesAssemblyBuilderAccess.RunAndCollectto it (edit: only if the associated ALC is unloadable). It also cleans-up some code.Besides allowing unneeded proxy types to be granularly unloaded, it also added support for creating proxies for the same type in different ALC. I added tests for both cases.
Fixes #60468
Fixes #62050