Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API proposal AssemblyLoadContext.ActiveForContextSensitiveReflection #29042

Closed
sdmaclea opened this issue Mar 22, 2019 · 7 comments · Fixed by dotnet/corefx#36845
Closed

API proposal AssemblyLoadContext.ActiveForContextSensitiveReflection #29042

sdmaclea opened this issue Mar 22, 2019 · 7 comments · Fixed by dotnet/corefx#36845
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@sdmaclea
Copy link
Contributor

Problem

.NET Core 3.0 is trying to enable a simple isolated plugin loading model.

The issue is that the existing reflection API surface changes behavior depending on how the plugin dependencies are loaded. For the problematic APIs, the location of the Assembly directly calling the reflection API, is used to infer the AssemblyLoadContext for reflection loads.

Consider the following set of dependencies:

Assembly pluginLoader;     // Assume loaded in AssemblyLoadContext.Default
Assembly plugin;           // Assume loaded in custom AssemblyLoadContext
Assembly pluginDependency; // Behavior of plugin changes depending on where this is loaded.
Assembly framework;        // Required loaded in AssemblyLoadContext.Default 

The .NET Core isolation model allows pluginDependency to be loaded into three distinct places in order to satisfy the dependency of plugin:

  • AssemblyLoadContext.Default
  • Same custom AssemblyLoadContext as plugin
  • Different custom AssemblyLoadContext as plugin (unusual, but allowed)

Using pluginDependency to determine the AssemblyLoadContext used for loading leads to inconsistent behavior. The plugin expects pluginDependency to execute code on its behalf. Therefore it reasonably expects pluginDependency to use plugin's AssemblyLoadContext. It leads to unexpected behavior except when loaded in the "Same custom AssemblyLoadContext as plugin."

Failing Scenarios

Xunit story

We have been working on building a test harness in Xunit for running the CoreFX test suite inside AssemblyLoadContexts (each test case in its own context). This has proven to be somewhat difficult due to Xunit being a very reflection heavy codebase with tons of instances of types, assemblies, etc. being converted to strings and then fed through Activator. One of the main learnings is that it is not always obvious what will stay inside the “bounds” of an AssemblyLoadContext and what won’t. The basic rule of thumb is that any Assembly.Load() will result in the assembly being loaded onto the AssemblyLoadContext of the calling code, so if code loaded by an ALC calls Assembly.Load(...), the resulting assembly will be within the “bounds” of the ALC. This unfortunately breaks down in some cases, specifically when code calls Activator which lives in System.Private.CoreLib which is always shared.

System.Xaml

This problem also manifests when using an Object deserialization framework which allows specifying assembly qualified type names.

We have seen this issue when porting WPF tests to run in a component in an isolation context. These tests are using System.Xaml for deserialization. During deserialization, System.Xaml is using the affected APIs to create object instances using assembly-qualified type names.

Scope of affected APIs

The problem exists whenever a reflection API can trigger a load or bind of an Assembly and the intended AssemblyLoadContext is ambiguous.

Currently affected APIs

These APIs are using the immediate caller to determine the AssemblyLoadContext to use. As shown above the immediate caller is not necessarily the desired context.

These always trigger assembly loads and are always affected:

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);
    }
}
namespace System.Reflection
{
    public abstract partial class Assembly : ICustomAttributeProvider, ISerializable
    {
        public static Assembly Load(string assemblyString);
        public static Assembly Load(AssemblyName assemblyRef);
    }
}

These are only affected when they trigger assembly loads. Assembly loads for these occur when typeName includes a assembly-qualified type reference:

namespace System
{
    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);
    }
}

Unamiguous APIs related to affected APIs

namespace System
{
    public abstract partial class Type : MemberInfo, IReflect
    {
        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);
    }
}

In this case, assemblyResolver functionally specifies the explicit mechanism to load. This indicates the current assembly's AssmblyLoadContext is not being used. If the assemblyResolver is only serving as a first or last chance resolver, then these would also be in the set of affected APIs.

Should be affected APIs

Issue https://github.com/dotnet/coreclr/issues/22213, discusses scenarios in which various flavors of the API GetType() is not functioning correctly. As part of the analysis and fix of that issue, the set of affected APIs may increase.

Root cause analysis

In .NET Framework, plugin isolation was provided by creating multiple AppDomain instances. .NET Core dropped support for multiple AppDomain instances. Instead we introduced AssemblyLoadContext.

The isolation model for AssemblyLoadContext is very different from AppDomain. One major distinction was the existence of an ambient property AppDomain.CurrentDomain associated with the running code and its dependents. There is no equivalent ambient property for AssemblyLoadContext.

The issue is that the existing reflection API surface design was based on the existence of an ambient AppDomain.CurrentDomain associated with the current isolation environment. The AppDomain.CurrentDomain acted as the Assembly loader. (In .NET Core the loader function is conceptually attached to AssemblyLoadContext.)

Options

There are two main options:

  1. Add APIs which allow specifying an explicit callback to load assemblies. Guide customers to avoid using the APIs which just infer assembly loading semantics on their own.

  2. Add an ambient property which corresponds to the active AssemblyLoadContext.

We are already pursuing the first option. It is insufficient. For existing code with existing APIs this approach can be problematic.

The second option allows logical the separation of concerns. Code loaded into an isolation context does not really need to be concerned with how it was loaded. It should expect APIs to logically behave in the same way independent of loading.

This proposal is recommending pursuing the second option while continuing to pursue the first.

Proposed Solution

This proposal is for a mechanism for code to explicitly set a specific AssemblyLoadContext as the ActiveForContextSensitiveReflection for a using block and its asynchronous flow of control. Previous context is restored upon exiting the using block. Blocks can be nested.

AssemblyLoadContext.ActiveForContextSensitiveReflection

namespace System.Runtime.Loader
{
    public partial class AssemblyLoadContext
    {
        private static readonly AsyncLocal<AssemblyLoadContext> asyncLocalActiveContext = new AsyncLocal<AssemblyLoadContext>(null);
        public static AssemblyLoadContext ActiveForContextSensitiveReflection 
        { 
            get { return _asyncLocalActiveContext.Value; }
        }
    }
}

AssemblyLoadContext.ActiveForContextSensitiveReflection is a static read only property. Its value is changed through the API below.

AssemblyLoadContext.ActiveForContextSensitiveReflection property is an AsyncLocal<T>. This means there is a distinct value which is associated with each asynchronous control flow.

The initial value at application startup is null. The value for a new async block will be inherited from its parent.

When AssemblyLoadContext.ActiveForContextSensitiveReflection != null

When AssemblyLoadContext.ActiveForContextSensitiveReflection != null, ActiveForContextSensitiveReflection will act as the primary AssemblyLoadContext for the affected APIs.
When used in an affected API, the primary, will:

  • determine the set of known Assemblies and from which AssemblyLoadContext each Assembly is loaded.
  • get the first chance to AssemblyLoadContext.Load(...) before falling back to AssemblyLoadContext.Default to try to load from its TPA list.
  • fire its AssemblyLoadContext.Resolving event if the both of the preceding have failed
Key concepts
  • Each AssemblyLoadContext is required to be idempotent. This means when it is asked to load a specific Assembly by name, it must always return the same result. The result would include whether an Assembly load occurred and into which AssemblyLoadContext it was loaded.
  • The set of Assemblies related to an AssemblyLoadContext are not all loaded by the same AssemblyLoadContext. They collaborate. An assembly loaded into one AssemblyLoadContext, can resolve its dependent Assembly references from another AssemblyLoadContext.
  • The root framework (System.Private.Corelib.dll) is required to be loaded into the AssemblyLoadContext.Default. This means all custom AssemblyLoadContext depend on this code to implement fundamental code including the primitive types.
  • If an Assembly has static state, its state will be associated with its load location. Each load location will have its own static state. This can guide and constrain the isolation strategy.
  • AssemblyLoadContext loads lazily. Loads can be triggered for various reasons. Loads are often triggered as code begins to need the dependent Assembly. Triggers can come from any thread. Code using AssemblyLoadContext does not require external synchronization. Inherently this means that AssemblyLoadContext are required to load in a thread safe way.

When AssemblyLoadContext.ActiveForContextSensitiveReflection == null

The behavior of .NET Core will be unchanged. Specifically, the effective AssemblyLoadContext will continued to be inferred to be the ALC of the current
caller's Assembly.

AssemblyLoadContext.ActivateForContextSensitiveReflection()

The API for setting ActiveForContextSensitiveReflection is intended to be used in a using block.

namespace System.Runtime.Loader
{
   public partial class AssemblyLoadContext
   {
       public IDisposable ActivateForContextSensitiveReflection();

       static public IDisposable ActivateForContextSensitiveReflection(Assembly activating);
   }
}

Two methods are proposed.

  1. Activate this AssemblyLoadContext
  2. Activate the AssemblyLoadContext containing Assembly. This also serves as a mechanism to deactivate within a using block (ActivateForContextSensitiveReflection(null)).

Basic Usage

  AssemblyLoadContext alc = new AssemblyLoadContext();
  using (alc.ActivateForContextSensitiveReflection())
  {
    // AssemblyLoadContext.ActiveForContextSensitiveReflection == alc
    // In this block, alc acts as the primary Assembly loader for context sensitive reflection APIs.
    Assembly assembly = Assembly.Load(myPlugin);
  }

Maintaining and restoring original behavior

static void Main(string[] args)
{
  // On App startup, AssemblyLoadContext.ActiveForContextSensitiveReflection is null
  // Behavior prior to .NET Core 3.0 is unchanged
  Assembly assembly = Assembly.Load(myPlugin); // Will load into the Default ALC.
}

void SomeCallbackMethod()
{
  using (AssemblyLoadContext.ActivateForContextSensitiveReflection(null))
  {
    // AssemblyLoadContext.ActiveForContextSensitiveReflection is null
    // Behavior prior to .NET Core 3.0 is unchanged
    Assembly assembly = Assembly.Load(myPlugin); // Will load into the ALC containing SomeMethod(). 
  }
}

Proposed API changes

namespace System.Runtime.Loader
{
    public partial class AssemblyLoadContext
    {
        public static AssemblyLoadContext ActiveForContextSensitiveReflection { get { return _asyncLocalActiveContext.Value; }}

        public IDisposable ActivateForContextSensitiveReflection();

        static public IDisposable ActivateForContextSensitiveReflection(Assembly activating);
    }
}

Design doc

Detailed design doc is still in early review dotnet/coreclr#23335.

@sdmaclea sdmaclea self-assigned this Mar 22, 2019
@sdmaclea
Copy link
Contributor Author

/cc @vitek-karas @jkotas @jeffschwMSFT @jkoritzinsky @AaronRobinsonMSFT @swaroop-sridhar

I'll update the proposal based on the minimized comments in #28491.

@sdmaclea
Copy link
Contributor Author

I have updated the proposal to make it more self sufficient and readable. I incorporated @josalem's xunit experience as part of the justification.

@sdmaclea sdmaclea removed their assignment Mar 25, 2019
@sdmaclea
Copy link
Contributor Author

I am marking this api ready for review.

  • The API has been stable during the discussion except for naming.
  • The biggest issue has been in my ability to adequately describe the rationale. I believe the discussion in the other thread has helped clarify the story. I believe the story is reasonably mature and coherent.
    We can ask for review.

@vitek-karas
Copy link
Member

Couple comments:

  • You mention pluginClient but it is not mentioned anywhere else
  • Use consistent terminology. In various places it's either AssemblyLoadContext, ALC, "loader binder", "assembly loader" or LoaderAllocator. While they technically do refer to similar things, it makes the description pretty confusing. I would stick to one, probably AssemblyLoadContext (or ALC for short) as it's the externally visible term.
  • Please add a bit more detail on the XAML scenario - what is the XamlParser doing so that it causes problems? The level of detail between the XUnit and XAML scenarios is very different.
  • ALC's load are not async in the typical way "async" is used. I would instead say that they are triggered lazily thus from arbitrary location/thread and they are no externally synchronized. For those reasons the ALC must be thread safe.
  • I would remove the static public IDisposable Activate(Assembly activating); API - from the description it's unclear what it does, and it is just a two liner on top of the other API. This feature is not "Easy to use" so we don't need "helper" APIs for it I think.

In all API proposals, include the surrounding namespace and class so that it's immediately obvious where the API is begin added.

@sdmaclea
Copy link
Contributor Author

@vitek-karas I have made the grammatical/editorial corrections you suggested.

@terrajobst
Copy link
Member

terrajobst commented Mar 26, 2019

Video

namespace System.Runtime.Loader
{
    public partial class AssemblyLoadContext
    {
        public static AssemblyLoadContext CurrentContextualReflectionContext { get; }

        public ContextualReflectionScope EnterContextualReflection();
        static public ContextualReflectionScope EnterContextualReflection(Assembly activating);

        [EditorBrowsable(Never)]
        public struct ContextualReflectionScope : IDisposable
        {
        }
    }
}

@carlreinke
Copy link
Contributor

carlreinke commented Mar 27, 2019

Maybe Ambient would be a better term than Contextual given that it was the term used to describe the problem?

@sdmaclea sdmaclea self-assigned this Apr 4, 2019
sdmaclea referenced this issue in sdmaclea/coreclr Apr 4, 2019
Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213
sdmaclea referenced this issue in sdmaclea/coreclr Apr 5, 2019
Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213
sdmaclea referenced this issue in sdmaclea/coreclr Apr 10, 2019
Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213
sdmaclea referenced this issue in sdmaclea/coreclr Apr 11, 2019
Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213
sdmaclea referenced this issue in dotnet/coreclr Apr 11, 2019
* Add ContextualReflection APIs

Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213

* SetParentAssembly even when IsCollectible()
* ContextualReflection tests

* PR Feedback
* Add more usage tests

Add using statement tests
Add bad usage tests including Assert.Throws<>

* Only initialize on set
* Add XML API comments
* Unify VerifyIsolation
* Fix unused expectedAssembly
* Remove ContextualReflectionScope throw
* Clean up TestResolveMissingAssembly et. al
* Remove unused QCall::AppDomainHandle
* Remove AppDomainBaseObject
* Pass AssemblyLoadContext as managed object to native
* Fix AssemblyLoadContextBaseObject packing

* AssemblyLoadContext backing stores

Use explicit backing stores for events and properties

* Remove StaticAsyncLocalCurrentContextualReflectionContext
* Remove PermissionSetObject
Dotnet-GitSync-Bot referenced this issue in Dotnet-GitSync-Bot/corefx Apr 11, 2019
* Add ContextualReflection APIs

Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue dotnet#22213

* SetParentAssembly even when IsCollectible()
* ContextualReflection tests

* PR Feedback
* Add more usage tests

Add using statement tests
Add bad usage tests including Assert.Throws<>

* Only initialize on set
* Add XML API comments
* Unify VerifyIsolation
* Fix unused expectedAssembly
* Remove ContextualReflectionScope throw
* Clean up TestResolveMissingAssembly et. al
* Remove unused QCall::AppDomainHandle
* Remove AppDomainBaseObject
* Pass AssemblyLoadContext as managed object to native
* Fix AssemblyLoadContextBaseObject packing

* AssemblyLoadContext backing stores

Use explicit backing stores for events and properties

* Remove StaticAsyncLocalCurrentContextualReflectionContext
* Remove PermissionSetObject

Signed-off-by: dotnet-bot <[email protected]>
stephentoub referenced this issue in dotnet/corefx Apr 12, 2019
* Add ContextualReflection APIs

Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213

* SetParentAssembly even when IsCollectible()
* ContextualReflection tests

* PR Feedback
* Add more usage tests

Add using statement tests
Add bad usage tests including Assert.Throws<>

* Only initialize on set
* Add XML API comments
* Unify VerifyIsolation
* Fix unused expectedAssembly
* Remove ContextualReflectionScope throw
* Clean up TestResolveMissingAssembly et. al
* Remove unused QCall::AppDomainHandle
* Remove AppDomainBaseObject
* Pass AssemblyLoadContext as managed object to native
* Fix AssemblyLoadContextBaseObject packing

* AssemblyLoadContext backing stores

Use explicit backing stores for events and properties

* Remove StaticAsyncLocalCurrentContextualReflectionContext
* Remove PermissionSetObject

Signed-off-by: dotnet-bot <[email protected]>
Dotnet-GitSync-Bot referenced this issue in Dotnet-GitSync-Bot/corert Apr 12, 2019
* Add ContextualReflection APIs

Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213

* SetParentAssembly even when IsCollectible()
* ContextualReflection tests

* PR Feedback
* Add more usage tests

Add using statement tests
Add bad usage tests including Assert.Throws<>

* Only initialize on set
* Add XML API comments
* Unify VerifyIsolation
* Fix unused expectedAssembly
* Remove ContextualReflectionScope throw
* Clean up TestResolveMissingAssembly et. al
* Remove unused QCall::AppDomainHandle
* Remove AppDomainBaseObject
* Pass AssemblyLoadContext as managed object to native
* Fix AssemblyLoadContextBaseObject packing

* AssemblyLoadContext backing stores

Use explicit backing stores for events and properties

* Remove StaticAsyncLocalCurrentContextualReflectionContext
* Remove PermissionSetObject

Signed-off-by: dotnet-bot <[email protected]>
Dotnet-GitSync-Bot referenced this issue in Dotnet-GitSync-Bot/mono Apr 12, 2019
* Add ContextualReflection APIs

Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213

* SetParentAssembly even when IsCollectible()
* ContextualReflection tests

* PR Feedback
* Add more usage tests

Add using statement tests
Add bad usage tests including Assert.Throws<>

* Only initialize on set
* Add XML API comments
* Unify VerifyIsolation
* Fix unused expectedAssembly
* Remove ContextualReflectionScope throw
* Clean up TestResolveMissingAssembly et. al
* Remove unused QCall::AppDomainHandle
* Remove AppDomainBaseObject
* Pass AssemblyLoadContext as managed object to native
* Fix AssemblyLoadContextBaseObject packing

* AssemblyLoadContext backing stores

Use explicit backing stores for events and properties

* Remove StaticAsyncLocalCurrentContextualReflectionContext
* Remove PermissionSetObject

Signed-off-by: dotnet-bot <[email protected]>
marek-safar referenced this issue in mono/mono Apr 12, 2019
* Add ContextualReflection APIs

Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213

* SetParentAssembly even when IsCollectible()
* ContextualReflection tests

* PR Feedback
* Add more usage tests

Add using statement tests
Add bad usage tests including Assert.Throws<>

* Only initialize on set
* Add XML API comments
* Unify VerifyIsolation
* Fix unused expectedAssembly
* Remove ContextualReflectionScope throw
* Clean up TestResolveMissingAssembly et. al
* Remove unused QCall::AppDomainHandle
* Remove AppDomainBaseObject
* Pass AssemblyLoadContext as managed object to native
* Fix AssemblyLoadContextBaseObject packing

* AssemblyLoadContext backing stores

Use explicit backing stores for events and properties

* Remove StaticAsyncLocalCurrentContextualReflectionContext
* Remove PermissionSetObject

Signed-off-by: dotnet-bot <[email protected]>
jkotas referenced this issue in dotnet/corert Apr 14, 2019
* Add ContextualReflection APIs

Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213

* SetParentAssembly even when IsCollectible()
* ContextualReflection tests

* PR Feedback
* Add more usage tests

Add using statement tests
Add bad usage tests including Assert.Throws<>

* Only initialize on set
* Add XML API comments
* Unify VerifyIsolation
* Fix unused expectedAssembly
* Remove ContextualReflectionScope throw
* Clean up TestResolveMissingAssembly et. al
* Remove unused QCall::AppDomainHandle
* Remove AppDomainBaseObject
* Pass AssemblyLoadContext as managed object to native
* Fix AssemblyLoadContextBaseObject packing

* AssemblyLoadContext backing stores

Use explicit backing stores for events and properties

* Remove StaticAsyncLocalCurrentContextualReflectionContext
* Remove PermissionSetObject

Signed-off-by: dotnet-bot <[email protected]>
davmason referenced this issue in davmason/coreclr Apr 27, 2019
* Add ContextualReflection APIs

Add ContextualReflection APIs approved in dotnet/corefx#36236
Fix issue #22213

* SetParentAssembly even when IsCollectible()
* ContextualReflection tests

* PR Feedback
* Add more usage tests

Add using statement tests
Add bad usage tests including Assert.Throws<>

* Only initialize on set
* Add XML API comments
* Unify VerifyIsolation
* Fix unused expectedAssembly
* Remove ContextualReflectionScope throw
* Clean up TestResolveMissingAssembly et. al
* Remove unused QCall::AppDomainHandle
* Remove AppDomainBaseObject
* Pass AssemblyLoadContext as managed object to native
* Fix AssemblyLoadContextBaseObject packing

* AssemblyLoadContext backing stores

Use explicit backing stores for events and properties

* Remove StaticAsyncLocalCurrentContextualReflectionContext
* Remove PermissionSetObject
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants