dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.91k stars 4.63k forks source link

[API Proposal]: AssemblyLoadContext.TryLoadFromAssemblyName #87185

Open jaredpar opened 1 year ago

jaredpar commented 1 year ago

Background and motivation

The C# compiler uses AssemblyLoadContext to isolate and manage analyzers and generators that plug into the compiler. Analyzers are passed to the compiler as a series of /analyzer:path/to/analyzer1.dll arguments. The compiler effectively groups these arguments by directory and creates an AssemblyLoadContext per directory.

The problem is that due to the nature of build, NuPkg authoring and analyzer detection, the compiler will end up getting passed DLLs that are not a part of the analyzer but instead part of the compiler. For example it's not uncommon to see /analyzer:System.Collections.Immutable.dll or /analyzer:System.Runtime.CompilerServices.Unsafe.dll to be passed as arguments. This is problematic because these DLLs contain exchange types. The compiler owns these DLLs and their copy must be used in both the compiler and analyzer for proper functioning. Loading the analyzer copy will lead to API mismatches later on that break the compilation process.

Today the only way to determine if the compiler owns the DLL is to first attempt to load the DLL into the compiler AssemblyLoadContext via LoadFromAssemblyName and if that succeeds use that DLL, otherwise load into the analyzer AssemblyLoadContext. That approach works great but has the downside that it introduces first chance FileNotFoundException instances because LoadFromAssemblyName throws on failure hence our core load path is as follows:

protected override Assembly? Load(AssemblyName assemblyName)
{
    var simpleName = assemblyName.Name!;
    try
    {
        if (_compilerLoadContext.LoadFromAssemblyName(assemblyName) is { } compilerAssembly)
        {
            return compilerAssembly;
        }
    }
    catch
    {
        // Expected to happen when the assembly cannot be resolved in the compiler / host
        // AssemblyLoadContext.
    }

    // Proceed with loading in the this analyzer AssemblyLoadContext

The compiler is hosted in a number of applications including Visual Studio. The Visual Studio team keeps tabs on first chance exceptions in core scenarios because it can contribute negatively to startup performance. This means the compiler and Visual Studio are at a tension point when it comes to one of our core scenarios. Every time we add or change analyzers / generators it introduces new first chance exceptions into the product, flags our insertions and requires discussion to resolve.

The motivation here is to have an API that does not throw here. Asking an AssemblyLoadContext to load an assembly and having it fail is not necessarily an exceptional item.

Note: happy to elaborate on why these unnecessary DLLs get passed by that is a problem inherent to both our ecosystem as well as other similar .NET plugin situations. Solving that is likely not realistic which is why the request for an API solution (it also seems reasonable by itself).

API Proposal

namespace System.Runtime.Loader

public class AssemblyLoadContext
{
    public bool TryLoadFromAssemblyName(AssemblyName assemblyName, [NotNullWhen(true)] out Assembly? assembly)
}

This API would function exactly as LoadFromAssemblyName does today except that it uses a bool to express failure instead of an exception.

API Usage

Given that code paths could change to the following

protected override Assembly? Load(AssemblyName assemblyName)
{
    var simpleName = assemblyName.Name!;
    if (_compilerLoadContext.TryLoadFromAssemblyName(assemblyName, out var compilerAssembly))
    {
        return compilerAssembly;
    }

    // Proceed with loading in the this analyzer AssemblyLoadContext

Alternative Designs

A potential alternative design is to have an oveload of LoadFromAssemblyName which has a throwOnError parameter similar to Type.GetType.

public class AssemblyLoadContext
{
    public Assembly? TryLoadFromAssemblyName(AssemblyName assemblyName, bool throwOnError)
}

That is undesirable for the following reasons:

  1. The API does not work well with nullable reference types.
  2. The Try pattern generally the standard approach for this style of method.

Risks

None that I can think of. In discussing with @elinor-fung and @davidwrighton they felt this was a reasonable approach to solving the problem.

ghost commented 1 year ago

Tagging subscribers to this area: @vitek-karas, @agocke, @vsadov See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation The C# compiler uses `AssemblyLoadContext` to isolate and manage analyzers and generators that plug into the compiler. Analyzers are passed to the compiler as a series of `/analyzer:path/to/analyzer1.dll` arguments. The compiler effectively groups these arguments by directory and creates an `AssemblyLoadContext` per directory. The problem is that due to the nature of build, NuPkg authoring and analyzer detection, the compiler will end up getting passed DLLs that are not a part of the analyzer but instead part of the compiler. For example it's not uncommon to see `/analyzer:System.Collections.Immutable.dll` or `/analyzer:System.Runtime.CompilerServices.Unsafe.dll` to be passed as arguments. This is problematic because these DLLs contain exchange types. The compiler _owns_ these DLLs and their copy must be used in both the compiler and analyzer for proper functioning. Loading the analyzer copy will lead to API mismatches later on that break the compilation process. Today the only way to determine if the compiler owns the DLL is to first attempt to load the DLL into the compiler `AssemblyLoadContext` via `LoadFromAssemblyName` and if that succeeds use that DLL, otherwise load into the analyzer `AssemblyLoadContext`. That approach works great but has the downside that it introduces first chance exceptions because `LoadFromAssemblyName` throws on failure hence our core load path is as follows: ```csharp protected override Assembly? Load(AssemblyName assemblyName) { var simpleName = assemblyName.Name!; try { if (_compilerLoadContext.LoadFromAssemblyName(assemblyName) is { } compilerAssembly) { return compilerAssembly; } } catch { // Expected to happen when the assembly cannot be resolved in the compiler / host // AssemblyLoadContext. } // Proceed with loading in the this analyzer AssemblyLoadContext ``` The compiler is hosted in a number of applications including Visual Studio. The Visual Studio team keeps tabs on first chance exceptions in core scenarios because it can contribute negatively to startup performance. This means the compiler and Visual Studio are at a tension point when it comes to one of our core scenarios. Every time we add or change analyzers / generators it introduces new first chance exceptions into the product, flags our insertions and requires discussion to resolve. The motivation here is to have an API that does not throw here. Asking an `AssemblyLoadContext` to load an assembly and having it fail is not necessarily an exceptional item. Note: happy to elaborate on why these unnecessary DLLs get passed by that is a problem inherent to both our ecosystem as well as other similar .NET plugin situations. Solving that is likely not realistic which is why the request for an API solution (it also seems reasonable by itself). ### API Proposal ```csharp namespace System.Runtime.Loader public class AssemblyLoadContext { public bool TryLoadFromAssemblyName(AssemblyName assemblyName, [NotNullWhen(true)] out Assembly? assembly) } ``` This API would function exactly as `LoadFromAssemblyName` does today except that it uses a `bool` to express failure instead of an exception. ### API Usage Given that code paths could change to the following ```csharp protected override Assembly? Load(AssemblyName assemblyName) { var simpleName = assemblyName.Name!; if (_compilerLoadContext.TryLoadFromAssemblyName(assemblyName, out var compilerAssembly)) { return compilerAssembly; } // Proceed with loading in the this analyzer AssemblyLoadContext ``` ### Alternative Designs A potential alternative design is to have an oveload of `LoadFromAssemblyName` which has a `throwOnError` parameter similar to `Type.GetType`. ```csharp public class AssemblyLoadContext { public Assembly? TryLoadFromAssemblyName(AssemblyName assemblyName, bool throwOnError) } ``` That is undesirable for the following reasons: 1. The API does not work well with nullable reference types. 2. The `Try` pattern generally the standard approach for this style of method. ### Risks None that I can think of. In discussing with @elinor-fung and @davidwrighton they felt this was a reasonable approach to solving the problem.
Author: jaredpar
Assignees: -
Labels: `api-suggestion`, `area-AssemblyLoader-coreclr`
Milestone: -
jkotas commented 1 year ago

Similar API proposal https://github.com/dotnet/runtime/issues/21785, with similar motivation, has been rejected by API review a few years back.

vitek-karas commented 1 year ago

The way I view this request is slightly different from #21785. The core question which this wants to answer is: "Can this ALC resolve this assembly name?". It doesn't necessarily need to actually resolve it (it can call the throwing method to do so). And in a way it may not need to be 100% correct (for example the context may know how to resolve it, but the file is missing on disk and the load eventually fails, or something like that).

Another way to look at this is that each ALC has a set of assembly names it knows how to resolve. But it never materializes the set - that's done lazily. The core of the question is "is my assembly name in that set?" - but we can't answer that with a lookup, it has to run the resolution algorithm.

Ultimately this is definitely doable, the problematic areas I can think of right now:

vitek-karas commented 1 year ago

/cc @elinor-fung

jkotas commented 1 year ago

Defining the desired functionality - if we want to just expose the "test" or we want the real "load but don't throw" behavior.

I do not think we would want to be creative with introducing new partial assembly loading models. The behavior of the API should 100% match the following implementation, except that it will try to avoid throwing and catching FileLoadExceptions internally where possible.

bool TryLoadFromAssemblyName(AssemblyName assemblyName, [NotNullWhen(true)] out Assembly? assembly)
{
    try
    {
        assembly = LoadFromAssemblyName(assemblyName(assemblyName);
        return true;
    }
    catch (FileLoadException e)
    {
        assembly = null;
        return false;
    }
}
jkotas commented 1 year ago

The Visual Studio team keeps tabs on first chance exceptions in core scenarios because it can contribute negatively to startup performance.

Note that we have work underway to make throwing and catching exceptions a lot cheaper. It would be useful to measure the cost of throwing and catching exception (after the perf improvements) and compare it with the total cost of analyzer assembly load in Roslyn scenarios. It is quite possible that this API would not actually result in any meaningful perf improvement.