Refactors LSP server extension assembly loading #71862

dibarbet · 2024-01-30T00:13:31Z

Fixes bug where multiple ALCs were created if two extension dlls had the same parent directory.
Provides better errors messages if extension paths are malformed.
Normalizes which assembly contributes to the MEF composition if provided multiple times
Loads analyzers provided by extensions into the same single extension ALC

Analyzers have same instance (and can load satellite assemblies):

Extension that does not ship VSTelemetry has same the instance as the host:

Two extensions have different instances of dependent assemblies:

Two extensions from the same extension directory have the same dependent assembly instance:

1. Fixes bug where multiple ALCs were created if two extension dlls had the same parent directory. 2. Provides better errors messages if extension paths are malformed. 3. Normalizes which assembly contributes to the MEF composition if provided multiple times 4. Loads analyzers provided by extensions into the same single extension ALC

dibarbet · 2024-01-30T00:15:14Z

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

+    public Assembly? TryLoadAssemblyInExtensionContext(AssemblyName assemblyName)
+    {
+        // We don't know exactly which extension the assembly came from, so we'll try each extension load context.
+        foreach (var loadContext in _directoryLoadContexts.Values)


cc @jasonmalinowski - I wasn't sure if there was a better way to do this. Basically the MEF custom assembly loader calls into us with an AssemblyName. I'm not sure how to get the actual path it is trying to load to find the exact extension ALC to use. There is a codebase path on AssemblyName, but its obsolete.

I could imagine that we could create an AssemblyName to path map in the constructor when you're initially walking through list of DLLs. Basically, you're restricting the list of potential assembly names that could be called to that list, but I think that's reasonable?

I'm not sure it strictly matters, but in some esoteric case like:

Extension A ships A.dll, a MEF component.

Extension B ships B.dll, a MEF component. It also ships A.dll, but not as a MEF component.

For the purposes of trying to get the MEF component for A.dll, you really want A, even if it's also shipped elsewhere. (and you'd want extension B getting it's own A.dll).

It's a bit unfortunate though that VS MEF can't give us a full path in this case?

@jasonmalinowski hmm I could do the assembly name map assuming that the assembly name from extension a and b are different.

It's a bit unfortunate though that VS MEF can't give us a full path in this case?

The assembly name code base does have the full path in this case, but its marked obsolete. I could suppress that warning and check it anyway?

The code base does have the full path in this case, but its marked obsolete. I could suppress that warning and check it anyway?

Oh I see: I was imagining you'd use the codebase given to the VS MEF IAssemblyLoader call that does have one, if it's available. But practically the only way we can ever make this work is if VS MEF is reliably able to tells the actual underlying path. If it's taking a model that you can't have more than one extension w/ the same assembly name then that might just be a limitation we have. And I can imagine that a least on VS for Windows, that's a reasonable limitation given the underlying .NET Framework limitations.

MEF in general (VS-MEF or otherwise) makes a very huge assumption that type full names (without their assembly names to qualify them) are unique in the MEF catalog. By default (and this comprises 99% of the cases) an import or export will use the Type.FullName as the MEF contract name, which is how imports and exports are matched.
If the same assembly is included in the catalog twice (whether or not it's the same version of the assembly) and that assembly includes MEF parts, it will most likely produce many cases of export duplication, which would bring down everyone who expected just one export (i.e. [Import]) and those who use [ImportMany] would get an export from each assembly.
You should therefore be sure to only include one assembly of a given name in your MEF catalog. You might choose the newest version if there are multiple to choose from.
If you include one in the MEF catalog but you allow other copies to also load (in other ALCs), that shouldn't confuse MEF, but it could still produce some confusing errors around type non-equivalence, and some of those errors could show up with MEF on the callstack.

You should therefore be sure to only include one assembly of a given name in your MEF catalog. You might choose the newest version if there are multiple to choose from.
If you include one in the MEF catalog but you allow other copies to also load (in other ALCs), that shouldn't confuse MEF, but it could still produce some confusing errors around type non-equivalence, and some of those errors could show up with MEF on the callstack.

Yeah that was my rough plan - we'll only include an assembly once into the catalog, but we'll allow an extension assembly to have other copies in its own ALC that do not contribute to the catalog.

Did you still want to do a map here so we're not having to do an O(n) loop?

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ExportProviderBuilder.cs

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/CustomExportAssemblyLoader.cs

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ExportProviderBuilder.cs

jasonmalinowski · 2024-01-30T00:55:28Z

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

+
+            // Create the extension assembly load context for the extension.
+            logger.LogTrace("Loading extension {assemblyFilePath}", assemblyFilePath);
+            var loadContext = new ExtensionAssemblyLoadContext(fileNameNoExt, directory, loggerFactory);


For the name for the ALC and the logger, I'd say make it a directory name (or even something like "Extension ALC for ...") so that way it's really clear where loads fail in the _logger.Trace() calls in the ALC.

(you can also then get rid of fileNameNoExt in that case)

the logger is created with the ALC name, so its already pre-pended there. Currently the ALC name is the file name without the extension, but I should change that to the directory name. Will do.

So tested this out a bit - using the assembly name for the ALC name seems the best to me. Using the directory name can get confusing if you have multiple extension from the 'bin' folder for example.

I guess I was still wanting the full directory name for a few reasons:

Using the first file name found more or less means we're depending on potentially nondetermnistic behavior -- if I have two DLLs like A.dll and B.dll, the ALC name is more or less arbitrary at that point.

Full directory name would also include things like extension name or extension version on the directory path, which might be good so if we find a problem with a random extension named "foo.dll" we have a bit more info to track it down.

Using the directory name can get confusing if you have multiple extension from the 'bin' folder for example.

To me this is the critical bit: if you have more than one extension in a bin folder, that's just one ALC!

Well in my case, I had two different extensions, both using a bin folder. So using just the directory name wouldn't tell me which was which. I could do the full path though.

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

....CodeAnalysis.LanguageServer/Services/StarredCompletions/StarredCompletionsAssemblyHelper.cs

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

jasonmalinowski · 2024-01-30T01:03:04Z

...s/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/VSCodeAnalyzerLoader.cs

+                return assembly;
+            }
+
+            return _defaultLoader.LoadFromPath(fullPath);


So this won't reuse it, right? Because this is going to give it a file path and then reload that again in the default implementation's ALC?

Yes, if we do not find the assembly in an extension ALC, it will fallback to the current behavior (which is a separate analyzer ALC).

1. Ensure an assembly only contributes to the MEF catalog once 2. Prefer the code base to load MEF assemblies 3. Load devkit dlls via hooking AssemblyLoadContext.Default.Resolve 4. Misc feedback

jasonmalinowski

I think the only change I'd make here is for the load-by-assembly-name that doesn't have a codebase, just do a map rather than just loading through all the ALCs as you're doing.

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/CustomExportAssemblyLoader.cs

jasonmalinowski · 2024-01-31T22:16:11Z

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

+
+            // Create the extension assembly load context for the extension.
+            logger.LogTrace("Loading extension {assemblyFilePath}", assemblyFilePath);
+            var loadContext = new ExtensionAssemblyLoadContext(fileNameNoExt, directory, loggerFactory);


I guess I was still wanting the full directory name for a few reasons:

Using the first file name found more or less means we're depending on potentially nondetermnistic behavior -- if I have two DLLs like A.dll and B.dll, the ALC name is more or less arbitrary at that point.

Full directory name would also include things like extension name or extension version on the directory path, which might be good so if we find a problem with a random extension named "foo.dll" we have a bit more info to track it down.

jasonmalinowski · 2024-01-31T22:16:36Z

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

+
+            // Create the extension assembly load context for the extension.
+            logger.LogTrace("Loading extension {assemblyFilePath}", assemblyFilePath);
+            var loadContext = new ExtensionAssemblyLoadContext(fileNameNoExt, directory, loggerFactory);


Using the directory name can get confusing if you have multiple extension from the 'bin' folder for example.

To me this is the critical bit: if you have more than one extension in a bin folder, that's just one ALC!

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

jasonmalinowski · 2024-01-31T22:29:45Z

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

+    public Assembly? TryLoadAssemblyInExtensionContext(AssemblyName assemblyName)
+    {
+        // We don't know exactly which extension the assembly came from, so we'll try each extension load context.
+        foreach (var loadContext in _directoryLoadContexts.Values)


Did you still want to do a map here so we're not having to do an O(n) loop?

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

1. Move duplicate checking to ExtensionAssemblyManager 2. Store mapping of assembly full name to load context to allow MEF to load an assembly by name from the correct load context. 3. Move devkit assembly resolution out of ExtensionAssemblyManager 4. Misc feedback

…oading_vscode

jasonmalinowski

I worry any further comments I make will only make this worse when I later realize I was wrong.

...r/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/LanguageServerTestComposition.cs

jasonmalinowski · 2024-02-02T20:36:30Z

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/CustomExportAssemblyLoader.cs

+        if (codeBasePath is not null)
+        {
+            return LoadAssemblyFromCodeBase(assemblyName, codeBasePath);
        }


Random thought: if we have a codebase path and we know the path isn't our application direectory, should have just done this first? Rather than that earlier load that we could have predicted would fail?

jasonmalinowski · 2024-02-02T20:40:17Z

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

+        var directoryLoadContexts = new Dictionary<string, AssemblyLoadContext>(StringComparer.Ordinal);
+        var assemblyFullNameToLoadContext = new Dictionary<string, AssemblyLoadContext>(StringComparer.Ordinal);


Consider using ImmutableDictionary builders, but doesn't really matter much.

...es/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Services/ExtensionAssemblyManager.cs

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 30, 2024

dibarbet commented Jan 30, 2024

View reviewed changes

jasonmalinowski reviewed Jan 30, 2024

View reviewed changes

dibarbet force-pushed the analyzer_assembly_loading_vscode branch 2 times, most recently from 4d9a0a1 to 3a54eb2 Compare January 31, 2024 00:23

Review feedback:

fe8e648

1. Ensure an assembly only contributes to the MEF catalog once 2. Prefer the code base to load MEF assemblies 3. Load devkit dlls via hooking AssemblyLoadContext.Default.Resolve 4. Misc feedback

dibarbet force-pushed the analyzer_assembly_loading_vscode branch from 3a54eb2 to fe8e648 Compare January 31, 2024 01:44

dibarbet marked this pull request as ready for review January 31, 2024 19:00

dibarbet requested a review from a team as a code owner January 31, 2024 19:00

jasonmalinowski approved these changes Jan 31, 2024

View reviewed changes

dibarbet added 3 commits January 31, 2024 18:18

Fix test now that devkit dependency loading happens in entry point

b1142c5

Merge remote-tracking branch 'upstream/main' into analyzer_assembly_l…

dc40ba3

…oading_vscode

jasonmalinowski approved these changes Feb 2, 2024

View reviewed changes

Review feedback

cdcf895

dibarbet merged commit c3d3595 into dotnet:main Feb 7, 2024
28 checks passed

dibarbet deleted the analyzer_assembly_loading_vscode branch February 7, 2024 00:55

ghost added this to the Next milestone Feb 7, 2024

jjonescz modified the milestones: Next, 17.10 P2 Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors LSP server extension assembly loading #71862

Refactors LSP server extension assembly loading #71862

dibarbet commented Jan 30, 2024 •

edited

Loading

dibarbet Jan 30, 2024

jasonmalinowski Jan 30, 2024

dibarbet Jan 30, 2024 •

edited

Loading

jasonmalinowski Jan 30, 2024

AArnott Jan 30, 2024

dibarbet Jan 31, 2024

jasonmalinowski Jan 31, 2024

jasonmalinowski Jan 30, 2024

jasonmalinowski Jan 30, 2024

dibarbet Jan 30, 2024 •

edited

Loading

dibarbet Jan 31, 2024

jasonmalinowski Jan 31, 2024

jasonmalinowski Jan 31, 2024

dibarbet Jan 31, 2024

jasonmalinowski Jan 30, 2024

dibarbet Jan 30, 2024

jasonmalinowski left a comment

jasonmalinowski Jan 31, 2024

jasonmalinowski Jan 31, 2024

jasonmalinowski Jan 31, 2024

jasonmalinowski left a comment

jasonmalinowski Feb 2, 2024

jasonmalinowski Feb 2, 2024

		var directoryLoadContexts = new Dictionary<string, AssemblyLoadContext>(StringComparer.Ordinal);
		var assemblyFullNameToLoadContext = new Dictionary<string, AssemblyLoadContext>(StringComparer.Ordinal);

Refactors LSP server extension assembly loading #71862

Refactors LSP server extension assembly loading #71862

Conversation

dibarbet commented Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet Jan 30, 2024 • edited Loading

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

dibarbet Jan 30, 2024 • edited Loading

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

jasonmalinowski left a 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

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet commented Jan 30, 2024 •

edited

Loading

dibarbet Jan 30, 2024 •

edited

Loading

dibarbet Jan 30, 2024 •

edited

Loading