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

Collectible Assemblies and the CLR #552

Open
JoshVarty opened this Issue Mar 24, 2015 · 28 comments

Comments

Projects
None yet
@JoshVarty
Contributor

JoshVarty commented Mar 24, 2015

Hi there,

I would like to start the conversation about collectible assemblies and the CLR. For reference the user voice for this issue is available here. My understanding is that this is currently possible with Mono, but not the CLR.

With the new Roslyn compiler and projects like ScriptCS, I think we're going to see an increase in the number of users who are dynamically compiling and running code. Today there is no way to unload an assembly (without burning down the entire AppDomain 😄).

I'd love to get a feel for what limitations exist that would prevent this feature, how much work it might take and whether or not the team felt it was feasible (or necessary).

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Mar 25, 2015

Member

Thank you for starting the conversation.

We have been mulling over this feature request for a while.System.Runtime.Loader.AssemblyLoadContext - .NET Core replacement for AppDomain.AssemblyResolve event - was designed with collectability in mind. Our tentative plan has been to expose it by adding a new constructor to this type that takes an extra flag to mark the context as collectible. That is the easy part.

The hard part is the runtime implementation of the collectability. It should build upon the existing Reflection.Emit collectability infrastructure (look for FEATURE_COLLECTIBLE_TYPES). The implementation needs to include targeted tests for corner cases as well as stress testing the unloading (e.g. run all existing corefx tests as unloadable in a loop for a long time, etc.)

In case somebody would like to start the implementation before the core team gets to it, it should be done in series of smaller commits under FEATURE_XXX define (e.g. FEATURE_COLLECTIBLE) that will be turned off by default until the implementation is completed. The first baby steps may be to add the extra argument to the AssemblyLoadContext constructor under this define.

Member

jkotas commented Mar 25, 2015

Thank you for starting the conversation.

We have been mulling over this feature request for a while.System.Runtime.Loader.AssemblyLoadContext - .NET Core replacement for AppDomain.AssemblyResolve event - was designed with collectability in mind. Our tentative plan has been to expose it by adding a new constructor to this type that takes an extra flag to mark the context as collectible. That is the easy part.

The hard part is the runtime implementation of the collectability. It should build upon the existing Reflection.Emit collectability infrastructure (look for FEATURE_COLLECTIBLE_TYPES). The implementation needs to include targeted tests for corner cases as well as stress testing the unloading (e.g. run all existing corefx tests as unloadable in a loop for a long time, etc.)

In case somebody would like to start the implementation before the core team gets to it, it should be done in series of smaller commits under FEATURE_XXX define (e.g. FEATURE_COLLECTIBLE) that will be turned off by default until the implementation is completed. The first baby steps may be to add the extra argument to the AssemblyLoadContext constructor under this define.

@jkotas jkotas added the up-for-grabs label Mar 25, 2015

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Apr 10, 2015

Contributor

+1! 😄 Would love this for razor compilation and other things

Contributor

davidfowl commented Apr 10, 2015

+1! 😄 Would love this for razor compilation and other things

@jakesays

This comment has been minimized.

Show comment
Hide comment
@jakesays

jakesays Apr 10, 2015

I'd love to work on this!

jakesays commented Apr 10, 2015

I'd love to work on this!

@MattWhilden

This comment has been minimized.

Show comment
Hide comment
@MattWhilden

MattWhilden Apr 10, 2015

Member

Start with the "easy" constructor changes and feel free to ask questions about the rest. :-)

Member

MattWhilden commented Apr 10, 2015

Start with the "easy" constructor changes and feel free to ask questions about the rest. :-)

@Rohansi

This comment has been minimized.

Show comment
Hide comment
@Rohansi

Rohansi Oct 2, 2015

System.Runtime.Loader.AssemblyLoadContext - .NET Core replacement for AppDomain.AssemblyResolve event - was designed with collectability in mind. Our tentative plan has been to expose it by adding a new constructor to this type that takes an extra flag to mark the context as collectible.

Would this behave like AppDomains where all loaded assemblies are collected with the context or does the context just mark assemblies loaded through it as collectible? After looking through the code it looks like it would be per assembly since the CLR already supports that.

So AssemblyLoadContext would need a new constructor that lets you specify whether it loads assemblies as collectible. It looks like that flag would need to make it down to Assembly::Create which has a parameter to mark assemblies as collectible.

A LoaderAllocater is required for collectible assemblies as well. Assembly::CreateDynamic uses AssemblyLoaderAllocator which is probably what should be used, assuming per assembly collection I guess.

Is this correct? I'm not really sure how I would get that information from AssemblyLoadContext all the way to Assembly::Create, or if I even need to.

Rohansi commented Oct 2, 2015

System.Runtime.Loader.AssemblyLoadContext - .NET Core replacement for AppDomain.AssemblyResolve event - was designed with collectability in mind. Our tentative plan has been to expose it by adding a new constructor to this type that takes an extra flag to mark the context as collectible.

Would this behave like AppDomains where all loaded assemblies are collected with the context or does the context just mark assemblies loaded through it as collectible? After looking through the code it looks like it would be per assembly since the CLR already supports that.

So AssemblyLoadContext would need a new constructor that lets you specify whether it loads assemblies as collectible. It looks like that flag would need to make it down to Assembly::Create which has a parameter to mark assemblies as collectible.

A LoaderAllocater is required for collectible assemblies as well. Assembly::CreateDynamic uses AssemblyLoaderAllocator which is probably what should be used, assuming per assembly collection I guess.

Is this correct? I'm not really sure how I would get that information from AssemblyLoadContext all the way to Assembly::Create, or if I even need to.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Oct 2, 2015

Member

Would this behave like AppDomains where all loaded assemblies are collected with the context

All assemblies loaded into given collectible AssemblyLoadContext should be unloaded as a unit.

AssemblyLoadContext all the way to Assembly::Create

The owning AssemblyLoadContext can be retrieved in Assembly::Create from PEAssembly argument using GetBindingContext().

Member

jkotas commented Oct 2, 2015

Would this behave like AppDomains where all loaded assemblies are collected with the context

All assemblies loaded into given collectible AssemblyLoadContext should be unloaded as a unit.

AssemblyLoadContext all the way to Assembly::Create

The owning AssemblyLoadContext can be retrieved in Assembly::Create from PEAssembly argument using GetBindingContext().

@Rohansi

This comment has been minimized.

Show comment
Hide comment
@Rohansi

Rohansi Oct 3, 2015

pFile->GetBindingContext() is giving me an instance of BINDER_SPACE::Assembly* downcasted to ICLRPrivBinder*. I can see the field I think I need in there but BINDER_SPACE::Assembly::GetBinder() is protected. Would I need to add a friend class or should I be looking at something else?

Also how would I safely cast the ICLRPrivBinder* to CLRPrivBinderAssemblyLoadContext*? I think I would need to make an interface so it can be done with QueryInterface but I'm new to COM so I'm probably missing something.

Rohansi commented Oct 3, 2015

pFile->GetBindingContext() is giving me an instance of BINDER_SPACE::Assembly* downcasted to ICLRPrivBinder*. I can see the field I think I need in there but BINDER_SPACE::Assembly::GetBinder() is protected. Would I need to add a friend class or should I be looking at something else?

Also how would I safely cast the ICLRPrivBinder* to CLRPrivBinderAssemblyLoadContext*? I think I would need to make an interface so it can be done with QueryInterface but I'm new to COM so I'm probably missing something.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Oct 3, 2015

Member

Yes, creating interface would be a clean way to do this.

Member

jkotas commented Oct 3, 2015

Yes, creating interface would be a clean way to do this.

@Rohansi

This comment has been minimized.

Show comment
Hide comment
@Rohansi

Rohansi Oct 3, 2015

So I made an interface and changed CLRPrivBinderAssemblyLoadContext to inherit IUnknownCommon<ICLRPrivBinder, IAssemblyLoadContext> but now a static_cast is failing to compile, presumably because both interfaces inherit from IUnknown:

"bin\obj\Windows_NT.x64.Debug\src\vm\wks\cee_wks.vcxproj" (default target) (14:3) ->
    src\vm\coreassemblyspec.cpp(348): error C2594: 'static_cast': ambiguous conversions from 'IUnknown *' to 'CLRPrivBinderAssemblyLoadContext *'

Here's what I have for my interface right now:

class DECLSPEC_UUID("68220E65-3D3F-42E2-BAD6-2D07419DAB5E") IAssemblyLoadContext : public IUnknown
{
public:
    STDMETHOD(GetIsCollectible)(
            /* [retval][out] */ BOOL *pIsCollectible) = 0;
};

Also is there any reason why BINDER_SPACE::Assembly::GetBinder() is protected? I'm having trouble adding a friend class for ::Assembly.

Rohansi commented Oct 3, 2015

So I made an interface and changed CLRPrivBinderAssemblyLoadContext to inherit IUnknownCommon<ICLRPrivBinder, IAssemblyLoadContext> but now a static_cast is failing to compile, presumably because both interfaces inherit from IUnknown:

"bin\obj\Windows_NT.x64.Debug\src\vm\wks\cee_wks.vcxproj" (default target) (14:3) ->
    src\vm\coreassemblyspec.cpp(348): error C2594: 'static_cast': ambiguous conversions from 'IUnknown *' to 'CLRPrivBinderAssemblyLoadContext *'

Here's what I have for my interface right now:

class DECLSPEC_UUID("68220E65-3D3F-42E2-BAD6-2D07419DAB5E") IAssemblyLoadContext : public IUnknown
{
public:
    STDMETHOD(GetIsCollectible)(
            /* [retval][out] */ BOOL *pIsCollectible) = 0;
};

Also is there any reason why BINDER_SPACE::Assembly::GetBinder() is protected? I'm having trouble adding a friend class for ::Assembly.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Oct 4, 2015

Member

static_cast is failing to compile

How many places have this problem? You should be able to fix them by casting to the specific interface.

Any reason why BINDER_SPACE::Assembly::GetBinder() is protected?

I do not see a particularly strong reason. It should be ok to change it to public.

Member

jkotas commented Oct 4, 2015

static_cast is failing to compile

How many places have this problem? You should be able to fix them by casting to the specific interface.

Any reason why BINDER_SPACE::Assembly::GetBinder() is protected?

I do not see a particularly strong reason. It should be ok to change it to public.

@Rohansi

This comment has been minimized.

Show comment
Hide comment
@Rohansi

Rohansi Oct 4, 2015

How many places have this problem? You should be able to fix them by casting to the specific interface.

It's just one place but there is no interface for that method, even though it looks like there should be one. I changed it to a reinterpret_cast to get it to build but I doubt that would work properly at runtime.

Rohansi commented Oct 4, 2015

How many places have this problem? You should be able to fix them by casting to the specific interface.

It's just one place but there is no interface for that method, even though it looks like there should be one. I changed it to a reinterpret_cast to get it to build but I doubt that would work properly at runtime.

@Rohansi

This comment has been minimized.

Show comment
Hide comment
@Rohansi

Rohansi Oct 4, 2015

OK so I have it using the IsCollectible flag and LoaderAllocator I added to CLRPrivBinderAssemblyLoadContext in Assembly::Create() and it now crashes in LoaderAllocator::Destroy() because I didn't create a DomainAssembly. I'm not sure what it should be initialized with.

I'm basing this off of what Assembly::CreateDynamic() does and it creates a DomainAssembly for the PEAssembly it creates. I'm not sure if I should be doing the same. And how do I keep them together so they unload as a unit, do I just have to use the same LoaderAllocator for them?

Rohansi commented Oct 4, 2015

OK so I have it using the IsCollectible flag and LoaderAllocator I added to CLRPrivBinderAssemblyLoadContext in Assembly::Create() and it now crashes in LoaderAllocator::Destroy() because I didn't create a DomainAssembly. I'm not sure what it should be initialized with.

I'm basing this off of what Assembly::CreateDynamic() does and it creates a DomainAssembly for the PEAssembly it creates. I'm not sure if I should be doing the same. And how do I keep them together so they unload as a unit, do I just have to use the same LoaderAllocator for them?

@tzwlai tzwlai added the area-VM label Oct 23, 2015

@tzwlai tzwlai added this to the Future milestone Oct 23, 2015

@runxc1

This comment has been minimized.

Show comment
Hide comment
@runxc1

runxc1 Jan 12, 2016

Is this going to make RC2?

runxc1 commented Jan 12, 2016

Is this going to make RC2?

@terrajobst

This comment has been minimized.

Show comment
Hide comment
@terrajobst

terrajobst Feb 19, 2016

Member

No, which is why we moved it into the Future milestone.

Member

terrajobst commented Feb 19, 2016

No, which is why we moved it into the Future milestone.

@runxc1

This comment has been minimized.

Show comment
Hide comment
@runxc1

runxc1 Feb 28, 2016

So if it's in the Future milestone is it effectively dead? A 'Future' sprint is usually where most tasks go to die and be forgotten

runxc1 commented Feb 28, 2016

So if it's in the Future milestone is it effectively dead? A 'Future' sprint is usually where most tasks go to die and be forgotten

@terrajobst

This comment has been minimized.

Show comment
Hide comment
@terrajobst

terrajobst Feb 29, 2016

Member

@runxc1 That's not the intent. 'Future' is the equivalent of our backlog. The other milestones are tracking the upcoming RC and RTM versions early this year. Impactful features like this don't make the bar for V1.

Member

terrajobst commented Feb 29, 2016

@runxc1 That's not the intent. 'Future' is the equivalent of our backlog. The other milestones are tracking the upcoming RC and RTM versions early this year. Impactful features like this don't make the bar for V1.

@attilah

This comment has been minimized.

Show comment
Hide comment
@attilah

attilah Jul 12, 2016

@terrajobst any info on this since fabruary and a successful release of 1.0.0? ;-)

attilah commented Jul 12, 2016

@terrajobst any info on this since fabruary and a successful release of 1.0.0? ;-)

@xoofx

This comment has been minimized.

Show comment
Hide comment
@xoofx

xoofx Dec 16, 2016

Member

Will collectible assemblies be part of the 1.2... or it is more reasonable it will not?

Member

xoofx commented Dec 16, 2016

Will collectible assemblies be part of the 1.2... or it is more reasonable it will not?

@runxc1

This comment has been minimized.

Show comment
Hide comment
@runxc1

runxc1 Dec 16, 2016

That would be very nice if this made it into 2.0

runxc1 commented Dec 16, 2016

That would be very nice if this made it into 2.0

@xoofx

This comment has been minimized.

Show comment
Hide comment
@xoofx

xoofx Jan 3, 2017

Member

For folks following this feature, I have been working to bring back this feature with the PR #8677 on coreclr and the PR 14763 on corefx. The current status is that it seems that the implementation is working well so far but requires a bit more stress tests to verify that we have something rock solid.

Member

xoofx commented Jan 3, 2017

For folks following this feature, I have been working to bring back this feature with the PR #8677 on coreclr and the PR 14763 on corefx. The current status is that it seems that the implementation is working well so far but requires a bit more stress tests to verify that we have something rock solid.

@Suriman

This comment has been minimized.

Show comment
Hide comment
@Suriman

Suriman Dec 26, 2017

As I have understood, this feature allows, selectively, unload an assembly within a context, leaving the rest of the assemblies loaded. This is very powerful, but would it be possible to also implement the option to unload the full context, just like it can be done with the AppDomain?. This might accelerate the delivery of this feature.

Suriman commented Dec 26, 2017

As I have understood, this feature allows, selectively, unload an assembly within a context, leaving the rest of the assemblies loaded. This is very powerful, but would it be possible to also implement the option to unload the full context, just like it can be done with the AppDomain?. This might accelerate the delivery of this feature.

@agatlin

This comment has been minimized.

Show comment
Hide comment
@agatlin

agatlin Dec 27, 2017

What is the status of this feature? Is it yet available? Will it be available only for dynamic assemblies or also for static assemblies?

agatlin commented Dec 27, 2017

What is the status of this feature? Is it yet available? Will it be available only for dynamic assemblies or also for static assemblies?

@dandcg

This comment has been minimized.

Show comment
Hide comment
@dandcg

dandcg Jan 10, 2018

Any further update on this - or is the only work around to terminate the process and wait for daemon / service / iis to restart in order to clear the context?

dandcg commented Jan 10, 2018

Any further update on this - or is the only work around to terminate the process and wait for daemon / service / iis to restart in order to clear the context?

@seriouz

This comment has been minimized.

Show comment
Hide comment
@seriouz

seriouz Apr 3, 2018

@dandcg As jkotas stated here the functionality of unload assemblies will not be done with 2.1! But they will talk again about this topic when they make a new roadmap for 2.2 e.g.
I hope they pluck up courage and add this base functionality any time soon! Much work on this topic has already been done on this PR.

seriouz commented Apr 3, 2018

@dandcg As jkotas stated here the functionality of unload assemblies will not be done with 2.1! But they will talk again about this topic when they make a new roadmap for 2.2 e.g.
I hope they pluck up courage and add this base functionality any time soon! Much work on this topic has already been done on this PR.

@EnCey

This comment has been minimized.

Show comment
Hide comment
@EnCey

EnCey Aug 3, 2018

Seems the roadmap for 2.2 stands and 3.0 was announced as well some time ago, was there a discussion about this topic and can you share your plans? Could it be part of 3.0?

EnCey commented Aug 3, 2018

Seems the roadmap for 2.2 stands and 3.0 was announced as well some time ago, was there a discussion about this topic and can you share your plans? Could it be part of 3.0?

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Aug 3, 2018

Member

Yes, this is planned to be in 3.0.

Member

jkotas commented Aug 3, 2018

Yes, this is planned to be in 3.0.

@jkotas jkotas modified the milestones: Future, 3.0 Aug 3, 2018

@masonwheeler

This comment has been minimized.

Show comment
Hide comment
@masonwheeler

masonwheeler Aug 3, 2018

@jkotas Awesome!

So... when's that coming out? 😛

masonwheeler commented Aug 3, 2018

@jkotas Awesome!

So... when's that coming out? 😛

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Aug 3, 2018

Collaborator

@masonwheeler

We are planning on releasing a first preview of .NET Core 3 later this year and the final version in 2019.

https://blogs.msdn.microsoft.com/dotnet/2018/05/07/net-core-3-and-support-for-windows-desktop-applications/

(...which is terrifyingly soon, I just realized...)

Collaborator

jnm2 commented Aug 3, 2018

@masonwheeler

We are planning on releasing a first preview of .NET Core 3 later this year and the final version in 2019.

https://blogs.msdn.microsoft.com/dotnet/2018/05/07/net-core-3-and-support-for-windows-desktop-applications/

(...which is terrifyingly soon, I just realized...)

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