Add an escape hatch for ALC isolation #5098
Conversation
|
|
||
| private static readonly ImmutableHashSet<string> _wellKnownAssemblyNames = | ||
| internal static readonly ImmutableHashSet<string> WellKnownAssemblyNames = |
There was a problem hiding this comment.
The only difference I see between this and _wellKnownAssemblyNames is that this includes MSBuild. It makes sense that that should be included; why wasn't it included initially?
There was a problem hiding this comment.
It generally doesn't need to be because MSBuild is what does the loading and thus doesn't need to be unified. I added it here to smooth out some of our unit tests that do things differently.
| } | ||
|
|
||
| private Assembly LoadUsingLegacyDefaultContext(string fullPath) | ||
| { |
There was a problem hiding this comment.
Since I don't think fullPath is supposed to be null, might it be good to enable nullable?
There was a problem hiding this comment.
This is an undelete for compat reasons; I'd rather not add additional points of failure at the same time.
| // of the Microsoft.Build.* assembly. | ||
| assemblyName.Version = _currentAssemblyVersion; | ||
|
|
||
| var searchPaths = new[] { Assembly.GetExecutingAssembly().Location }; |
There was a problem hiding this comment.
This would probably be bad for performance, but do you want to include something like "if we fail to find it at the expected location, recursively check all directories under ..?
There was a problem hiding this comment.
I am extremely averse to any "magical" assembly loading technique like that. This whole mess is bad enough already.
| // bare search directory if that fails. | ||
| : new[] { assemblyName.CultureName, string.Empty }) | ||
| { | ||
| foreach (var searchPath in searchPaths) |
There was a problem hiding this comment.
At what point are _dependencyPaths added? Is it somewhere buried in ComputeClosure?
There was a problem hiding this comment.
Nice catch! Fixed with 44af1ff
| { | ||
| foreach (var extension in MSBuildLoadContext.Extensions) | ||
| { | ||
| var candidatePath = Path.Combine(searchPath, |
There was a problem hiding this comment.
This makes me slightly nervous just because it vaguely resembles @benvillalobos's DependentUpon change.
There was a problem hiding this comment.
This emulates the .NET Framework's loader behavior. It can blessedly be deleted with #5037, but until then we have to do it ourselves. And again: this is not new code, just undeleted.
|
|
||
| if (_namesToAssemblies.TryGetValue(assemblyName.FullName, out assembly)) | ||
| { | ||
| return assembly; |
There was a problem hiding this comment.
Is there no need to check version here?
There was a problem hiding this comment.
Undeleting, so don't want to change behavior. But no, because .NET Core allows only one assembly per simple name in the same ALC, so if it's already loaded at some version, that's the one you get.
| /// <summary> | ||
| /// Disable AssemblyLoadContext isolation for plugins. | ||
| /// </summary> | ||
| public readonly bool UseSingleLoadContext = Environment.GetEnvironmentVariable("MSBUILDSINGLELOADCONTEXT") == "1"; |
There was a problem hiding this comment.
Do you think this should be opt-in?
There was a problem hiding this comment.
No, we want the new behavior by default, because we expect it to be better for most people. This PR just buys us time to fix problems if it's not . . .
| return assembly; | ||
| } | ||
|
|
||
| return TryResolveAssemblyFromPaths(context, assemblyName, _dependencyPaths); |
There was a problem hiding this comment.
This looks like mostly the same as the previous version—it just adds the possibility of cultures and custom search paths. Is that right?
There was a problem hiding this comment.
The same as what previous version? It should be the same as the v16.4 version.
There was a problem hiding this comment.
This was just a bad question. That's not what I meant, but I can't tell you what I did mean.
There was a problem hiding this comment.
Just confirmed that this successfully works around #5080 in context.
Restore the original load-with-funky-rules-into-the-default-ALC behavior under an environment-variable escape hatch. Fixes #5086.
This looks like a lot of code, but it's almost all just undeletions. I cherry-picked the change on top of the existing ALC changes, so you can see that more clearly: https://github.com/rainersigwald/msbuild/compare/alc-escape-hatch-base...rainersigwald:alc-escape-hatch-cleanerdiff?expand=1
Compare that to the diff in #4916 to see that it makes it look like this is just adding functionality.