Runtime: Add new API for collectible AssemblyLoadContext

[xoofx]

This is related to the undergoing work on coreclr: Collectible assemblies and AssemblyLoadContext

I would like to add the following API to the System.Runtime.Loader.AssemblyLoadContext:

// Allow to create a Collectible ALC. The default constructor will call this method with false
protected AssemblyLoadContext(bool isCollectible);

// Returns true if this ALC is collectible
public bool IsCollectible {get; }

// Allows to explicitly unload an ALC. Once this method is called,
// any call to LoadFromXXX method will throw an exception
public void Unload();

[karelz]

cc: @davidwrighton @jkotas

[xoofx]

cc: @gkhanna79 @kouvel

[AlexGhiondea]

Before we submit this for review, @davidwrighton @jkotas @gkhanna79 @kouvel -- any comments on this API?

[gkhanna79]

This API is part of the collectible load context work @xoofx has been driving.

CC @rahku @kouvel

[kouvel]

Looks good to me

[terrajobst]

What's the behavior of Unload?

• Presumbaly, Unload will throw an exception if the context isn't collectible. In that case it seems a bit odd that we use both "collectible" and "unload". We should probably use one terminology.
• Or does Unload work for any context and simply result in disallowing future loads?

[xoofx]

Currently, the behavior is to throw an exception if the ALC is not collectible. Do you think of using something like Collect()? (with the associating event Collecting - which would break the current Unloading event introduced).

We could also use the IDisposable pattern instead (I'm not a particular supporter of the Unload API actually) and keep the Unloading event as it is, but still the behavior for non collectible would not be clear (it would just disallow access to LoadFromXXX methods, but would not unload anything)

If It was possible, I would ditch IsCollectible/Unload at all and make ALC always collectible (and would fix the remaining issues that makes collectible assemblies less efficient than regular assemblies... a work that I would like to do anyway)

[jkotas]

If It was possible, I would ditch IsCollectible/Unload at all and make ALC always collectible

Unloadability will always have runtime overhead. It is important that folks pay this overhead only if they need it.

[xoofx]

Unloadability will always have runtime overhead.

Never looked exactly but while checking this, Is it mainly related to the empty CGCDescSeries *pSeries that are inserted in the code? (attached to FEATURE_COLLECTIBLE_TYPES)... looks like indeed the overhead should be avoided then...

[terrajobst]

To me "unload" assumes a user-initiated action. "Collection" implies an automated clean-up. We should be consistent with out nomenclature. If we call the feature "collectible load context" having an "Unload" that shows up on all load context but will only work when it's collectible load context seems surprising.

[xoofx]

What about a new CollectibleAssemblyLoadContext instead?

[kouvel]

We already have the event called Unloading that was part of the API surface for 1.0.0 and it would be a breaking change to change its name. Maybe we should stick with "unload" and call the parameter "canBeUnloaded", and the property maybe could also be "CanBeUnloaded"? The Unload method is user-initiated, but like Dispose, it may take some time for objects to be finalized after the Dispose call (and only after all references are released). And also like Dispose, I assume AssemblyLoadContext instances can be finalized before Unload is called. It seems to me like Unload has similar functionality to Dispose but with a more applicable name for this type.

[kouvel]

Any further thoughts on this?

[danmosemsft]

@terrajobst for question above. Would like to get this to review (or not)

[danmosemsft]

Setting ready for review so that Immo sees it

[terrajobst]

@kouvel

Your explanation makes total sense. So what about this?

namespace System.Runtime.Loader
{
    public class AssemblyLoadContext
    {
        // Allow to create an unloadable ALC. The default constructor
        // will call this method with false
        protected AssemblyLoadContext(bool unloadable);

        // Returns true if this ALC is collectible
        public bool Unloadable {get; }

        // Allows to explicitly unload an ALC. Once this method is called,
        // any call to LoadFromXXX method will throw an exception
        public void Unload();
    }
}

[gkhanna79]

Looks fine to me.

[kouvel]

Same here

[chuckeng-isp]

Looks good

[karelz]

@terrajobst @kouvel why is it marked as "api-approved", while it seems we're still discussing API shape? Or did I miss some point in the discussion?

[kouvel]

Should we prefix unloadable with is - isUnloadable / IsUnloadable?

[Lakritzator]

Is there a reason not to make this implement IDisposable?
And thus have the Unload be renamed (refactored) into an implementation of the Dispose method?

As it's still being used a lot, e.g. in System.Reactive, I assume the interface is not "dead" and makes a clear intention of what it does...

[kouvel]

A couple of things seem odd to me on using Dispose instead of Unload:

• Unload wants to fail to indicate error when the instance was not created for the purpose of being unloaded later. 'Dispose' is typically not expected to fail.
• The Unloading event (nevermind the name) may be raised long after Dispose is called at the actual time of unloading, not during the Dispose call as I would expect

[Lakritzator]

@kouvel The part of indicating an error, already disqualifies IDisposable. 👍
The second reason is the cream on top, thanks for explaining.

I agree, don't use IDisposable.

[black-byte]

Any time plans for solving this issue ?

[karelz]

This is a large feature. AFAIK it is near the top of our CoreCLR backlog, but I don't expect investments in next few months ...

[aurocoin]

Any track on this yet with 2.0 ?

[dazinator]

I assume you can never unload the default / top level AssemblyLoadContext is that correct? i.e similar to AppDomains?

It seems to me like the use case for a disposable / collectable ALC warrants a seperate class:

DisposableAssemblyLoadContext : IDisposable
AssemblyLoadContext

You can never upgrade a non collectable ALC to a collectable one, and a non collectable ALC can never be collected. At the point the developer creates the ALC, they could opt to create a DisposableAssemblyLoadContext which arguably expresses their intent more explicitly than passing a flag into the constructor of an AssemblyLoadContext

This also solves the following problem, because it can no longer happen (ALC's can't be disposed only DisposableALC can):

Unload wants to fail to indicate error when the instance was not created for the purpose of being unloaded later. 'Dispose' is typically not expected to fail.

Saying that, if a seperate class is not possible, then I would adovcate implementing IDisposable in ALC.

Developers are used to checking for IDisposable interface when managing lifetimes. It will also sit better with DI containers. For example, registering a Disposable ALC into an child / nested DI container will just work out of the box - when the container is disposed, the ALC will be disposed and collected as expected. I mention DI containers here, because both DI Containers and AppDomain's are often used to achieve levels of isolation / sandboxing within an application, and so I imagine use cases around isolating subsystems in an application will often involve both - and I am correlating this new collectible ALC with AppDomain.Unload.. However if you have this flag:

public bool Unloadable {get; }

This is not as smooth and straightforward as there now have to be custom branching conditions in the code based on checking this property.

[dazinator]

On a sidenote, with respect to:

Unload wants to fail to indicate error when the instance was not created for the purpose of being unloaded later. 'Dispose' is typically not expected to fail.

One simple way around that, when Disposing, if Unloadable is false - then don't call Unload.

[seriouz]

Any news on here, a half year has passed?
The pull request https://github.com/dotnet/coreclr/pull/8677 has still not be merged. 😢

[jvelezc]

merge this!

[jkotas]

@janvorli is on vacation. We will merge the PR once he is back in August, addresses the remaining codereview feedback, and is around to deal with the fallout of the changes.

[janvorli]

Implemented by dotnet/corefx#32002

[dazinator]

Shame it's not the IDisposable pattern for reasons mentioned above, but you can't always get everything you want in life I guess! Still a good feature! :-)