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

Support Collectible Dynamic Assemblies #473

Open
jonorossi opened this issue Sep 10, 2019 · 15 comments
Open

Support Collectible Dynamic Assemblies #473

jonorossi opened this issue Sep 10, 2019 · 15 comments
Assignees
Milestone

Comments

@jonorossi
Copy link
Member

.NET Core 3.0 introduces support for unloading assemblies after loading them into a custom AssemblyLoadContext via the new Unload method and constructor with isCollectible parameter. The custom AssemblyLoadContext is set up to only contain the assemblies you want with everything else being in the default context. Obviously this is a .NET Core 3.0 feature, and .NET Framework will never support it; .NET Standard also does not define System.Runtime.Loader classes. Unfortunately it looks like dynamic assemblies were missed (https://github.com/dotnet/corefx/issues/38426) and support for them won't make the cut for .NET Core 3.0.

.NET Framework 4.0 introduced AssemblyBuilderAccess.RunAndCollect which allows dynamic assemblies to be garbage collected when no reference is held. RunAndCollect has a whole heap of restrictions on what can be emitted into the assembly (e.g. no COM interop), but I think we could fairly trivially add opt-in for this per ModuleScope to make it disposable.

I don't know how useful this feature would actually be since we don't get any requests for it, but logging this as an enhancement to raise the topic since it is possible. It sounds like it RunAndCollect could be helpful for #472.

@stakx
Copy link
Member

stakx commented Sep 23, 2019

I don't know how useful this feature would actually be since we don't get any requests for it [...]

There has been at least one StackOverflow question which seems related to just this.

we could fairly trivially add opt-in for this per ModuleScope to make it disposable [...]. It sounds like it RunAndCollect could be helpful for #472.

I haven't thought it through all the way, one thing that perhaps would have to be pointed out in the documentation is that dynamic assemblies could be collected only once their associated ProxyGenerator is no longer referenced, nor any proxy objects created by that generator.

@jonorossi
Copy link
Member Author

I haven't thought it through all the way, one thing that perhaps would have to be pointed out in the documentation is that dynamic assemblies could be collected only once their associated ProxyGenerator is no longer referenced, nor any proxy objects created by that generator.

Yep, that was my intention. There are heaps of limitations to the feature and it would really only fit the scenario of plugins or something similar where you dump everything and start again without restarting the process, most applications obviously don't work this way.

I only logged the issue after reading the .NET Core 3.0 release notes and doing a little bit of investigation into how it works, so thought I'd capture my notes.

@xlievo
Copy link

xlievo commented Nov 17, 2019

I admire your efforts and persistence over the years, as well as the rigorous way to discuss issues! Thank

@mhascak
Copy link

mhascak commented Apr 3, 2020

Hello, I´m currently extending plugins system which based on AspNetBoilareplate framework and this enhancement will help me to create more dynamic dev worklow. When plugins DLLs changes in CORE project I desperately need to unload / reload this plugins assemblies., It works thanks custom AssemblyLoadContext, but after that I have this error:

Castle.DynamicProxy - A non-collectible assembly may not reference a collectible assembly.

So I vote +1 for this. Tahanks!

@jairbubbles
Copy link

Hi @jonorossi, I stumbled into that issues through NSubstitute. Rather than an opt-in I was wondering if we couldn't use RunAndCollect when we detect that the current assembly is collectible (see https://learn.microsoft.com/en-us/dotnet/api/system.reflection.assembly.iscollectible?view=net-7.0).

Something like:

Assembly.GetExecutingAssembly().IsCollectible ? AssemblyBuilderAccess.RunAndCollect : AssemblyBuilderAccess.Run;

Using collectible assemblies is not common so I feel like it would have limited repercussions.

@SongOfYouth
Copy link

If we use AssemblyLoadContext and set isCollectible to true, when call ProxyGenerator.CreateInterfaceProxyWithoutTarget, we get a error with "A non-collectible assembly may not reference a collectible assembly".

@stakx
Copy link
Member

stakx commented Mar 8, 2023

"A non-collectible assembly may not reference a collectible assembly"

If this is generally true, then it would seem to be the death sentence for this enhancement, since (I assume) assemblies that use DynamicProxy will typically not be collectible. I wonder if the feature would still be worth having (as an opt-in) for the few remaining cases (like @mhascak's, see above).

@SongOfYouth
Copy link

SongOfYouth commented Mar 13, 2023

"A non-collectible assembly may not reference a collectible assembly"

If this is generally true, then it would seem to be the death sentence for this enhancement, since (I assume) assemblies that use DynamicProxy will typically not be collectible. I wonder if the feature would still be worth having (as an opt-in) for the few remaining cases (like @mhascak's, see above).

In some scenarios, it's needed. for example, the component using DynamicProxy is just a plugin in my application, and i want to load and hot unload it at runtime, sometimes for dynamic update. I used DynamicProxy to create http client proxy, before, all my components is collectible plugins, but now they are not.

@afantyro
Copy link

afantyro commented May 21, 2024

Do we have any update on this? Or an EST? I am facing the same scenario where I need to generate dynamic proxies on both collectible ALC and default ALC side. But I kept getting "A non-collectible assembly may not reference a collectible assembly"

@simonferquel
Copy link

As we are pretty busy working on Unity with CoreCLR, we got hit by this issue, and we have a proposed fix here: #679

@ydie22
Copy link

ydie22 commented Jul 18, 2024

We are also facing this issue, where we have a dynamic proxy intercepting a target with a type defined in a collectible assembly. I validated that the proposed PR solves the issue, albeit I had to explicitly change the default to RunAndCollect (the load context was not collectible, but the referenced assembly by the generated proxy was).

@stakx
Copy link
Member

stakx commented Aug 31, 2024

I've been reviewing @simonferquel's PR #679 again. (Thanks again for the initiative of putting it together!) While I'm not opposed to it in principle, there's one thing that bothers me about it: the introduction of AssemblyBuilderAccessDefaults, a static class that acts as a kind of global option switch. I would prefer to have as few globals in DynamicProxy as possible.

It then occurred to me that we might already have all necessary abstractions in place to enable AssemblyBuilderAccess.RunAndCollect, so I've put together an alternative PR #685 for further discussion. If we followed that approach, one would make the generated assembles collectible like so:

-var generator = new ProxyGenerator();
+var generator = new ProxyGenerator(new CollectibleProxyBuilder());

This follows an already established pattern that's used on .NET 4 to make the generated assemblies persistable to disk:

var generator = new ProxyGenerator(new PersistentProxyBuilder());

I think this would be slightly cleaner than introducing a global static option switch.

@simonferquel's PR includes additional logic such that DynamicProxy automatically chooses between Run and RunAndCollect, instead of always defaulting to Run-only. It should be easy to add such logic to my alternate PR as well. (I've marked the code location for that in a review comment.) I've omitted that logic for now because I am not certain that such auto-detection would always make the right decision; nor am I certain that it's needed: perhaps collectible assemblies aren't a very common use case? Perhaps it might be preferable to have to be explicit about assembly collectibility?

Any thoughts or opinions on this?

@ydie22
Copy link

ydie22 commented Oct 3, 2024

I verified that the proposed alternate PR works as expected when integrated in our code base, and that's OK. From a design point of view, I find it slightly more elegant and more in line with the current library design than the initial proposal.
Regarding the auto-detection of "collectibility", we don't need it in our specific case, and, as stated in my previous comment, it is not working in all cases when based exclusively on load context.
Practical question: what is preventing merging this PR and releasing an updated version with the feature? Actually, this issue dates back from 2019 already, and we are waiting for it for about two years. Not having it prevents us from upgrading other libraries (namely protobuf-net.Grpc), which is a problem.

@stakx
Copy link
Member

stakx commented Oct 3, 2024

@ydie22, thanks for the positive feedback.

As to your question, nothing prevents us from going forward and merging this extension. And it took so long because (apparently) this wasn't important enough for anyone to take the initiative... until quite recently. Remember this open-source project is essentially unpaid work, and we don't have many people maintaining it anymore... both of us have lately been busy with life outside Castle so things have progressed even slower. :-/

I'm happy to go ahead with this though if it's good enough for everyone.

@stakx stakx self-assigned this Oct 19, 2024
@stakx stakx added this to the vNext milestone Oct 19, 2024
@ankud1nov
Copy link

With these changes I was able to migrate my project from netFramework to netCore, thank you. 👍 Really looking forward to these changes.😎

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