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

Load plugins in an AssemblyLoadContext #4916

Merged
merged 7 commits into from
Jan 9, 2020

Conversation

rainersigwald
Copy link
Member

@rainersigwald rainersigwald commented Nov 13, 2019

Use AssemblyLoadContext and the new AssemblyDependencyResolver to allow task implementations to have independent, incompatible dependencies on .NET Core.

Follows the spec from #4133 and the tutorial on creating .NET Core plugin hosts.

Since AssemblyDependencyResolver is a .NET Core 3.0 type, required targeting .NET Core 3.0.

Fixes #1754 (task ALC)
Fixes #4635 (logger ALC)

Copy link
Member Author

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-review

src/MSBuild/MSBuild.csproj Outdated Show resolved Hide resolved
@rainersigwald rainersigwald force-pushed the assemblyloadcontext branch 2 times, most recently from c4af6d0 to f38c2f2 Compare November 15, 2019 17:29
@rainersigwald
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@rainersigwald
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@rainersigwald rainersigwald marked this pull request as ready for review November 20, 2019 17:26
@rainersigwald
Copy link
Member Author

@vitek-karas would you mind taking a look at this PR using AssemblyDependencyResolver in MSBuild?



public MSBuildLoadContext(string assemblyPath) :
base(name: assemblyPath, isCollectible: false) // TODO: make this collectible?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way the MSBuild can decide to unload a task? Is there a natural trigger for it? If the answer is no, then don't enable unloading - just enabling it comes with a perf cost (not a big one, but still).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is moot now that I backed down to 2.1 and don't have the option, but: we don't have a great place to unload tasks. There's a fairly natural point for it--when a worker node is released, or when a build completes--but we're also likely to want to go past that point for the sake of performance: we don't really want to slow down inner-loop builds reloading Csc for every build. So I think we should just not make it collectible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe key the task load contexts based on the submission entry point's project path? If the new build submission is based on a different entry point, unload the previous one. Else, keep it.

src/Shared/MSBuildLoadContext.cs Show resolved Hide resolved
base(name: assemblyPath, isCollectible: false) // TODO: make this collectible?
{
_directory = assemblyPath;
_resolver = new AssemblyDependencyResolver(_directory);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to make sure this is intentional - this will look for task.deps.json next to the task.dll and if it's there it will use it - new and potentially breaking behavior.
If it's not there it will use all .dll files in the same folder as the dependencies - which is probably closer to the current behavior.

I'm not saying we should not do this - in fact not using the .deps.json will basically fix only half of the problem we could fix with this change. I'm just pointing out that this is potentially a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked about this with @livarcocc. We're planning to be conservative now (use the same behavior but in an ALC) and reconsider adding an AssemblyDependencyResolver later (probably in the .NET 5 timeframe). We agree that it's better moving forward but don't want to break folks if possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - this means that tasks with native (or platform specific dependencies) will still not be easy to write. For example SourceLink is such a task - where it needs to implement a two-layer approach and implement it's own ALC and custom resolution and so on. Such tasks carry some of their dependencies in subfolders which are keyed by the platform - so for example win/NativeLib.dll and linux/NativeLib.so - similarly for managed dependencies which are different per-platform. Using AssemblyDependencyResolver in some way would make this possible as it will be able to correctly pick the right one. The current solution will not help with these tasks.

I'm writing this down just to make it clear, I fully understand the desire to keep this as risk-free as possible.

There might be some way to combine both AssemblyDependencyResolver and plain directory search together to achieve a middle ground - but it would require .NET Core 3.0, so we're not there yet anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #5037 to make sure we come back to reevaluate this.

loadedAssembly = Assembly.UnsafeLoadFrom(assemblyLoadInfo.AssemblyFile);
#else
// If the Assembly is provided via a file path, the following rules are used to load the assembly:
// - if the simple name of the assembly exists in the same folder as msbuild.exe, then that assembly gets loaded, indifferent of the user specified path
// - otherwise, the assembly from the user specified path is loaded, if it exists.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly we're making a breaking change here.
Again, probably a good thing - but I think we need to be intentional about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the implementation I just pushed I added the MSBuild directory as a fallback within the ALC, which I think makes sense as a middle ground between "keep task assemblies with poorly-packaged dependencies happy" (mostly, unless they deeply depended on the overriding) and "best isolation" (a well-packaged task can use a different version of something in our directory now).

```sh-session
$ rep -r "#if !FEATURE_ASSEMBLY_LOADFROM" "#if
FEATURE_ASSEMBLYLOADCONTEXT" *.cs
1680 files processed. 4 files changed. 8 replacements.

$ rep -r "#if FEATURE_ASSEMBLY_LOADFROM" "#if
!FEATURE_ASSEMBLYLOADCONTEXT" *.cs
1680 files processed. 7 files changed. 29 replacements.
```

And manual move/rename of the definition.
Augment MSBuild's handling of assembly loading for plugins (currently tasks,
SDK resolvers, and loggers) to use an AssemblyLoadContext, allowing
plugins to use different versions of dependencies.

Creates a new ALC per assembly path (of initially-loaded assembly, not
indirect dependencies).

Closes dotnet#1754. Closes dotnet#4635.

Promote task isolation spec "possible solution"s to be the implemented
solution.
This broke the new test, and would remove much of the benefit of the ALC
because of the flattened directory structure of the .NET Core SDK.

But it might also be the reason some tasks keep going. So keep the MSBuild
directory as a fallback location.
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the question of loading "sort of shared" assemblies this looks good to me.
I think it would be nice to have a test which actually uses the important aspects of this - isolation.
And then another one which validates the intended behavior for "not well behaved tasks".

src/Shared/MSBuildLoadContext.cs Show resolved Hide resolved

if (FileSystems.Default.FileExists(assemblyNameInExecutableDirectory))
{
return LoadFromAssemblyPath(assemblyNameInExecutableDirectory);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will load a "copy" of the assembly into the task's ALC - it will not "share" it across ALCs.
Let's assume that there's an assembly "BuildUtils.dll" which lives in the msbuild directory. With the code as is, each task which has a dependency on BuildUtils will load its own copy of BuildUtils.
Not sure if this is the intended behavior - if so, I would explicitly state it in the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really interesting. I almost changed this to unify anything that's in the MSBuild directory. But I don't think that's a good idea long-term: right now, NuGet assemblies are there, so that would lock anyone who uses NuGet APIs to their current version. Since they've been known to make breaking changes, that could break folks.

The comment doesn't match the new behavior: the in-msbuild-directory check is now performed after looking through the plugin's directory with the culture search code above. So my next push proposes fixing the comment and the code to:

  • Check first in the plugin directory--if it's there, use the new ALC.
  • Fall back to the msbuild directory--if it's there, use the default ALC.

Does that sound reasonable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

foreach (var cultureSubfolder in string.IsNullOrEmpty(assemblyName.CultureName)
// If no culture is specified, attempt to load directly from
// the known dependency paths.
? new[] { string.Empty }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can cache the empty string array

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is moved code that we should be able to delete after we go full plugin, so I'm going to leave it alone for now.

// bare search directory if that fails.
: new[] { assemblyName.CultureName, string.Empty })
{
foreach (var extension in _extensions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if multiple extensions exist? does the array order actually signify a priority? If so, put a comment on it :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.


// If the Assembly is provided via a file path, the following rules are used to load the assembly:
// - if the simple name of the assembly exists in the same folder as msbuild.exe, then that assembly gets loaded, indifferent of the user specified path
// - otherwise, the assembly from the user specified path is loaded, if it exists.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this - otherwise case implemented by returning null and letting the base context resolve it? Would it be more explicit to just call base.Load?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base.Load will always return null, so there's no real difference. And it's the documented contract - return null from ALC.Load -> fallback to the Default ALC resolution.

Copy link
Contributor

@cdmihai cdmihai Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case though, wouldn't it be better to load that dll in the current context rather than default? Let's say a task has a dependency to an arbitrary path /a/b/newtonsoft.dll. By loading it in the default context it will hardcode the version of newtonsoft.dll between all tasks that point to newtonsoft via an arbitrary location.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically the reverse of your comment #4916 (comment): If the arbitrary dependency is under the msbuild bin directory, you do want sharing. If the arbitrary dependency is not in the msbuild bin dir, then I think you do not want sharing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the code the way it is now, plugins will get their own version if they have their own copy (as we search the plugins folder first and load into custom ALC if it's there). Only if the plugin doesn't have it, we fallback to Default.

I do agree that a slightly cleaner way to implement this would be to return null here and implement an event handler for AssemblyLoadContext.Default.Resolving which would resolve assemblies by looking into the msbuild directory (and load them to Default). Such a solution would make it consistent regardless of where the load comes from (task/plugin or msbuild itself). But if we're concern only with plugins/tasks then the end result is the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last case: what if an assembly is neither under the plugin's directory, nor under the msbuild bin directory? Currently it would be loaded in the default context, which could pollute other future tasks that load the same dll from a different, arbitrary path.


protected override Assembly? Load(AssemblyName assemblyName)
{
if (_wellKnownAssemblyNames.Contains(assemblyName.Name!))
Copy link
Contributor

@cdmihai cdmihai Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the requested msbuild version missmatches the currently running msbuild? Wouldn't it cause all sorts of type missmatches?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that exactly what this does? If it tries to load an MSBuild assembly, don't load it in the ALC and take the default-context one, unifiying to that version.

_directory = Directory.GetParent(assemblyPath).FullName;
}

protected override Assembly? Load(AssemblyName assemblyName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about framework assemblies? Should they be "binding redirected" to assemblies shared between all plugins? Otherwise you could get stuff like String cannot be cast to String, if communicate types between themselves (e.g. tasks and loggers)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to identify whether something is a framework assembly? The situation here would be that someone publishes their plugin as fully self-contained, including its own System* and netstandard stuff, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently SDK has no way to build a self-contained plugin (classlib project), so one would have to go outside of the supported SDK scenarios to create something like that. But in general plugins must not bring framework assemblies with them, otherwise all kinds of things can break. MSBuild could blacklist/whitelist assemblies just like it does so for MSBuild framework assemblies to make this a bit more robust. In theory MSBuild could enumerate all assemblies in the Default load context (not an easy thing to do, but possible) and "pin" all of them.

Another solution to this could be to not do anything in Load method and instead register event handler to the Resolving event on the ALC. The code flow then is that all bind requests will fall back to Default ALC first - and if Default can resolve it, it will do so. And only if Default can't resolve it it would get into the custom ALC's event handler. This would guarantee that all framework assemblies are loaded through Default only. But it would also mean that any other assemblies MSBuild itself uses would get the same treatment. So for example if MSBuild itself would use Newtonsoft.Json, all plugins would get that one version only. Doing this would probably require careful review of all MSBuild dependencies (And potentially work to move some of those into secondary ALCs - out of the Default ALC).

Note that there's "framework" assembly and then there's a different kind of "framework" assembly. Some framework assemblies (this happens specifically in ASP.NET, but in theory in any framework) can be overridden by the application. For example if a customer wants a patch for a specific issue and so on. Currently we allow apps to carry some framework assemblies with them and we pick the higher version from the app/framework pair to load. So blindly whitelisting all framework assemblies would disable this behavior for plugins. Maybe not a big deal... but worth knowing about.

And to answer the question - currently there's no standard way to ask if an assembly is a framework assembly. We could come up with pretty good "guesses", but without doing specific feature work in the runtime/BCL, it would never be 100%. (also as noted above, it's not always 100% clear what is a framework assembly in some cases).

Copy link
Contributor

@cdmihai cdmihai Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One "temporary" way, until the BCL gains some self descriptive capabilities, would be to look at msbuild's project.assets.json and define a whitelist. At the very least, look at what types are ferried across public msbuild interfaces / types (ITask, ILogger, ISDKResolver, etc) since a mismatch on those will definitely lead to runtime errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of notes (my own opinions):

  • Defining the list of types which can be shared between the MSbuild core and tasks is definitely worthwhile. One output of that can be the whitelist of assemblies (where those types are defined) for this purpose. It is a good idea no matter what as that will clearly define expectations on plugins.
  • We currently don't have any plans to teach BCL to determine "framework" assemblies. Note that marking something as "framework" is not as clear cut as it seems (especially for apps which override some framework assemblies and for self-contained apps). Also just having that functionality would not be enough for MSBuild - you would still need the whitelist of MSBuild's own "framework" on top of it.
  • SDK way of building plugins today effectively enforces "framework dependent" plugins and thus framework assemblies will not be part of the plugin in any way so there should be no collisions anyway. This is not 100% guaranteed, but it's close enough to reduce the importance of trying to solve this in some other way in my opinion.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • SDK way of building plugins today effectively enforces "framework dependent" plugins and thus framework assemblies will not be part of the plugin in any way so there should be no collisions anyway. This is not 100% guaranteed, but it's close enough to reduce the importance of trying to solve this in some other way in my opinion.

@vitek-karas Can you elaborate on how that statement is true and what the scope of that statement is? Are you saying if new AssemblyLoadContext's don't respect <RollForward>LatestMajor</RollForward> and I can't hack it by editing runtimeconfig.json? I'm not internal to Microsoft so I try to wrap my head around this as a customer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jzabroski What the statement I wrote meant is that:

  • Plugins should be built as classlib projects (producting a dll not an exe)
  • If that is true SDK currently has no way to produce a stand-alone classlib (as in classlib which contains frameworks copied into the output). All classlibs will be framework dependent (the build assumes that all the necessary dependencies will be available at runtime).
  • If the classlib is consumed via build (ProjectReference/PackageReference) the build system is the one to figure out all the dependencies and make sure they're available
  • If the classlib is loaded as a plugin then this job is left to the runtime/ALC. That's what the AssemblyDependencyResolver class is for - it implements the necessary logic to parse .deps.json (dependencies description produced by the build of the classlib) and resolve them at runtime. The implementation in this PR doesn't use AssemblyDependencyResolver (as it's only available in 3.0 and above), but it implements something which should work for the most part (searches for the assemblies in several locations)

Given the above it's very unlikely that framework assemblies (let's sat System.Xml.dll as an example) will be part of the plugins directory. Technically there is a way to force SDK to put it into the output, but it's an explicit action by the developer of the plugin and works on one assembly at a time. So almost no plugins will do this (why would they).

I must admit I don't understand how RollForward and runtimeconfig relates to this topic though. When loading plugins purely from managed code (As in this case), no runtimeconfig or rollforward is considered. It is assumed that the host (msbuild in this case) can resolve dependencies and if it can't it will fail.

Plugins may have runtimeconfig but that's for cases where they are loaded via native hosting APIs (and so they may start a runtime, in which case the runtimeconfig is used to determine which runtime/frameworks to load).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that marking something as "framework" is not as clear cut as it seems

True. Though one can conceive a list of "core language types", like String, IEnumerable, IDisposable, etc. Especially the types that the compiler knows about. But this analysis is beyond the scope of this PR :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Vitek. I think I am slowly closing my gap of knowledge on all the knobs you can turn in .NET Core to configure how assemblies get loaded. I wish there was a "CLR VIA C#" type chapter that explained all this, but I don't think there is one source of truth for all this information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jzabroski Thanks for the feedback - it's hard for me to tell which parts of the system are tricky to understand as I've been living in it for the last year 😄 . We have some docs in this area.
The overview is here: https://docs.microsoft.com/en-us/dotnet/core/dependency-loading/overview
And more details are in the Loading details section (in the TOC on the left). Of note are probably:

Some more details on how the managed assemblies in .deps.json are found is here: https://github.com/dotnet/runtime/blob/master/docs/design/features/host-probing.md (although it's probably a tiny bit out of date).

And then framework resolution is described here: https://github.com/dotnet/runtime/blob/master/docs/design/features/framework-version-resolution.md - but be warned, this is a really detailed doc - only if you're feeling hardcore about it 😉

@@ -152,35 +152,18 @@ private static Assembly LoadAssembly(AssemblyLoadInfo assemblyLoadInfo)
{
if (assemblyLoadInfo.AssemblyName != null)
{
#if FEATURE_ASSEMBLY_LOADFROM
#if !FEATURE_ASSEMBLYLOADCONTEXT
Copy link
Contributor

@cdmihai cdmihai Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the else branch use the custom ALC to avoid loading in the default ALC?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we identify the right context, though? Right now we have a mapping from plugin entry-point path to ALC; how would we find the right one based only on an assembly identity? Plus hopefully the only tasks identified by assembly name are MSBuild's built-in ones (on Framework they could also be GACed but that's less relevant here, and hopefully no one is modifying our .deps.json to find their own assemblies).

@cdmihai
Copy link
Contributor

cdmihai commented Dec 10, 2019

Another good set of test assertions would be to update the tests in TypeLoader_Tests and TypeLoader_Dependencies_Tests with assertions checking that no new types were loaded in the default context.

@jzabroski
Copy link

@rainersigwald
A great test case is to load System.ComponentModel.Annotations 4.7 nuget package via net48 and netstandard2.0 tasks, as net48 maps to assemblyVersion 4.2.1 (UGH!!!) and netstandard2.0 maps to assemblyVersion 4.2.0.

I checked the cache to see if something had been added, but never actually added anything . . .
var thisLoadContext = AssemblyLoadContext.GetLoadContext(typeof(ValidateAssemblyLoadContext).Assembly);

// Check by name because this always reports false, presumably due to ALC shenanigans:
// if (thisLoadContext is MSBuildLoadContext context)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you file a follow up issue for that commented out section? That definitely seems like a bug or at least an item worthy of documentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't understand what's going on here.

I patched it a bit to get more details (again, I think I did this before):

Log.LogMessage(MessageImportance.High, $"thisLoadContext is MSBuildLoadContext context : {thisLoadContext is MSBuildLoadContext context}");
Log.LogMessage(MessageImportance.High, $"thisLoadContext            loaded in AssemblyLoadContext {AssemblyLoadContext.GetLoadContext(thisLoadContext.GetType().Assembly)}");
Log.LogMessage(MessageImportance.High, $"typeof(MSBuildLoadContext) loaded in AssemblyLoadContext {AssemblyLoadContext.GetLoadContext(typeof(MSBuildLoadContext).Assembly)}");
  thisLoadContext is MSBuildLoadContext context : False
  thisLoadContext            loaded in AssemblyLoadContext "Default" System.Runtime.Loader.DefaultAssemblyLoadContext #2
  typeof(MSBuildLoadContext) loaded in AssemblyLoadContext "Default" System.Runtime.Loader.DefaultAssemblyLoadContext #2

My theory was that they were in different ALCs but that doesn't seem to be true.

@vitek-karas do you know what might cause this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The different ALCs would be my theory as well - but as you mention that doesn't seem to be the case. You can try to GetType on both ends of the comparison and look it up in the debugger (make object ID) to double check. If you can prove that the Type objects are the same, but "is" doesn't work - that would be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel uneasy that this isn't resolved. Please replace "shenanigans" with an explanation as what is happening. It feels like we don't even know if this is a bug or not yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to Nick offline I think I now understand what's going on. Because MSBuildLoadContext is compiled as source into MSBuild.exe and Microsoft.Build.dll, there are two copies of it available at runtime. MSBuild's copy is exposed to the test assembly via IVT, but Microsoft.Build's is the one that is relevant for loading the task. So in fact they don't match--even though they have the same namespace names they have different assembly-qualified names.

I'll amend the comment to describe this--I don't think it's worth juggling IVT right now. Filing an issue to come back and look at that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does IVT stand for? Intermediate Value Theorem?? Interrupt Vector Table?? InternalsVisibleTo?

lock (_guard)
{
Assembly assembly = TryGetWellKnownAssembly(context, assemblyName);
assembly = contextForAssemblyPath.LoadFromAssemblyPath(fullPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading inside a lock seems a bit dicey. Are there re-entrancy issues to be worried about here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely doesn't fill me with joy either. But we've been doing it inside a lock for a long time (the previous code did it too, into the default ALC), so it seems ok?

MSBuild calls this code when loading the entry-point assembly for:

  1. Loggers (serially, at application startup, before loading any other plugins)
  2. SDK resolvers (serially, on first SDK reference IIRC)
  3. Tasks (serially within a node since only one project builds at a time)

And each of those are serialized with the others. So I think it's really ok? Even if we started parallelizing logger/resolver initialization it should just serialize on the lock, and it's always just primary plugin assemblies that hit this--subsequent loads are handled outside the lock by the ALC.

lock (_guard)
{
Assembly assembly = TryGetWellKnownAssembly(context, assemblyName);
assembly = contextForAssemblyPath.LoadFromAssemblyPath(fullPath);

if (assembly != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't null loads be recorded as well? If not failed loads will just be attempted over and over again correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well - runtime caches the results of bind operations (including failures), so overall this should not be called too often really. That said if the cache is already in place, it would definitely help to use if for all assemblies, but I would not expect the perf gain to be substantial.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking around, it looks like there are static fields that keep a CoreCLRAssemblyLoader around for the lifetime of the process, which for a worker node could be long. If you:

  1. build
  2. it fails to load a task because you didn't restore so the file doesn't exist (or something)
  3. restore
  4. build

We need to try to load again in step 4 even if it failed in step 2.

(Did I make the change and get 80% though typing up a commit message justifying it before realizing this? Yes I did.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a conflict between above needing to work and the comment just above that says runtime caches failures?

Copy link
Member Author

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at adding a "stuff didn't get loaded into the default ALC" test in TypeLoader_Tests, but the tests there all spawn a separate MSBuild.exe process, so there's not a great place to programmatically inspect it.

"Microsoft.Build.Utilities.Core",
}.ToImmutableHashSet();

private static readonly string[] _extensions = new[] { "ni.dll", "ni.exe", "dll", "exe" };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate, but if I'm not mistaken ni.dll and ni.exe are holdovers from early days of CoreCLR and not something we need to support. Potentially could speed things up to stop probing for them. Or at least move them to the end of the list? Probably best as a separate issue, though, either way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking along the same lines but didn't bother to follow up. Filed #5042

The confusing situation can be resolved by an IVT change, filed as dotnet#5041.
@benvillalobos benvillalobos merged commit 86d9494 into dotnet:master Jan 9, 2020
StevenTCramer pushed a commit to TimeWarpEngineering/timewarp-architecture that referenced this pull request Feb 4, 2020
Fix to the bug dotnet/msbuild#4916
Update to latest Blazor-State 3.2.0
Update MediatR to 8.0
@rseanhall
Copy link

@rainersigwald Do you mind editing the description of this PR? The final version doesn't use AssemblyDependencyResolver, follow the referenced spec, or change the target of msbuild to .NET Core 3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AssemblyLoadContext for loggers Each MSBuild task should exist in its own AssemblyLoadContext
8 participants