Adding some other details I forgot:

• I had to change the behavior of the AssemblyLoadContext.cs of the handle created in the InitializeLoadContextmethod. Previously the handle was normal, hence preventing the AssemblyLoadContext itself to be collected. I guess that making it Weak is more adequate in this case. I believe that when we resolve back with GCHandle.FromIntPtr in the callbacks from native, there is always a LoadXXX blocking happening on the managed side, so this is always alive.
• Another issue in AssemblyLoadContext.cs is that the event Unloading was added. I had to remove it because I don't see how to run anything on the finalizer of this object if there is a reference to it. This was added by issue corefx issue 5205 by @kouvel If we don't want to run it on the finalizer, then it will require a more complex lock setup and prevention for using any Load method (and accessing the underlying native context). What do we want to do with this?
• Also in the LoaderAllocator::GCLoaderAllocators at the end, the code was previously calling pAppDomain->ShutdownFreeLoaderAllocators(TRUE); meaning that underneath, the LoaderAllocator was delayed collected until the next GEN2... the problem is that it makes the collection happening a long time after and it is letting many LoaderAllocator in flight until this happens (in my simple run, the memory was going up to 80Mo instead of staying around 7Mo) . I have changed this to ShutdownFreeLoaderAllocators(FALSE); but seeing the comment in this method saying that some MethodTable of the LoaderAllocator could be somewhere kept until GEN2, I'm not comfortable with this issue as well...

cc @rahku @gkhanna79

meaning that underneath, the LoaderAllocator was delayed collected until the next GEN2...

The GC heap has to be always walkable. It means that all objects, even the dead ones that points to unloaded loader allocator have to have a valid method table. Gen2 turns all dead objects into free space. That's why it is safe to free the loader allocator memory only after the Gen2 GC. In theory, it may possible to invent a special pass over the GC heap to turn all objects that point to the unloaded loader allocator into free space, but it would not be very different from Gen2 GC and the synchronization between this special pass and the GC would be tricky.

I don't see how to run anything on the finalizer of this object if there is a reference to it

The object graph that became unreachable has still references between objects, so there should not be any fundamental problem with getting this event fired.

The current design for the reflection.emit collectible assemblies depends on the GC to figure out that it is no longer reachable. It means that all references to it have to be done via WeakHandles, etc. It would be likely pretty inconvenient in practice. It may be useful to have explicit Unload or Dispose method on the AssemblyLoadContext to initiate the unloading process: The unloading event would fire, finalizers would start running, ... .

It would be likely pretty inconvenient in practice. It may be useful to have explicit Unload or Dispose method on the AssemblyLoadContext to initiate the unloading process: The unloading event would fire, finalizers would start running, ... .

So AssemblyLoadContext would have:

• an explicit unloading process done via an Unload method
• an implicit unloading process via the finalizer

Explicit mode

The native AssemblyLoadContext would have a strong handle on the LoaderAllocator.
Assuming that there is no longer any references to assemblies loaded, a call to AssemblyLoadContxet.Unload would release this handle, return immediately and the Unloading event would occur once the finalizer on the LoaderAllocator has been called (but the native LoaderAllocator would not be deleted straightaway, we will keep the Gen2 as it was then). Once a LoaderAllocator is released, the Unloading event would be fired on the AssemblyLoadContext

Here I'm just a little bit worried about calling back the Unloading event indirectly from native code that was called from the finalizer of the LoaderAllocator, is it a good idea or should it be delayed? (but for delay, I have no idea right now how this would work...)

In order to secure the access for the explicit Unload, it would require an internal lock in Unload and in any LoadFromXXXX. Also after a call to Unload, any subsequent call to LoadFromXXX would throw an exception.

I guess that the Dispose pattern might not be a good mental fit for this workflow, as the AssemblyLoadContext would still be partially valid (through its pending UnLoading event), so maybe only expose an UnLoad method?

Implicit node

AssemblyLoadContext finalizer would be run without a prior Unload. The main difference with the explicit mode is that we would not have any Unloading event fired (because it would mean that nobody subscribed to it because of the finalizer)

What do you think?

I do not understand why the implicit mode cannot fire the unloading event.

What would be passed as the sender for the Unloading event? Would it be the AssemblyLoadContext instance? If that's the case wouldn't it be possible to resurrect it?

Yes, poorly written or malicious code can resurrect it - but it can resurrect it in number of other ways too, so it is not introducing a new unique problem.

I do not understand why the implicit mode cannot fire the unloading event.

Actually, re-reading the code with my original problem with Unloading, it was only due to the current workaround with subscribing the AssemblyLoadContext to the AppDomain.ProcessExit event, so as this would not exist anymore, we can indeed call the unloading event from the finalizer.

I assume the default assembly load context would not go through the explicit unload path during shutdown. If that's the case, only the default assembly load context would need to subscribe to ProcessExit so that the Unloading event on that instance is raised upon shutdown.

Also, what about non-default assembly load contexts on shutdown? Finalizers aren't guaranteed to run on shutdown anymore, and the Unloading event was meant to provide a notification of shutdown, so the event would still need to be raised, if those instances don't go through the explicit unload path somehow during shutdown.

@kouvel good catch about the shutdown process and finalizers.

I guess one possibility is to keep track of assembly load contexts that have not yet been unloaded in a static weak reference collection. Unload and the finalizer would remove the instance from the collection and raise the Unloading event. A static method can be registered for the ProcessExit event, to raise the Unloading event on the remaining instances in the collection.

So I have pushed a new version.
I tried to add in the first commits changes that are not strictly related to ALC.
The last commit (Add support for collectible ALC) is still a WIP.
The AssemblyLoadContext contains now a Unload method and the Unloading event is supported.
Thought it is working most of the time, there are issues with cache invalidation that are making the feature still unstable.
I have for example discovered that a few methods from AppDomain accessing the AssemblySpecBindingCache were not protecting the access with the associated Crst CrstHolder holder(&m_DomainCacheCrst);. For example, AppDomain::FindCachedAssembly. Adding a protection to these methods lead me to a different bug:

Assert failure(PID 9916 [0x000026bc], Thread: 16008 [0x3e88]): CONTRACT VIOLATION by CrstBase::Enter at "c:\code\dotnet\coreclr\src\vm\crst.cpp" @ 358

You cannot enter a lock in a GC_NOTRIGGER + MODE_COOPERATIVE region.

VIOLATED-->  CONTRACT in Module::GetAssemblyIfLoaded at "c:\code\dotnet\coreclr\src\vm\ceeload.cpp" @ 5698
                        CONTRACT in Module::LoadAssembly at "c:\code\dotnet\coreclr\src\vm\ceeload.cpp" @ 6108
                        CONTRACT in Assembly::FindModuleByExportedType at "c:\code\dotnet\coreclr\src\vm\assembly.cpp" @ 1498
                        CONTRACT in EEClassHashTable::UncompressModuleAndClassDef at "c:\code\dotnet\coreclr\src\vm\classhash.cpp" @ 165
                        CONTRACT in ClassLoader::FindClassModuleThrowing at "c:\code\dotnet\coreclr\src\vm\clsload.cpp" @ 1745
                        CONTRACT in ClassLoader::LoadTypeHandleThrowing at "c:\code\dotnet\coreclr\src\vm\clsload.cpp" @ 2016
                        GCX_COOP in TypeName::GetTypeWorker at "c:\code\dotnet\coreclr\src\vm\typeparse.cpp" @ 1439
                        CONTRACT in TypeName::GetTypeWorker at "c:\code\dotnet\coreclr\src\vm\typeparse.cpp" @ 1437
                        CONTRACT in TypeName::GetTypeManaged at "c:\code\dotnet\coreclr\src\vm\typeparse.cpp" @ 1193
                        CONTRACT in RuntimeTypeHandle::GetTypeByName at "c:\code\dotnet\coreclr\src\vm\runtimehandles.cpp" @ 1899
                        OVERRIDE_TYPE_LOAD_LEVEL_LIMIT in MethodDescCallSite::CallTargetWorker at "c:\code\dotnet\coreclr\src\vm\callhelpers.cpp" @ 382
                        CONTRACT in MethodDescCallSite::CallTargetWorker at "c:\code\dotnet\coreclr\src\vm\callhelpers.cpp" @ 372
                        CONTRACT in GetExceptionMessage at "c:\code\dotnet\coreclr\src\vm\excep.cpp" @ 379
                        CONTRACT in GetExceptionMessage at "c:\code\dotnet\coreclr\src\vm\excep.cpp" @ 300
                        GCX_COOP in DefaultCatchHandler at "c:\code\dotnet\coreclr\src\vm\excep.cpp" @ 5840
                        CONTRACT in DefaultCatchHandler at "c:\code\dotnet\coreclr\src\vm\excep.cpp" @ 5793
                        CONTRACT in MethodDescCallSite::CallTargetWorker at "c:\code\dotnet\coreclr\src\vm\callhelpers.cpp" @ 372
                        GCX_COOP in Assembly::ExecuteMainMethod at "c:\code\dotnet\coreclr\src\vm\assembly.cpp" @ 2725
                        CONTRACT in Assembly::ExecuteMainMethod at "c:\code\dotnet\coreclr\src\vm\assembly.cpp" @ 2709
                        GCX_COOP in CorHost2::ExecuteAssembly at "c:\code\dotnet\coreclr\src\vm\corhost.cpp" @ 1334
                        ...

CORECLR! CONTRACT_ASSERT + 0x342 (0x00007ff8`8913acf2)
CORECLR! CrstBase::Enter + 0x15E (0x00007ff8`89266ebe)
CORECLR! CrstBase::AcquireLock + 0x15 (0x00007ff8`891e1295)
CORECLR! CrstBase::CrstHolder::CrstHolder + 0x25 (0x00007ff8`891dfcb5)
CORECLR! AppDomain::FindCachedAssembly + 0x34 (0x00007ff8`89525dc4)
CORECLR! Module::GetAssemblyIfLoaded + 0xAD0 (0x00007ff8`8952be00)
CORECLR! Module::LoadAssembly + 0x92A (0x00007ff8`8954e6ca)
CORECLR! Assembly::FindModuleByExportedType + 0x531 (0x00007ff8`89369341)
CORECLR! EEClassHashTable::UncompressModuleAndClassDef + 0x711 (0x00007ff8`89988691)
CORECLR! ClassLoader::FindClassModuleThrowing + 0xC88 (0x00007ff8`8945f028)
    File: c:\code\dotnet\coreclr\src\vm\crst.cpp Line: 358
    Image: C:\code\dotnet\coreclr\bin\Product\Windows_NT.x64.Debug\CoreRun.exe

But for this, I'm not really sure how to solve this, as it goes deeply into dependencies between components accessing concurrently cache of the AppDomain, so currently, the code is not threadsafe for this... The solution might be obvious for someone familiar with this codebase, or the problem is more fundamental...
Any ideas?

Ok, so the problem above is indirectly caused by deleting LoaderAllocator in the SystemDomain::ProcessDelayedUnloadDomains so it looks like there is some code running that is referencing some data allocated in a LoaderAllocator that has been deleted, causing weird side effects after (null reference...etc.)... will try to check if I can protect the memory for read/write on a fake delete and check if I get any luck...

I'm stuck with understanding what the ICLRPrivBinder is supposed to be...

When called on AssemblyNative::LoadFromPath, it is calling AssemblyNative::LoadFromPEImage which is setting the binder via

// We are working with custom Assembly Load Context so bind the assembly using it.
        CLRPrivBinderAssemblyLoadContext *pBinder = reinterpret_cast<CLRPrivBinderAssemblyLoadContext *>(pBinderContext);
        hr = pBinder->BindUsingPEImage(pImage, fIsNativeImage, &pAssembly);

At this line BindUsingPEImage there is the original CLRPrivBinderAssemblyLoadContext but then the Extract is providing a new dual interface there and storing this in the pAssembly

and after the PEAssembly is instantiated with this pAssembly... but then PEAssembly.GetBindingContext is in the end using PEFile::GetBindingContext which returns a different ICLRPrivBinder (the one bound through a dual interface on pAssembly)... so in the end, it is ending with a different BindingContext.

I'm a bit lost between these different ICLRPrivBinder, and I don't know if this is a cause of my issues (e.g they end up creating different entries in the AssemblySpecBindingCache because of these different binders)... I thought initially that I would have a single ICLRPrivBinder for a DomainAssembly, but this is not the case...

Could someone explain why we have two different ICLRPrivBinder for the DomainAssembly and the PEAssembly?

More precisely, the BINDER_SPACE::Assembly class inherits indirectly from ICLRPrivBinder (this is this one used by AssemblySpec when initializing from a PEAssembly file) but it has also a ICLRPrivBinder GetBinder() method which return the ICLRPrivBinder created for the ALC.
So how do they relate to each other? Which one should be used when attached to an AssemblySpec? (currently both are used, leading to different entries in the binding cache)

After finding some documentation about ICLRPrivBinder in the ICLRPrivBinder.idl file and from the BINDER_SPACE::Assembly, I'm starting to understand a bit more (but not a lot) how they relate to each other:

The calling sequence from AssemblyLoadContext::LoadFromPath used in a Load override is like this:

AssemblyNative::LoadFromPath
|- AssemblyNative::LoadFromPEImage
   | // create a BINDER_SPACE::Assembly which inherits from ICLRPrivAssembly and ICLRPrivBinder. 
   | // This binder is having binderALC as the parent binder
   | binderALC->BindUsingPEImage(...)
   | appDomain->LoadDomainAssembly
       appDomain->LoadDomainAssemblyInternal(...)
       |- appDomain->LoadDomainFile(...)
       |    appDomain->TryIncrementalLoad()
       |      domainFile->DoIncrementalLoad()
       |        domainAssembly->Allocate()
       |        |  // Add to AssemblySpecBindingCache with a Spec defined with binding context 
       |        |  // from BINDER_SPACE::Assembly (not the binderALC)  domainfile.cpp
       |        |- appDomain->AddAssemblyToCache (1)
       |  // Add to AssemblySpecBindingCache with the spec bound to binderALC
       |- appDomain->AddAssemblyToCache  (2)

So the (1) AddAssemblyToCache is using a spec bound to the BINDER_SPACE::Assembly allocated for the loaded assembly.
The (2) is bound to the binderALC.

My question is: Why (1) would not get the parent binder of BINDER_SPACE::Assembly, which in our case is binderALC to push it to the cache? Is it relevant to store this one? (as it looks like the spec is per assembly...)

In fact my original code from cleaning the AssemblyBindingCache was doing it on the DomainAssembly value and not the spec key, so It was working. But when I switched back to use an AssemblySpec to remove the Domainassembly from the cache, it was only able to remove the (1) but not (2). So (2) was later accessed through a AssemblyNative::Load but It was destroyed in the meantime (I still have to understand from where this AssemblyNative::Load is fired from the managed side)

So I guess that when trying to remove the cache (1), I will remove also (2) by getting the parent GetBinder() on it. Not sure if it is all clear and the right way to do it...

Any hints If I'm doing something obviously wrong would be appreciated! 😅

nevermind, the problem is somewhere else, the AssemblySpecBindingCache is using ultimately the BinderId which looks like the same for both... so still investigating...

So I just pushed a stable and working version. 🎉

Finally got rid of the caching problems by removing from the cache based on the DomainAssembly and not on the AssemblySpec (thought I had a bug but discovered that there was many entries with slightly different AssemblySpec in the AssemblySpecBindingCache while they could point to the same DomainAssembly)

The AssemblyLoadContext.cs implements also the UnLoading event on AppDomain.ProcessExit

There is a slight increase of memory when running over 1M of reload, but I haven't been able to find exactly what is leaking (tried taking Memory heap native snapshot from VS2015 but not sure how much it is reliable...)

There are a few bits not completely polished (missing a few #ifdef FEATURE_COLLECTIBLE_ALC and localized exception messages)... but before going further, I will wait for some feedback here! 😉

Going to look at the failing tests and how to add new tests related to collectible ALC.

Here is a small test that I have written that fails with the asserts in FileLoadLock::Release: https://gist.github.com/jkotas/0113226ff908484dca9b7353a5da51fd . I am not sure about the best way to fix them. It may be necessary simplify/refactor the various assembly loader hashtables to make this work well. Some of these tables are redundant and/or hard to reason about.

My test shows how to get this feature tested under stress, e.g. replace loading of hello.exe by loading all CoreCLR tests. https://github.com/Microsoft/dotnet-reliability has harness that we are using for stress testing. It may be a good idea to build a stress mode for unloadable ALC into it. cc @schaabs

Also, we will need to make sure that it works well with other features - interop, debugger, ... .

The overall direction looks reasonable to me so far. @xoofx Thank you for doing this!

@karelz All right, thanks :)

So, here's the official word to set expectations - likely not what most people hoped for (sorry for that ☹️), but at least it provides clarity:

• Majority of the remaining work is around testing (mostly stress testing). It will require some non-trivial investment (months).
• The work is not planned for 2.1 release.
• We do not have firm plans beyond 2.1 yet.

@karelz You're right, that's not what we want.

But remember that this is an open-source project now. So, what can the community do to help improve on that?

@masonwheeler The community (in the person of @xoofx) already went as far as it could, but if I remember correctly, the specialized test environment was (is?) not accessible to the community, so it was up to MS to finish the job.

In other words, this issue is dead.

@kwaclaw Sorry, but no. I'm not willing to accept that.

This is a feature that has not only been needed, but obviously needed, from the days of .NET Framework version 1. We've waited too long on Microsoft not doing anything about it. Now that it's finally available to the community to work on it, Microsoft continuing to not do anything about it is no longer a reasonable excuse.

What can the community do to improve this?

@DemiMarie That sounds like a good idea. Who here has experience with stress testing?

Very disappointing that this feature is not considered important. We really wanted to use .net core for a larger project but will now have to use another platform.

@DemiMarie I think a community-driven stress test won't work because if I remember the thread correctly, Microsoft would only accept the pull request if it passed their own stress test environment.

However, Microsoft is not able to share this environment with the community and has no intention of performing the stress testing themselves. Therefore I conclude that this pull request/feature is dead for the foreseeable future.

Months of work? Really? I find that hard to believe. This is really bad news. I am surprised that such a needed feature has been shelved.

There have a few problems that ms team could not be solved.

1.Not enough skilled C++ programmers.
2.there problem that all c++ code leaks a memory! Thats why a need a months to make it work! Threads are very leak memory sensivity!
3. Net core platform is not for enterprise development! Net core is for startup small projects ! Products that written on django and ruby in rails! Enterprise is not the main goal! The aim small teams, small projects thats why nobody want mess with leak memory!
4.there a problem that ms team has lost mobile market, and ms team want lose a small market of startup projects ! They do everything for azure platform dependency!
I learn net core, and im think many problem with net core! Many things that could not be cover by buisinnes requremns! Assembly is main goal of ernterprise development!

5. The reflection usage drop down .net permomance! Maybe why ms team dont like usage of reflectuions!

Whats going on here? Can someone from MS fix this?

@seriouz Yes, but they're choosing not to. Even though this is something that .NET has needed from the very beginning, which developers have been requesting from the very beginning, even though someone else has gone and done for them the work that they chose not to do, they won't integrate it until it's been tested in a special testing system that they're choosing not to use and they won't permit anyone else to do for them.

They could. But they aren't.

It's ridiculous...
Perhaps ms doesn't want to be next web app container standard promoting web devs with Java and Oracle (ironic inside) ?

The changes in this PR are a good prototype. It is very light on testing. It has not been hard to find problems in it by just looking at the code that should have been found by testing it well instead. Majority work on this feature is on testing and getting it to the right quality level. Our estimate is that this needs months, likely close to a year, of experienced runtime developer to make it happen. We were not able to dedicate one for this due to competing priorities. I do acknowledge that we could have handle this better. I am sorry.

The sources for the stress testing system are at https://github.com/Microsoft/dotnet-reliability. It is basically xunit with stress testing extensions. It is a special system, but we have not been actively preventing anybody from using it. Inventing a new system for testing this feature would be ok too similar to what we have for GC stress or JIT stress.

We are starting to plan for where we are going to invest once .NET Core 2.1 ships. Having better story for code unloading is high on our list. I am hopeful that it will become a part of plan for the next release.

@jkotas Thanks, that's really helpful! 👍

OK, so now we have the means to build a stress test environment. Anyone up for a bit of work on this?

Expect this function

Follow up PR #18476

Superseded by #18476