Fix Loading of Plugin Assemblies #6189

brianrob · 2021-02-23T22:55:25Z

Context

#6126 changed the load behavior for NuGet dlls by forcing them to load in the default assembly load context. This effectively pinned their versions to the SDK, which is not a safe behavior for targets that may wish to carry their own versions of NuGet or other DLLs that they might load.

Changes Made

Rather than pinning specific DLLs, change the behavior to load all DLLs that are in the MSBuild directory in the default assembly load context. This allows targets to carry their own versions of dependencies, while avoiding loading assemblies that are packaged with MSBuild into multiple assembly load contexts, and thus losing the benefits of precompilation.

Testing

Verified that dotnet build hello-world has the correct load and jitting behavior.
Verified that tasks can load their own copies of NuGet binaries into the plugin assembly load context.

Notes

This reverts commit 183645c.

…d directory.

benvillalobos · 2021-02-23T23:08:10Z

Is there a way to create unit tests for this behavior?

Edit: Spoke too soon, looks like tests that cover this already exist and are failing.

brianrob · 2021-02-23T23:45:09Z

Yup, I am working on fixing that now.

ericstj · 2021-02-23T23:58:16Z

src/Shared/CoreCLRAssemblyLoader.cs

+            // the load is part of the platform, and load it using the Default ALC.
+            string assemblyDir = Path.GetDirectoryName(fullPath);
+
+            if (Traits.Instance.EscapeHatches.UseSingleLoadContext || string.Equals(assemblyDir, _msbuildDirPath))


Is any normalization needed for this path, or can you guarantee that a string comparison is sufficient? Things that could matter: relative vs full path, casing on some platforms, directory separator normalization, whitespace, extended prefixes (eg \??\)

I think both should be full paths to the directory with MSBuild, assuming they match, and since they describe the exact same file system path, I think they should be identical in casing, too.

My related question is: is this the right MSBuild.exe? If we're running in, say, VS, this point to the version of MSBuild associated with VS, not the version that's executing at the moment. Is that correct?

I have a commit that I'll post shortly that will normalize the path in the same way that the input is normalized, just to be sure. The cost should be insignificant, as we only do it once per instance of MSBuild.

@Forgind on your related question, I believe that this is the right MSBuild based on what you're saying. We'd want to be pointing at the VS MSBuild in that case, right? That would ensure that we only load DLLs in the task ALC for tasks that aren't "part of MSBuild".

I don't know, but I trust you.

Is it easy to test this? Things like:

Task.dll <msbuildDir>\sub\..\Task.dll C:\<msBuildPath>\Task.dll \<msbuildPath>\Task.dll

Presumably the path to the task is coming from the project/props/targets so it is user specified. I think there's also a version of UsingTask that uses StrongName. Does that need to be considered?

Assuming that can be fed in at the right level.

ViktorHofer · 2021-02-24T00:49:57Z

src/Shared/CoreCLRAssemblyLoader.cs

+            // the load is part of the platform, and load it using the Default ALC.
+            string assemblyDir = Path.GetDirectoryName(fullPath);
+
+            if (Traits.Instance.EscapeHatches.UseSingleLoadContext || string.Equals(assemblyDir, _msbuildDirPath))
            {
                return LoadUsingLegacyDefaultContext(fullPath);


nit: why is the default ALC called legacy?

Not sure. This is beyond my time in this code base.

The not-legacy ALC was a recent addition. rainersigwald tried to make it on by default, but that broke people, so he switched it to off-by-default but encouraged. It might have been turned on by default in the SDK; not sure.

ericstj · 2021-02-24T16:41:57Z

It might be interesting to measure the perf difference of this change compared to before the regression. It may provide greater wins than the first fix since it is sharing more.

ericstj

This change is a reasonable fix to the issue we faced. If you need to expedite the fix consider it approved: this is much better than current behavior. I think it'd be good to add some test coverage but that could be a separate PR.

brianrob · 2021-02-24T21:42:26Z

Discovered a bug around case sensitivity of paths. Consulted with @ladipro and landed at the latest iteration.

brianrob · 2021-02-24T21:43:11Z

@ericstj, I did some offline testing around the types of paths that you proposed above, and they are handled properly by the file path normalization logic.

Forgind · 2021-02-24T22:13:55Z

src/Shared/CoreCLRAssemblyLoader.cs

        {
-            _msbuildDirPath = FileUtilities.NormalizePath(BuildEnvironmentHelper.Instance.CurrentMSBuildExePath);
+            _msbuildDirPath = FileUtilities.NormalizePath(typeof(CoreClrAssemblyLoader).Assembly.Location);


ComparePathsNoThrow handles the normalization part of this for you.

Thanks @Forgind. I'd like to leave this here. The reason is that it happens only once per process, and has the ability to simplify the checks that must happen per assembly, instead of just once per process. If we don't do this here, and the value would have changed, then when loading an assembly, we'd have to do a string comparison which would fail, and then we'd have to normalize both inputs, and then try again.

If on the other hand, the initial normalize call here changed the input, it could possibly avoid the extra normalizations per assembly load.

ladipro · 2021-03-01T14:48:00Z

src/Shared/CoreCLRAssemblyLoader.cs

@@ -23,8 +23,15 @@ internal sealed class CoreClrAssemblyLoader

        private bool _resolvingHandlerHookedUp = false;

+        private static string _msbuildDirPath;


super-nit: readonly

brianrob · 2021-03-01T15:36:17Z

@marcpopMSFT, can you help me understand when this should be merged so that it doesn't impact the #6195?

marcpopMSFT · 2021-03-02T19:30:18Z

We have the final build for preview 2 now so we should be clear to get this in. However, our PR queue backed up again because of a sev 3 feed livesite incident where our build feed was taking many hours to update with new builds so we haulted our checkins. They say that's fixed now so we need to confirm and get our 11 approved PRs flowing again.

brianrob added 2 commits February 23, 2021 11:32

Revert "Add NuGet assemblies to the list of well-known assemblies."

cc30d98

This reverts commit 183645c.

Don't load assemblies in the plugin context if they are in the MSBuil…

3e6e653

…d directory.

brianrob requested review from ericstj and Forgind February 23, 2021 22:55

ericstj requested a review from ViktorHofer February 23, 2021 23:54

ericstj reviewed Feb 23, 2021

View reviewed changes

ViktorHofer reviewed Feb 24, 2021

View reviewed changes

brianrob added 2 commits February 23, 2021 17:18

Normalize the MSBuild dir path.

364eb66

Fix the assembly loader test.

0b3d41e

Forgind approved these changes Feb 24, 2021

View reviewed changes

ericstj approved these changes Feb 24, 2021

View reviewed changes

brianrob mentioned this pull request Feb 24, 2021

Regression when loading NuGet in MSBuild tasks #6186

Closed

Harden path checks and detection of MSBuild dll file path.

b86d06c

Forgind reviewed Feb 24, 2021

View reviewed changes

ladipro approved these changes Mar 1, 2021

View reviewed changes

Mark _msbuildDirPath readonly.

3de22d1

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 1, 2021

benvillalobos merged commit a71a130 into dotnet:master Mar 3, 2021

brianrob deleted the fix-nuget-load branch March 4, 2021 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Loading of Plugin Assemblies #6189

Fix Loading of Plugin Assemblies #6189

brianrob commented Feb 23, 2021

benvillalobos commented Feb 23, 2021 •

edited

Loading

brianrob commented Feb 23, 2021

ericstj Feb 23, 2021

Forgind Feb 24, 2021

brianrob Feb 24, 2021

Forgind Feb 24, 2021

ericstj Feb 24, 2021

ViktorHofer Feb 24, 2021

brianrob Feb 24, 2021

Forgind Feb 24, 2021

ericstj commented Feb 24, 2021

ericstj left a comment

brianrob commented Feb 24, 2021

brianrob commented Feb 24, 2021

Forgind Feb 24, 2021

brianrob Feb 24, 2021

ladipro Mar 1, 2021

brianrob commented Mar 1, 2021

marcpopMSFT commented Mar 2, 2021

		@@ -23,8 +23,15 @@ internal sealed class CoreClrAssemblyLoader

		private bool _resolvingHandlerHookedUp = false;

		private static string _msbuildDirPath;

Fix Loading of Plugin Assemblies #6189

Fix Loading of Plugin Assemblies #6189

Conversation

brianrob commented Feb 23, 2021

Context

Changes Made

Testing

Notes

benvillalobos commented Feb 23, 2021 • edited Loading

brianrob commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Feb 24, 2021

ericstj left a comment

Choose a reason for hiding this comment

brianrob commented Feb 24, 2021

brianrob commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrob commented Mar 1, 2021

marcpopMSFT commented Mar 2, 2021

benvillalobos commented Feb 23, 2021 •

edited

Loading