Skip to content

Support unloadability in DispatchProxy. - #62095

Merged
buyaa-n merged 8 commits into
dotnet:mainfrom
teo-tsirpanis:dispatch-proxy-unloadability
Dec 3, 2021
Merged

Support unloadability in DispatchProxy.#62095
buyaa-n merged 8 commits into
dotnet:mainfrom
teo-tsirpanis:dispatch-proxy-unloadability

Conversation

@teo-tsirpanis

@teo-tsirpanisteo-tsirpanis commented Nov 27, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

The System.Reflection.DispatchProxy type used to be incompatible with unloadability, because the AssemblyBuilder with the generated proxy types is created with AssemblyBuilderAccess.Run and because all proxy types were associated with a single one.

This PR creates a separate AssemblyBuilder, one for each AssemblyLoadContext of the base type's assembly, and passes AssemblyBuilderAccess.RunAndCollect to 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

@ghostghost added area-System.Reflection community-contribution Indicates that the PR has been added by a community member labels Nov 27, 2021
@teo-tsirpanis
teo-tsirpanisforce-pushed the dispatch-proxy-unloadability branch from f5d1489 to 62afbe1CompareNovember 27, 2021 11:19
d066z
d066z reviewed Nov 27, 2021
View reviewed changes

@d066zd066z left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

delete

@teo-tsirpanis

teo-tsirpanis commented Nov 27, 2021

Copy link
Copy Markdown
ContributorAuthor

The two tests I added fail on Browser because Assembly.Location returns an empty string. Any idea how to selectively disable them?

@jkotas

jkotas commented Nov 27, 2021

Copy link
Copy Markdown
Member

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).

@teo-tsirpanis

teo-tsirpanis commented Nov 27, 2021

Copy link
Copy Markdown
ContributorAuthor

Thanks for the feedback @jkotas, that was my first guess, thought there might be a more elegant way. I updated the tests.

d066hie
d066hie reviewed Nov 30, 2021
View reviewed changes

@d066hied066hie left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

change

@teo-tsirpanis

teo-tsirpanis commented Nov 30, 2021

Copy link
Copy Markdown
ContributorAuthor

Hello @d066hie, would you like to explain in more detail what to change in this PR?

vitek-karas
vitek-karas reviewed Dec 1, 2021
View reviewed changes
Comment threadsrc/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs Outdated
Comment threadsrc/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs Outdated
Comment threadsrc/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs
@teo-tsirpanis
Address PR feedback.
eec20f8
Use a different name for the default ALC's proxy assembly and make it collectible only if its associated ALC is.
@teo-tsirpanis
Name the dynamic module "testmod" again.
7f74356
@buyaa-n

buyaa-n commented Dec 2, 2021

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Dec 2, 2021

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

buyaa-n
buyaa-n approved these changes Dec 2, 2021
View reviewed changes

@buyaa-nbuyaa-n left a comment

Copy link
Copy Markdown
Contributor

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

@buyaa-n

buyaa-n commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

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.

@buyaa-n
buyaa-n merged commit 2f0341d into dotnet:mainDec 3, 2021
@teo-tsirpanis
teo-tsirpanis deleted the dispatch-proxy-unloadability branch December 3, 2021 17:53
@vitek-karas

vitek-karas commented Dec 3, 2021

Copy link
Copy Markdown
Member

Thanks a lot @teo-tsirpanis !!

@ghostghost locked as resolved and limited conversation to collaborators Jan 7, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas left review comments
@vitek-karasvitek-karasvitek-karas approved these changes
@stevehartersteveharterAwaiting requested review from steveharter
@GrabYourPitchforksGrabYourPitchforksAwaiting requested review from GrabYourPitchforks
+3 more reviewers
@d066hied066hied066hie approved these changes
@d066zd066zd066z left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Reflectioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DispatchProxy cannot work with types from collectible assemblies. DispatchProxy.Create<IProxy, Base> uncastable to Base in custom load context

8 participants

@teo-tsirpanis@jkotas@buyaa-n@vitek-karas@d066hie@d066z@eiriktsarpalis@joperezr