Runtime: AssemblyLoadContext .NET Core 3.0 improvements

Created on 24 Jan 2019  路  80Comments  路  Source: dotnet/runtime

Motivations

Concrete Type

There is a desire to simplify System.Runtime.Loader.AssemblyLoadContext usage by making it a concrete type. (from dotnet/runtime#28397)

The existing class is abstract because the Load() method is not implemented. Recommend adding a default implementation of Load() as a method returning null and make the constructors public. This is a logical default implementation as it implies falling back to the AssemblyLoadContext.Default for first chance to load the assembly.

Naming ALCs

To simplify diagnostics for System.Runtime.Loader.AssemblyLoadContext (from dotnet/runtime#28395):

  • Add a Name to each ALC making understanding of role of each ALC easier

The name will make it more convenient to understand what is happening in the debugger. It will also make any tracing easier to understand.

  • The Default ALC would be constructed with name = "Default".
  • The Assembly.LoadFile(path) generated ALCs would be constructed with name = "Assembly.LoadFile(_path_)" .
  • Custom AssemblyLoadContext implementations could provide a logical name.
  • The legacy Custom AssemblyLoadContext using the current protected constructor will not set the name. In these cases the name will default to null.

Set of ALCs & Assemblies

Provide a mechanism to iterate through ALCs and their contents to create a graphical representation. These APIs are intended to help make understanding and diagnosing isolation designs (from dotnet/runtime#28395).

Add Contexts, and Assemblies properties.

Proposed API changes

```C#
namespace System.Runtime.Loader
{
public partial class AssemblyLoadContext
{
// Add a public constructor
// Allow naming each AssemblyLoadContext
public AssemblyLoadContext(String name, bool isCollectible = false);

    // Add default Load() implementation
    protected virtual System.Reflection.Assembly Load(System.Reflection.AssemblyName assemblyName)
    {
        return null;
    }

    public String Name { get; }

    // Override the ToString to return Name, type name, and unique instance id.
    public override string ToString();

    // Set of ALCs
    public static IEnumerable<AssemblyLoadContext> Contexts { get; }

    // Set of Assemblies in the ALC
    public IEnumerable<System.Reflection.Assembly> Assemblies { get; }
}

}
```

Open issues

  • Consider renaming Contexts to AllContexts or GetAllContexts
api-approved area-System.Runtime blocking

Most helpful comment

Thanks... I am sure the API review would have fixed this, but it is better to be as close as possible.

OK back, to properties both returning IEnumerable<T>

All 80 comments

dotnet/runtime#28397 and dotnet/runtime#28395 have reached consensus. Since they are both affecting ALCs, it seemed best to close the original proposals to create a combined proposal.

I have marked this ready for review. The API review will help set the final shape.

/cc @jkotas @vitek-karas @swaroop-sridhar @jeffschwMSFT @terrajobst

@sdmaclea where did we land with the various extension events for missing Native/Managed assemblies? Can you link any other updates to ALC to this issue?

Double checking that my 'wish list' of having an early load callback (eg. the equivalent of TryLoad) is still not part of any plan, correct?

where did we land with the various extension events for missing Native/Managed assemblies? Can you link any other updates to ALC to this issue?

dotnet/runtime#27647 & dotnet/corefx#32015 are approved

I closed dotnet/runtime#28398 because I couldn't justify the API. @jkotas wanted to make sure we were not creating another way to break the consistency requirements for the ALC. If we fail to load an assembly by name in one context, the result is cached. We won't even try again. If we succeed in one context, we will always use the cached result. My main argument for having the parent assembly was for Satellite assemblies. Since we are solving this another way for dotnet/coreclr#20979, it seemed irrational to use it as an argument here.

Double checking that my 'wish list' of having an early load callback (eg. the equivalent of TryLoad) is still not part of any plan, correct?

From the history @vitek-karas told me, it was in the original design but was removed before .NET Core 1.0 because it caused too many deadlock issues for the Default context. I think if we were to add it, it would probably need to go into a derived class. Or fire an exception if there is a TryLoad event handler on Default.

We might be able to do it, but I think it could be controversial. I would like @jkotas & @vitek-karas opinions before seriously considering adding TryLoad

Correct, we had the Load extensibility point in the original .NET Core 1.0 implementation and it was full of infinite recursion land mines, impossible to use. It was no-brainer to pull that feature. If you think that we need something along these lines, I think we should start with discussing the concrete problems/scenarios that we are trying to solve.

Is there any particular reason that the Contexts member is an IEnumerator<AssemblyLoadContext> instead of an IEnumerable<AssemblyLoadContext>?

@jkoritzinsky Thanks for pointing out the unintended inconsistency.

IEnumerable<T> for creating IEnumerator<T>. I think logically it is added because an enumerator is logically an instance which is disposed when it is no longer needed.

Therefore, I think

  • using properties returning IEnumerator<T>, is semantically incorrect.
  • using properties returning IEnumerable<T> seems clumsy
  • methods returning IEnumerator<T> seems best.

I will update top post.

foreach (AssemblyLoadContext alc in AssemblyLoadContext.Contexts.GetEnumerator())

You don't need the GetEnumerator() call in a foreach statement. You can do foreach (AssemblyLoadContext alc in AssemblyLoadContext.Contexts) if Contexts is an IEnumerable<AssemblyLoadContext>. I at least find it clumsier to use IEnumerator<T> directly. Others may feel differently though.

Nowhere is IEnumerator ever used directly if not absolutely necessary. This is best for convenience reasons and now also for consistency.

Thanks... I am sure the API review would have fixed this, but it is better to be as close as possible.

OK back, to properties both returning IEnumerable<T>

/cc @lambdageek

A few questions:

  1. It seems weird that some ctors will default to collectible being false while the one taking a name defaults to true. That seems inconsistent. Having or not having a name feels logically independent to whether or not you want to be collectible.

  2. You said For legacy usage, these would default to the Custom ALC type name.. How would a customer have observed the name?

  3. I'm not entirely sure I understand why making the type concrete is useful if the default implementation can't resolve assemblies by name at all. Presumably, when I load one assembly by path, the defaut behavior should probe dependencies in at least that assembly's directory, no?

  4. Presumably both enumerators are protected against iteration when we're adding to them, correct? IOW, when I foreach over them and trigger a new context to be created or an assembly to be added, the enumeration will throw InvalidOperationException. I think this makes sense for the non-static Assemblies property but I don't think this is great for the static due to threading. I suspect you want snapshotting semantics instead, so I'd suggest the static property should really be a method that creates a copy of the collection as it exists at the moment.

  5. I'm not sure I understand the behavior of ExplicitContext. Why would I use it?

Let's spend an hour on this on 3/26. I'll send out invites.

  1. It seems weird that some ctors will default to collectible being false while the one taking a name defaults to true. That seems inconsistent. Having or not having a name feels logically independent to whether or not you want to be collectible.

I don't have a particular bias for the collectible defaulting to true, it may not have been intentional at all. I did wonder why the default was false for other constructors. I will update above to keep consistent with existing defaults.

  1. You said _For legacy usage, these would default to the Custom ALC type name._. How would a customer have observed the name?

Legacy code will not set the name because the API was missing. This is trying to establish a mechanism for creating a reasonable name in these legacy cases.

In the dotnet/corefx#22273 prototype I think I chose to leave the name null, but ToString() includes the type name. I revised spec to be consistent with the prototype.

  1. I'm not entirely sure I understand why making the type concrete is useful if the default implementation can't resolve assemblies by name at all. Presumably, when I load one assembly by path, the defaut behavior should probe dependencies in at least that assembly's directory, no?

In many cases returning null is sufficient. When the Load() method returns null, the default ALC gets a chance to load the Assembly, then the Custom ALC Resolving event handler, then the AppDomain AssemblyResolve event handler.

The only reason to override the Load() method would be to get first shot at loading an assembly.

  1. Presumably both enumerators are protected against iteration when we're adding to them, correct? IOW, when I foreach over them and trigger a new context to be created or an assembly to be added, the enumeration will throw InvalidOperationException. I think this makes sense for the non-static Assemblies property but I don't think this is great for the static due to threading. I suspect you want snapshotting semantics instead, so I'd suggest the static property should really be a method that creates a copy of the collection as it exists at the moment.

I intend both to be thread safe. With minimal locking. There is a prototype implementation in dotnet/corefx#22273. The Contexts prototype makes a quick copy while locked. The Assemblies one doesn't at the moment, but iterates over the result of AppDomain.GetAssemblies(), which should be snapshotting already.

  1. I'm not sure I understand the behavior of ExplicitContext. Why would I use it?

The current API in certain circumstances infer the ALC from the immediate caller's assembly and AssemblyLoadContext. This has proven problematic in certain circumstances. The deserialization of types by System.Xaml into a custom ALC was the poster child.

I am working on a more detailed design proposal now. If there is not consensus by 3/26 we can defer to another API review for preview5.

  1. seems weird that some ctors will default to collectible being false while the one taking a name defaults to true. That seems inconsistent. Having or not having a name feels logically independent to whether or not you want to be collectible.

All constructors should default to non-collectible. Marking the context as collectible carries a performance penalty for code loaded into the context. It should be explicit opt-in.

+1 on having a more detailed design proposal for the ExplicitContext.

I updated the proposal above in anticipation for the API review

  • Used markdown headers to break proposal into logical categories. Reorganized.
  • Added explanation why Load() { return null; } is a logical/reasonable default
  • Dropped the Contexts property. Added Parent and Children property to allow proper tree
  • Commented on the expected hierarchical assembly load fallback algorithm
  • Renamed ExplicitContext to ActiveContext (wanted an english verb/noun logical pair). Revised the proposal with excerpts from the in progress design doc.
  • Added a place holder for a citation to the design doc

Do we have a customer or real scenario for the Parent / Children? It looks really complex. AppDomains did not have Parent/Children properties on them either and I do not think anybody ever asked for it.

@jeffschwMSFT Asked how we would support nesting. As I was documenting it, it looked like it would be very difficult for a user to support. So I decided to propose it.

It feels like it will be complicated to implement. As you imply it is probably a corner case of a corner case.

I'll remove Parent & Children and restore Contexts

/cc @elinor-fung

implies falling back to the parent AssemblyLoadContext

This is not what actually happens - the fallback is always to the Default - there's no "parent"

Personal preference:
foreach (var ac in AssemblyLoadContext.Contexts) doesn't read right to me.

I think AllContexts would work better, or a method GetAllContexts...
But it's just a nit.

Per our discussion about nesting, I am OK with some scenarios being hard. As @jkotas points out, without a customer scenario I do not think the additional complexity is warranted.

// Convenience method to call AssemblyLoadContext on an assemblies parent
// Activate(null) will explicitly clear the ActiveContext until the IDisposable.Dispose()
static public IDisposable Activate(Assembly activating);

Similar to @vitek-karas 's comment, I am not sure what this is intended on doing.

// When set supersedes implicit context as determine by callers assembly.
// Affects most/all APIs which are (or will be) marked [System.Security.DynamicSecurityMethod]
// Implemented as AsyncLocal to allow setting to follow async & threads
public static AssemblyLoadContext ActiveContext { get { return _asyncLocalActiveContext.Value; }}

What is the default value of this parameter? When there is only 1 context (eg. set to Default)? The reason I am asking, is that can developers expect and can they reliably use it? Eg. do they always need to check if it is null before use (if (AssemblyLoadContext.ActiveContext != null) or AssemblyLoadContext?.ActiveContext ?

do they always need to check if it is null before use

Yes. Note that this API is for advanced users: our own framework and library writers.

do they always need to check if it is null before use
Yes. Note that this API is for advanced users: our own framework and library writers.

IMHO the core scenario for this feature is the implicit nature of the flow of ALC. What APIs will then respect that this is set? Type.GetType(), Assembly.LoadFrom? Others. I would like to see more about how this is going to help most developers with the stated issue the callers ALC not always being what we want to use.

What APIs will then respect that this is set? Type.GetType(), Assembly.LoadFrom? Others.

This question is important. Consider the scenario where we determine there are N APIs that are called that exhibit the need for this kind of scoped behavior. If N is small (e.g. 2 or 3, < 10), why not specialize those APIs instead of creating this API that is possible to breed very complex and difficult to diagnose issues without substantial logging.

why not specialize those APIs instead of creating this API

In ideal case, we would do this and tell everybody to call the alternative APIs that take explicit context instead (most or all these APIs have alternatives that do not operate on ambient context already).

The problem is with existing libraries that call the problematic APIs. They are expected to be slow to update to call the alternative APIs. I think the primary goal of this proposal is to solve the problem with existing libraries.

@jeffschwMSFT

do they always need to check if it is null before use

While I would like it to be set to Default on startup, this would be a breaking change. It would change the behavior of existing shipping code.

In many cases the old behavior is convenient. So it is nice for it still be set.

What APIs will then respect that this is set?

The list is incomplete because of dotnet/coreclr#22213, but this is the current set of APIs in coreclr which are marked [System.Security.DynamicSecurityMethod], and could be affected. I marked my working assumptions of the unaffected APIs // Will not be affected.
C# namespace System { public static partial class Activator { public static ObjectHandle CreateInstance(string assemblyName, string typeName); public static ObjectHandle CreateInstance(string assemblyName, string typeName, bool ignoreCase, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture, object[] activationAttributes); public static ObjectHandle CreateInstance(string assemblyName, string typeName, object[] activationAttributes); } public abstract partial class Type : MemberInfo, IReflect { public static Type GetType(string typeName, bool throwOnError, bool ignoreCase); public static Type GetType(string typeName, bool throwOnError); public static Type GetType(string typeName); public static Type GetType(string typeName, Func<AssemblyName, Assembly> assemblyResolver, Func<Assembly, string, bool, Type> typeResolver); public static Type GetType(string typeName, Func<AssemblyName, Assembly> assemblyResolver, Func<Assembly, string, bool, Type> typeResolver, bool throwOnError); public static Type GetType(string typeName, Func<AssemblyName, Assembly> assemblyResolver, Func<Assembly, string, bool, Type> typeResolver, bool throwOnError, bool ignoreCase); } } namespace System.Reflection { public abstract partial class Assembly : ICustomAttributeProvider, ISerializable { public static Assembly Load(string assemblyString); public static Assembly Load(AssemblyName assemblyRef); // Will not be affected public static Assembly GetExecutingAssembly(); // Will not be affected public static Assembly GetCallingAssembly(); } public abstract partial class MethodBase : MemberInfo { // Will not be affected public static MethodBase GetCurrentMethod(); // Will not be affected } internal class RuntimeAssembly : Assembly { // Will not be affected internal Assembly InternalGetSatelliteAssembly(CultureInfo culture, Version version, bool throwOnFileNotFound); } } namespace System.Reflection.Emit { public sealed class DynamicMethod : MethodInfo { // Will not be affected private static RuntimeModule GetDynamicMethodsModule(); } } namespace System.Runtime.Loader { public abstract partial class AssemblyLoadContext { // Will not be affected public Assembly LoadFromAssemblyName(AssemblyName assemblyName); } }

the fallback is always to the Default - there's no "parent"

This is left over from the proposal to add a Parent and revise the fallback algorithm which was removed.

I'll fix

Added an Affected APIs section.
Added an Open Issues section.
Fixed stray parent reference.
Fixed confusing comment.

/cc @josalem This is the API proposal @vitek-karas was referring to. See also dotnet/coreclr#23335

@sdmaclea, I think it is worth mentioning that setting AssemblyLoadContext.ActiveContext also affects raising the AssemblyLoadContext.ResolvingUnmanagedDll event for native library loads.

Since there are no parent ALCs any more, shouldn't the parent ALC argument be dropped?

public AssemblyLoadContext(String name, bool isCollectible = false, AssemblyLoadContext parent = ActiveContext ?? Default);

Also fix the parent ALC in the comments for Activate() method.

// Convenience method to call AssemblyLoadContext on an assemblies parent

setting AssemblyLoadContext.ActiveContext also affects raising the AssemblyLoadContext.ResolvingUnmanagedDll event

How so?

also affects raising the AssemblyLoadContext.ResolvingUnmanagedDll event for native library loads.

I hope so. All resolution should go through the current ALC rather than having some calls serviced by the current ALC and others serviced by another non-default ALC.

ResolvingUnmanagedDll should be always raised against the ALC of the assembly that is referencing the Unmanaged Dll. ActiveContext should not play in this directly at all.

ResolvingUnmanagedDll should be always raised against the ALC of the assembly that is referencing the Unmanaged Dll.

Bah! That seems a bit confusing. The Load() on the ALC can be triggered by callers, but the LoadUnmanagedDll() only applies to the enclosing Assembly?

I second.

I agree that loading unmanaged DLLs/PInvoke etc is closely related to the calling assembly (ex: per-module DllImportSearchPaths attribute etc). It is confusing if Load()/Resolving run from one ALC while LoadUnmanagedDll()/ResolvingUnmanagedDll run from another.

The user already has per-assembly callbacks NativeLibrary.SetDllImportResolver to control the native library load per assembly irrespective of ALCs.

Just so we are on the same page, consider the following:

class AlcA : AssemblyLoadContext
{
  protected override Assembly Load(AssemblyName assemblyName) { ... }
  protected override IntPtr LoadUnmanagedDll(string unmanagedDllName) { ... }
}

class AlcB : AssemblyLoadContext
{
  protected override Assembly Load(AssemblyName assemblyName) { ... }
  protected override IntPtr LoadUnmanagedDll(string unmanagedDllName) { ... }
}

AssmA - Loaded using AlcA
AssmB - Loaded using AlcB

So if we create a scenario like the following:

using (AssemblyLoadContext.Activate(AssmA))
{
   Type.GetType("FromB");
}

The above call to create a FromB instance will use AlcA to load a new AssmB, but if AssmB was already created then it will use AlcB for the native parts of the activation?

+1 to not affecting ResolvingUnamangedDll with this. That is unless there's a codepath I'm not aware of.

There are 3 sets of APIs:

  1. Those which take some kind of context - i.e. ALC.Load, ALC.LoadUnmanagedDll, Assembly.GetType (the instance based one). These clearly state which ALC to use - so no change to these.
    Metadata assembly reference resolution falls here as well - the context is the calling code.
    PInvoke resolution falls here as well - the context is the calling code.
  2. Those which don't need context since they are defined to use a fixed one - i.e. Assembly.LoadFile (always creates a new one) or Assembly.LoadFrom (always loads to default).
  3. Those which don't have any context or take it from the "caller" - i.e. Assembly.Load, Assembly.GetType (static) and so on.

Unless I'm mistaken the only way to call ResolvingUmanagedDll is through implicit PInvoke resolution. In that case the context is clearly defined by the calling code. Why would we want to override that?

@AaronRobinsonMSFT The answer should be:
No matter what happened before, FromB will be resolved against AlcA. That typically means loading a second copy of AssmB (if AlcA can actually resolve it). Such a copy would run its PInvoke resolution thorugh AlcA (since it belongs there).

In theory AlcA could resolve FromB by returning AssmB from AlcB (it's possible, not recommended). In that case there would be only one copy and PInvoke resolution would run through AlcB. but for this to happen AlcA would have to go out of its way - it would have to be a very explicit action - normal ALC will not do that.

This goes back to our discussion about ALCs being stable. If AlcA is stable, then it will always behave the same way and the PInvoke resolution would run through the same ALC, regardless of call order and load order.

@vitek-karas Okay that sounds right, but the verification I want is that when AssmB is loaded with AlcA, that is the ALC that will be used for P/Invokes and not AlcB even though the first instance of AssmB was loaded in AlcB right?

Let me just add that this feature should only affect APIs in point 3. from my comment above - APIs which don't have context or take it from the "caller". All the other APIs should be left as is.

That also means that all instance methods (properties) on ALC already have a context and should not be affected.

I will leave it to @sdmaclea or @swaroop-sridhar to answer in detail. There's some caching in PInvoke resolution - not sure exactly how that works. But in theory the answer should be YES. If there's a second copy of the same assembly (even the exact same bits on the disk) I would expect us to re-resolve all the PInvokes in it (using the new context). I would also expect this to already work (we re-JIT all the code it in for example, so why not re-resolve PInvokes).

@swaroop-sridhar Are you referring to the following scenario:

[DllImport(...)]
static extern void SomePInvoke();
...
using (AssemblyLoadContext.Activate(AssmA))
{
   SomePInvoke();
}

Where as my example where the type in AssmB that has a P/Invoke in the constructor does what I expect.

Unless I'm mistaken the only way to call ResolvingUmanagedDll is through implicit PInvoke resolution. In that case the context is clearly defined by the calling code. Why would we want to override that?

I'm not sure I understand this statement correctly. PInvoke resolution happens at the point of invocation, not the [DllImport ...] line. The Invocation call could be within a using() block.

Thanks @AaronRobinsonMSFT, that was the case I was thinking of.

@vitek-karas

Let me just add that this feature should only affect APIs in point 3. from my comment above - APIs which don't have context or take it from the "caller". All the other APIs should be left as is.

That also means that all instance methods (properties) on ALC already have a context and should not be affected.

The obvious exception is the case you describe here https://github.com/dotnet/coreclr/issues/22213#issuecomment-461478774 where two assemblies are required to resolve a generic type.

@swaroop-sridhar Yay. Okay, I am with @jkotas @vitek-karas and @sdmaclea. Since that resolution happens at invocation if we permit that behavior we are going to create an absolute mess. The issue here is that the mapping is sticky and there is a single slot for that function. It shouldn't affect the P/Invoke even though it would be the most consistent approach.

I'm actually a bit confused about the type loading within the ActiveContext blocks.

// Assembly A.dll <- loaded only into alc1
class C1
{
     void N() { ... }
}

class C2
{
   void M()
   {
       using (alc2.activate())
       {
            C1.N();
       }
   }
  • Does the use of C1.N() within C2.M() cause a new copy of A.dll to be loaded within alc2 (or the default context)?
  • If so, what will be GetAssembly(C1) and GetAssembly(C2) within the using() block? Will they be same or different?
  • Will GetAssembly(C2) inside and outside the using() blocks yield different answers?

@swaroop-sridhar Yes the invocation happens inside the using but that should not change anything. In short, if the code inside the using calls for example Newtonsoft.JSON for the first time - the resolution of that dependency should also ignore the using (the managed counterpart). Otherwise it's just a mess.

@sdmaclea RE dotnet/coreclr#22213 - I don't think it's an exception. Assembly.GetType (instance based) already works "correctly" - it takes the context from the assembly on which it's called.

In the generics case, if the inner type specifies full assembly name, currently it's loaded in Default - which is a bug. In my mind it should use the same context as for the non-generic case.

Basically myAssembly.GetType("Foo<Bar>") should behave the same as if I have code in myAssembly which does typeof(Foo<Bar>). The latter should clearly resolve all type refs in the context of the owning assembly (myAssembly). So the former should behave the same.

Basically myAssembly.GetType("Foo") should behave the same as if I have code in myAssembly which does typeof(Foo). The latter should clearly resolve all type refs in the context of the owning assembly (myAssembly). So the former should behave the same.

@vitek-karas Sounds correct, I had not made that assumption yet.

// When set supersedes implicit context as determine by callers assembly.
// Affects most/all APIs which are (or will be) marked [System.Security.DynamicSecurityMethod]
// Implemented as AsyncLocal to allow setting to follow async & threads
public static AssemblyLoadContext ActiveContext { get { return _asyncLocalActiveContext.Value; }}

I was chatting with @davidwrighton about our proposal and we got onto the semantics of this API. Pulling a comment I made back several comments ago about could this return null.

The reason I asked is that I think it is a reasonable pattern to ask developers to start using ActiveContext when loading instead of Assembly.Load or Default, etc. But that only makes sense IF it always returns something. So as a proposal, what if ActiveContext returns _asyncLocalActiveContext.Value (if not null) and returns the Callers context if null?

In this way, we could start advocating people (and plugins) to use ActiveContext to ensure they are loading within the right context. And we could allow the pattern of Activate(null) to rely on the 'use caller' pattern for loads. Hopefully this would alleviate the backward compat concern of changing this (since it would be semantically equivalent to what happens now).

The issue here is that the mapping is sticky and there is a single slot for that function.

In a way, the semantics is confusing both ways. I think the answers to these questions will help me process the issue better.

we could start advocating people (and plugins) to use ActiveContext

@jeffschwMSFT : What are the APIs that they would use this with? (A few concrete before/after code snippets.)

Not considering existing apps, they can continue to use what they want.

For new apps, the guidance is to use AssemlyLoadContext.Default.Load*, but I think we should recommend they use AssemblyLoadContext.ActiveContext.

(before)
Assembly.Load(...) OR AssemlyLoadContext.Default.Load*
(now)
AssemblyLoadContext.CurrentContext.LoadAssemblyFromPath(..)

we should recommend they use AssemblyLoadContext.ActiveContext

This depends on what you are doing:

If you are loading assembly for your own use (it is the most common scenario), you should keep using Assembly.Load(AssemblyName). Switching to AssemblyLoadContext.ActiveContext would be wrong.

AssemblyLoadContext.ActiveContext should be used only if you are loading assembly for somebody else (e.g. you are deserializer or similar component).

The answer is even more complex once you start talking about paths: Then the right answer really depends on what you are doing.

I'm actually a bit confused about the type loading within the ActiveContext blocks.

@jkotas suggested using a better name for Activate and ActiveContext in https://github.com/dotnet/coreclr/pull/23335#discussion_r266989250. At the time I didn't fully understand, but seeing the confusion it caused, I am agree the names need to be changed.

Looking at the set of Affected APIs, I can understand the use of Reflection to describe this set. Yet even that is too large. Since this is a fairly limited API, I am tempted to use a longer names:

  • ActivateForContextSensitiveReflection
  • ActiveForContextSensitiveReflection

It think this helps make your sample code more intuitive at first glance.
C# using (alc2.ActivateForContextSensitiveReflection()) { // Only changes the behavior for context sensitive reflection APIs. C1.N(); }

Does the use of C1.N() within C2.M() cause a new copy of A.dll to be loaded within alc2 (or the default context)?
If so, what will be GetAssembly(C1) and GetAssembly(C2) within the using() block? Will they be same or different?
Will GetAssembly(C2) inside and outside the using() blocks yield different answers?

None of these are using a reflection API so there would be no change.

I will update the proposal.

Regarding Assembly.Load and AssemblyLoadContext.Load* - I personally struggle with our guidance on when to recommend which set. I would like to see much better guidance from us (eg. customers can do what feels best for their apps, but we should have a recommendation).

If you are loading assembly for your own use (it is the most common scenario), you should keep using Assembly.Load(AssemblyName). Switching to AssemblyLoadContext.ActiveContext would be wrong.

I feel that it depends on the circumstatnces and is not clearly wrong. I also am not sure that all code has the ability to know if it is doing a load for itself or loading for somebody else. Eg. a plug-in is in essence loading on behalf of someone else (eg. the host would want the plug-in to load with in the context that it describes).

We can ensure that Assembly.Load and Type.GetType, etc. all do the right contextual thing. If customers use these (exclusively) than we are in an OK place. I was leaning towards having guidance that developers should stop using these in favor of ALC.Load*, and in that context using ActiveContext is always the right thing to do.

in that context using ActiveContext is always the right thing to do.

ActiveContext works only if somebody sets it up around the call to your component. If we start recommending it, it needs to come with guidance for where to strategically place the ActivateForContextSensitiveReflection calls that is hard to explain. It would work 100% if you had your component wrapped in auto-generated proxies that comes with a different set of problems.

I think our guidance should be:

  1. In apps or plugins, use the old fashioned Assembly.Load and Assembly.LoadFile
  2. In libraries (deserializers, dependency injection frameworks, etc.), avoid loading arbitrary assemblies and types on user behave. Defer the loading to the calling code. It makes the assembly loading into contexts work correctly, and it is best practice for secure programming.
  3. Consider using ActivateForContextSensitiveReflection as interim solution for libraries that end up calling the 3 problematic APIs and that are not easy to fix.

In other words, I see the ActivateForContextSensitiveReflection as a compat quirk to patch existing components. It is not see it as something we should encourage to spread everywhere.

ActivateForContextSensitiveReflection

馃憤 Thanks @sdmaclea; this definitely helps clarify the semantics of the API.

So, I'll modify my example above using reflection -- to understand the semantics.

```C#
public class ALC1 : AssemblyLoadContext { LoadUnmanagedDll(){...} }
public class ALC2 : AssemblyLoadContext { LoadUnmanagedDll(){...} }

// A.dll <-- Loaded into an instance of ALC1
namespace A
{
class C1
{
public static void N()
{
NativeFn();
}
}

class C2
{
   void M()
   {
       Type t1 = Type.GetType("A.C1");
       Assembly a1 = t1.Assembly;

       using (new ALC2().ActivateForContextSensitiveReflection())
       {
           Type t2 = Type.GetType("A.C1"); 
           Assembly a2 = t1.Assembly;

           if(toss == heads)
               t2.GetMethod("N").Invoke(null, null);
       }

       using (new ALC2().ActivateForContextSensitiveReflection())
       {
            if(toss == tails)
                C1.N();
       }
   }
}

[DllImport("NativeLib")]
static extern void NativeFN();

}
```

1) Will the reflection in t2 = Type.GetType("A.C1") cause a new copy of A.dll be loaded into ALC2?
That is, are a1 and a2 different?
2) In the first using block, where C1.N() is loaded via reflection, will NativeLib be resolved using LoadUnmanagedDll() in ALC2 or ALC1?
3) The second using block where C1.N() is called directly, my understanding is that NativeLib will be loaded via alc1.LoadUnmanagedDll() helper.

Thanks.

  1. No. Type.GetType (I assume that what you meant) looks in the caller assembly when the assembly name is not explicitly specified. I think it should stay that way. Neither of the two GetType("A.C1") in your example trigger any assembly loads. ActiveContext context should come into play for Type.GetType only when the assembly name is explicitly specified.
  2. ALC1
  3. Correct.

@jkotas I wrote the above code snippet based on the @sdmaclea's description above.

Are you suggesting that ActiveContext doesn't affect:
```C#
public static Type GetType(string typeName, bool throwOnError, bool ignoreCase);
public static Type GetType(string typeName, bool throwOnError);
public static Type GetType(string typeName);

In the case of GetType() APIs that take resolvers, 
```C#
        public static Type GetType(string typeName, Func<AssemblyName, Assembly> assemblyResolver, Func<Assembly, string, bool, Type> typeResolver);

if an assemblyResolver is not provided, then the default resolution will be based on the ALC marked ActivateForContextSensitiveReflection()? Which may load the current assembly again within that ALC?

Are you suggesting that ActiveContext doesn't affect:

It does affect these APIs, but not in the example you have provided.

Type.GetType looks in the caller assembly when the assembly name is not explicitly specified. I think it should stay that way

Interesting. I need to think about it...

Representative Type.GetType() calls with assembly qualified names:
c# // An assembly-qualified type Type.GetType("MyType,MyAssembly"); // A generic type with two assembly-qualified type arguments Type.GetType("MyGenericType`2[[MyType,MyAssembly],[AnotherType,AnotherAssembly]]") // An assembly-qualified generic type with an assembly-qualified type argument | 聽 Type.GetType("MyGenericType`1[[MyType,MyAssembly]],MyGenericTypeAssembly"); // A generic type whose type argument is a generic type with two type arguments Type.GetType("MyGenericType`1[AnotherGenericType`2[[MyType,MyAssembly],[AnotherType,AnotherAssembly]]]");

Thanks @jkotas, I misunderstood what you meant by AssemblyName -- it is the GetType() calls with AssemblyQualifiedName.

Thanks for the examples @sdmaclea. The sample in your description above "Basic Usage" section also needs to be updated.

Going back to my example:

```C#
using (new ALC2().ActivateForContextSensitiveReflection())
{
Type t2 = Type.GetType("A.C1, A, version=1.0.0.1, Culture=neutral, PublicKeyToken=aaaabbbb0000ffff");
Assembly a2 = t1.Assembly;

           if(toss == heads)
               t2.GetMethod("N").Invoke(null, null);

}

1) Does this cause assembly A to be loaded in ALC2? 
2) Consequently, is NativeLibrary resolved using ALC2's LoadUnmanagedLibrary() ? 

I agree that semantics of "calling assembly" lookup shouldn't change because of `ActiveContext`.
But it is a bit strange if 
```C#
Type t2 = Type.GetType("A.C1"); 

and
C# Type t2 = Type.GetType("A.C1, A, ...");
have very different semantics. (I guess it should be called out properly).

@swaroop-sridhar It really depends which GetType method you're using - the code above doesn't say so.

The static Type.GetType is one of the "context sensitive" APIs and thus would be affected by the using. So if you're using that one, then A will be resolved using ALC2 (if it will be actually loaded into ALC2 depends on ALC2 implementation and that is not clear from the sample).

There's also the Assembly.GetType which takes assembly instance. I think we need to exactly specify the behavior here, but in general I would think it should work in the context of the calling assembly. So it would NOT be one of the "context sensitive" APIs.

@sdmaclea would we consider adding a constructor that took an AssemblyDependencyResolver as input? That way the primary use case for a customer would look something like this...

AssemblyDependencyResolver resolver = new AssemblyDependencyResolver(pluginPath);
AssemblyLoadContext alc = new AssemblyLoadContext("My plugin", resolver);

In this way the Load would not just return null, it would resolve an assembly (if a resolver existed) and return null other wise.

My thinking here is that without this overload a customer would have to also know about the ADR (as it is separate) and wire up the event. If it were a parameter it is likely more discoverable.

I do not think we would to attach specific behaviors to the abstract base class. This should be a factory method, not a constructor. E.g.:

AssemblyDependencyResolver resolver = new AssemblyDependencyResolver(pluginPath);
AssemblyLoadContext alc = resolver.CreateAssemblyLoadContext("My plugin");

It really depends which GetType method you're using ...

@vitek-karas I had Type.GetType() in mind, similar to earlier posts. I've updated the sample now.

My point is that it is possible to construct/run-into use-patterns that will have somewhat unintuitive behavior -- because ActivateForContextSensitiveReflection only affects some APIs, in some situations. For example, it may be more straightforward if the 10 APIs that are affected by ActiveContext took an extra context parameter (or some other syntactical clue).

Anyway, since this API is intended for specific advanced use-cases, it's probably OK. We should document the behavior and samples well. Thanks.

```C#
namespace System.Runtime.Loader
{
public partial class AssemblyDependencyResolver
{
// Load assembly and dependencies into alc
public static System.Reflection.Assembly
LoadAssembly(string path, AssemblyLoadContext alc = AssemblyLoadContext.Default);

    // Load assembly and dependencies in custom AssemblyLoadContext
    //
    // Dependent assemblies will be loaded in AssemblyLoadContext.Default
    // if AssemblyLoadContext.Default already knows how to find them from
    // its TPA list. Otherwise it will be loaded in the custom 
    // AssemblyLoadContext
    //
    // This behavior can selectively be disabled with assemblyPreferIsolatedLoad. 
    // If the user supplied function returns true and the AssemblyDependencyResolver
    // can resolve the AssemblyName's path, then it will be loaded into 
    // the custom AssemblyLoadContext.
    //
    // The behavior for unmanaged libraries is similar. It can optionally
    // be controlled by the optional unmanagedDllPreferIsolatedLoad 
    // function.
    public static System.Reflection.Assembly 
    LoadAssemblyInNewAssemblyLoadContext(string path
                                         string alcName = null, // "LoadAssemblyInNewAssemblyLoadContext({0})" % path
                                         bool isCollectible = false,
                                         Func<bool, AssemblyName, AssemblyDependencyResolver> assemblyPreferIsolatedLoad  = null,
                                         Func<bool, string, AssemblyDependencyResolver> unmanagedDllPreferIsolatedLoad  = null);
}

}

With basic usage like 
```C#
Assembly myPlugin = AssemblyDependencyResolver.LoadAssembly(pluginPath);

or
C# Assembly myPlugin = AssemblyDependencyResolver.LoadAssemblyInNewAssemblyLoadContext(pluginPath);

Advanced usage could use assemblyPreferIsolatedLoad and/or unmanagedDllPreferIsolatedLoad to allow per dependency control over isolation.

Anyway, since this API is intended for specific advanced use-cases, it's probably OK. We should document the behavior and samples well. Thanks.

@swaroop-sridhar The confusion is helping me understand the gaps in documentation and samples.

Given the proposed API in https://github.com/dotnet/corefx/issues/34791#issuecomment-475426699

The goal is for these to behave as simarly as possible.

```C#
Assembly myPlugin = AssemblyDependencyResolver.LoadAssembly(pluginPath);

InvokeMyPlugIn(myPlugin);

```C#
Assembly myPlugin = AssemblyDependencyResolver.LoadAssemblyInNewAssemblyLoadContext(pluginPath);
using (AssemblyLoadContext.ActivateForContextSensitiveReflection(myPlugin))
{
    InvokeMyPlugIn(myPlugin);
}

The first use is simple. The plugin is loaded and all dependencies will be loaded into the default.

The second case is not as easy. The plugin is loaded into a custom Assembly load context with a subset of its dependencies. The remainder of its dependencies are loaded into AssemblyLoadContext.Default.

If any of these remainder dependencies call into the Context Sensitive Reflection APIs on behalf of the plugin, we need to make the behavior consistent. Sometimes the behavior needs to be unchanged. Other times it is important for them to act as if they were loaded into the same ALC as the plugin.

For Context Sensitive Reflection APIs, it is clear that when attempting to load an Assembly, the (correct) custom AssemblyLoadContext will need to be consulted. Otherwise there may be an unexpected type not found exception. Or the type may be loaded into the wrong AssemblyLoadContext leading to Type A cannot be cast to Type A exceptions.

For this case, based on @jkotas comments, I believe an assembly load is never triggered. Therefore the behavior would be unchanged. The very different semantics should be correctly preserved.
C# Type t2 = Type.GetType("A.C1");
Until I fully understand each of the context sensitive use cases, I will not be able to fully describe the effective change.

@jeffschwMSFT If it were a parameter it is likely more discoverable.

We can certainly add samples and docs cross references. I like the factory method @jkotas suggested. I will amend the proposal tomorrow if there are not objections.

Assembly myPlugin = AssemblyDependencyResolver.LoadAssembly(pluginPath);

I would not recommend expanding our Load* to AssemblyDependencyResolver. I would rather keep loading on AssemblyLoadContext and enable some sort of either constructor (which is my preference) or a factor method (which @jkotas suggested).

We can certainly add samples and docs cross references.

I agree this is required, but I would rather offer an intellisense option to help lead people in the right direction and not have to search to discover how to best use it. I feel one of our goals of making this class concrete is that it enables people to new and use. Having an intuitive way to pair that with the ADR would help even more.

Is there a reason we're trying to come up with "Easy to use" loading method... once again I might add :-)
I personally would prefer for us to solve the basics first. Then try to write some sample using the new APIs and see where it's getting overly complex. And maybe then try to add some "Easy to use" APIs.

Re some of the proposed APIs:
ADR.LoadAssembly(into alc) - I would like to see at least a rough idea how this would be implemented. Especially if it allows loading into Default. I don't think it's that simple - it would have to be based on event handlers... I guess.

The factory method which creates an ALC from ADR is much more realistic - it can provide its own implementation of ALC and override Load. But then again, it implies isolation policy...

Can we please split the discussion:

  • Basic improvements to ALC - get it through API review, make the change and so on
  • ActiveALC - figure out the detailed design - specifically all the APIs which are affected and how... get that solidified.
  • Easy to use APIs - come up with a couple of samples which would show why we need these and might help us with the API design.

Is there a reason we're trying to come up with "Easy to use" loading method

I hope that is our goal for .NET Core 3.0. ;)

Can we please split the discussion:

I am OK with splitting, the consideration is we likely have one of our last opportunities to make API level changes, and since this is drimane to this API I offered it as a suggestion. Though you are right, we need to close down the discussion and prepare for the current solid set of enhancements that we already have a great proposal for.

Overall, I am super happy with the advances we are making in this API and like most things am looking for even more.

OK I split the API Proposal. Created dotnet/corefx#36236 & dotnet/corefx#36237. Minimized all conversations related to the now off topic proposals. Updated the top post to remove split content.

@sdmaclea Could you please include a "Proposed API" block in C# syntax that describes the exact APIs being proposed in the top post? It will help to avoid confusion during the API review discussion.

(https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md has link to a an example of what a good API proposal should look like.)

Sorry accidentally deleted it

Video

  • Replace AssemblyLoadContext.Contexts with AssemblyLoadContext.All to make foreach easier to read
  • Other than that, looks good as proposed
Was this page helpful?
0 / 5 - 0 ratings

Related issues

matty-hall picture matty-hall  路  3Comments

chunseoklee picture chunseoklee  路  3Comments

btecu picture btecu  路  3Comments

jkotas picture jkotas  路  3Comments

omajid picture omajid  路  3Comments