Fix analyzer conflicts on Linux #69406

jaredpar · 2023-08-04T23:45:23Z

This changes analyzer loading on Linux to be per user where before clashes could occur in the temp directory.

This fix initially started out by making our temp directory logic more robust. Essentially add per user sub directories, do extended permission checks, etc ... That exacerbated a number of existing corner cases in the code and forced us to put a lot more error handling into the workspaces layer.

After discussion with the IDE team I decided to take the approach of loading from a Stream on non-Windows platforms. The performance difference is neglible (1ms to 2ms per assembly) and it removes the overhead of copying them around on disk as well as the complexities that come with managing the shadow copy directory.

This will be observable to any analyzer that used Assembly.Location as it will now be "". That is only useful though for inspecting disks which is already illegal for analyzers hence it's considered an acceptable break.

To test the performance of this change I wrote a simple benchmark that loads assemblies directly from path, copy then load (simulate shadow copy) and loading them from stream. The results on Windows is pretty striking

Method	Mean	Error	StdDev	Median
LoadViaPath	1.552 ms	0.0387 ms	0.1142 ms	1.529 ms
LoadViaPathShadow	33.147 ms	0.9000 ms	2.6537 ms	33.883 ms
LoadViaStream	495.945 ms	9.5040 ms	9.7599 ms	493.297 ms

Notice the penalty of LoadFromStream is severe on Windows. That is due to the AV having to run on every load since it can't cache like it can when loading from path. On Linux though the results are much nicer

Method	Mean	Error	StdDev	Median
LoadViaPath	1.301 ms	0.0258 ms	0.0326 ms	1.299 ms
LoadViaPathShadow	4.126 ms	0.3567 ms	1.0060 ms	3.742 ms
LoadViaStream	2.514 ms	0.0501 ms	0.0536 ms	2.497 m

This means the change should be a small win on Linux but probably not in the noticable range.

closes #65415

This changes analyzer loading on Linux to be per user where before clashes could occur in the temp directory. This fix initially started out by making our temp directory logic more robust. Essentially add per user sub directories, do extended permission checks, etc ... That exacerbated a number of existing corner cases in the code and forced us to put a lot more error handling into the workspaces layer. After discussion with the IDE team I decided to take the approach of loading from a `Stream` on non-Windows platforms. The performance difference is neglible (1ms to 2ms per assembly) and it removes the overhead of copying them around on disk as well as the complexities that come with managing the shadow copy directory. This will be observable to any analyzer that used `Assembly.Location` as it will now be `""`. That is only useful though for inspecting disks which is already illegal for analyzers hence it's considered an acceptable break. closes dotnet#65415

jaredpar · 2023-08-07T16:47:58Z

@dotnet/roslyn-compiler PTAL
@jasonmalinowski PTAL at IDE changes

jaredpar · 2023-08-07T16:49:12Z

src/Workspaces/Remote/ServiceHub/Host/RemoteAnalyzerAssemblyLoader.cs

@@ -18,6 +18,7 @@ internal sealed class RemoteAnalyzerAssemblyLoader : AnalyzerAssemblyLoader

        public RemoteAnalyzerAssemblyLoader(string baseDirectory)
        {
+            // jason should we load from stream here on Linux or is this VS only?


@jasonmalinowski I wanted you to look at this. The current state is always loading directly from disk (no shadow copy) and confirm it is the correct decision. If so I'll just leave this file alone (maybe add a comment to the effect). If this runs on Linux we could consider doing a stream based load here.

This file isn't used on Linux, so your question may be somewhat moot. But the comment here makes me think that this kicks in if we're loading analyzers that shipped in Roslyn itself, so reading directly from disk is fine since there's not locking concerns in the first place.

@genlu may also want to chime in here.

Yes, the sole purpose of this is to load our own assemblies in ServiceHub process

src/Compilers/Core/CodeAnalysisTest/AnalyzerAssemblyLoaderTests.cs

docs/Breaking API Changes.md

src/Compilers/Core/CodeAnalysisTest/InvokeUtil.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.Core.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.cs

For dotnet#69406 I updated several places where the IDE created analyzers. Even though I specifically called out the PR to the IDE team, I forgot I'd changed their code and merged once I had two compiler sign offs. This PR is commenting the parts of the IDE I changed in the PR so I can get actual sign off on these changes.

jasonmalinowski

Some comments left but nothing concerning -- I would have signed off on the original PR (except for that line tagging me. 😄)

jasonmalinowski · 2023-08-10T23:27:50Z

docs/Breaking API Changes.md

+# Version 4.8.0
+
+### Changed `Assembly.Location` behavior in non-Windows


Might want to adjust the title to say something about analyzers/generators so the context is clearer from the outset.

jasonmalinowski · 2023-08-10T23:28:34Z

docs/Breaking API Changes.md

+# Version 4.8.0
+
+### Changed `Assembly.Location` behavior in non-Windows


Massive kudos for remembering to update the docs!

Kudos go to @jjonescz for reminding me to do this

#69406 (comment)

jasonmalinowski · 2023-08-10T23:31:49Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.Core.cs

+                        using var stream = File.Open(assemblyPath, FileMode.Open, FileAccess.Read, FileShare.Read);
+                        return LoadFromStream(stream);


Random question: for the performance penalty on Windows due the virus scanner, is that because .NET is explicitly invoking the virus scanner on the stream data, or just because the virus scanner doesn't see this as a memory mapped file?

The runtime team said it has to do with caching. When the file is on disk, even when it's copied, the scans can be cached, while when it's in memory they cannot be. Feels like you could still do a SHA-256 cache but right now this is the reality 😦

jasonmalinowski · 2023-08-10T23:33:04Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.cs

@@ -17,14 +18,18 @@ internal sealed class DefaultAnalyzerAssemblyLoader : AnalyzerAssemblyLoader
    {
 #if NETCOREAPP

-        // Called from a netstandard2.0 project, so need to ensure a parameterless constructor is available.
        internal DefaultAnalyzerAssemblyLoader()


To @RikkiGibson's other question -- do we need this still? Because I don't quite understand the comment here that we had a net2.0 project calling something #ifdef'ed in a NETCOREAPP TFM?

There is a need for a parameterless ctor but I think it can be pulled out of the #if now and just have a single one.

jasonmalinowski · 2023-08-10T23:38:54Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.cs

+        /// will be the base directory where shadow copy assemblies are stored. </param>
+        internal static IAnalyzerAssemblyLoader CreateNonLockingLoader(string windowsShadowPath)
+        {
+#if NETCOREAPP


If you restructure this as:

#if NETCOREAPP if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { return new DefaultAnalyzerAssemblyLoader(loadOption: AnalyzerLoadOption.LoadFromStream); } #endif // (the contents of createShadowLoaderWindows)

You can shorten this up a bit and also remove the local function. That might make it a bit clearer that we are only doing something special on NETCOREAPP and non-Windows.

src/Compilers/Shared/BuildServerConnection.cs

jasonmalinowski · 2023-08-10T23:44:14Z

src/Workspaces/Remote/ServiceHub/Host/RemoteAnalyzerAssemblyLoader.cs

@@ -18,6 +18,7 @@ internal sealed class RemoteAnalyzerAssemblyLoader : AnalyzerAssemblyLoader

        public RemoteAnalyzerAssemblyLoader(string baseDirectory)
        {
+            // jason should we load from stream here on Linux or is this VS only?


This file isn't used on Linux, so your question may be somewhat moot. But the comment here makes me think that this kicks in if we're loading analyzers that shipped in Roslyn itself, so reading directly from disk is fine since there's not locking concerns in the first place.

jasonmalinowski · 2023-08-10T23:44:22Z

src/Workspaces/Remote/ServiceHub/Host/RemoteAnalyzerAssemblyLoader.cs

@@ -18,6 +18,7 @@ internal sealed class RemoteAnalyzerAssemblyLoader : AnalyzerAssemblyLoader

        public RemoteAnalyzerAssemblyLoader(string baseDirectory)
        {
+            // jason should we load from stream here on Linux or is this VS only?


@genlu may also want to chime in here.

This reverts commit f7cb1f2.

* IDE changes for analyzer loading For #69406 I updated several places where the IDE created analyzers. Even though I specifically called out the PR to the IDE team, I forgot I'd changed their code and merged once I had two compiler sign offs. This PR is commenting the parts of the IDE I changed in the PR so I can get actual sign off on these changes. * PR feedback

This fixes a bug where-by the compiler did not load satellite assemblies properly on Unix and Linux. The compiler recently changed (dotnet#69406) our loading strategy there to use `AssemblyLoadContext.LoadFromStream`. The runtime fallback logic for loading satellite assemblies is effectively the following: 1. Call `AssemblyLoadContext.Load(satelliteAssemblyName)` 2. If that fails call `AssemblyLoadContext.Load(realAssemblyName)` for the real assembly 3. Look at `Assembly.Location` on the return and change it to have the appropriate satellite path. 4. Call `AssemblyLoadContext.LoadFromPath(...)` on the result of (3) That logic breaks when assemblies are loaded via `LoadFromStream` as there is no location information. This changes our `AssemblyLoadContext` implementation to just handle satellite loads in step (1). This is done irrespective of whether the loader is loading via path or via stream. The loader has the information already so simpler to just handle it the same way every time. closes dotnet#56708 Note: these are [the docs](https://learn.microsoft.com/en-us/dotnet/core/dependency-loading/loading-resources) describing satellite assembly loading.

* Handle satellite assembly loading in .NET Core This fixes a bug where-by the compiler did not load satellite assemblies properly on Unix and Linux. The compiler recently changed (#69406) our loading strategy there to use `AssemblyLoadContext.LoadFromStream`. The runtime fallback logic for loading satellite assemblies is effectively the following: 1. Call `AssemblyLoadContext.Load(satelliteAssemblyName)` 2. If that fails call `AssemblyLoadContext.Load(realAssemblyName)` for the real assembly 3. Look at `Assembly.Location` on the return and change it to have the appropriate satellite path. 4. Call `AssemblyLoadContext.LoadFromPath(...)` on the result of (3) That logic breaks when assemblies are loaded via `LoadFromStream` as there is no location information. This changes our `AssemblyLoadContext` implementation to just handle satellite loads in step (1). This is done irrespective of whether the loader is loading via path or via stream. The loader has the information already so simpler to just handle it the same way every time. closes #56708 Note: these are [the docs](https://learn.microsoft.com/en-us/dotnet/core/dependency-loading/loading-resources) describing satellite assembly loading. * fixup * Apply suggestions from code review Co-authored-by: Jan Jones <[email protected]> * PR feedback * build failure * style issue * Apply suggestions from code review Co-authored-by: Rikki Gibson <[email protected]> * PR feedback --------- Co-authored-by: Jan Jones <[email protected]> Co-authored-by: Rikki Gibson <[email protected]>

dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 4, 2023

build-analysis bot mentioned this pull request Aug 5, 2023

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

more

1249be8

jaredpar marked this pull request as ready for review August 7, 2023 16:47

jaredpar requested review from 333fred, dibarbet and a team as code owners August 7, 2023 16:47

jaredpar commented Aug 7, 2023

View reviewed changes

jaredpar mentioned this pull request Aug 7, 2023

Rationalize how temporary directory works today #68226

Merged

PR feedback

30920a0

jjonescz reviewed Aug 8, 2023

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/AnalyzerAssemblyLoaderTests.cs Outdated Show resolved Hide resolved

PR feedback

02652a8

RikkiGibson self-assigned this Aug 8, 2023

RikkiGibson reviewed Aug 8, 2023

View reviewed changes

PR feedback

8ad2451

jaredpar added the Area-Compilers label Aug 8, 2023

RikkiGibson approved these changes Aug 8, 2023

View reviewed changes

PR feedback

7cff172

RikkiGibson approved these changes Aug 8, 2023

View reviewed changes

RikkiGibson mentioned this pull request Aug 9, 2023

add Assembly.Location to AnalyzerBannedApis? dotnet/roslyn-analyzers#6844

Open

jjonescz approved these changes Aug 9, 2023

View reviewed changes

jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Aug 9, 2023

jaredpar merged commit f7cb1f2 into dotnet:main Aug 9, 2023
27 checks passed

jaredpar deleted the temp-dir2 branch August 9, 2023 16:47

ghost added this to the Next milestone Aug 9, 2023

jaredpar mentioned this pull request Aug 10, 2023

IDE changes for analyzer loading #69470

Merged

jasonmalinowski reviewed Aug 10, 2023

View reviewed changes

dibarbet added a commit to dibarbet/roslyn that referenced this pull request Aug 14, 2023

Revert "Fix analyzer conflicts on Linux (dotnet#69406)"

49e7e87

This reverts commit f7cb1f2.

allisonchou mentioned this pull request Aug 16, 2023

[Automated] PRs inserted in VS build main-34016.170 #69571

Closed

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023

jaredpar mentioned this pull request Nov 10, 2023

Handle satellite assembly loading in .NET Core #70769

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix analyzer conflicts on Linux #69406

Fix analyzer conflicts on Linux #69406

jaredpar commented Aug 4, 2023 •

edited

Loading

jaredpar commented Aug 7, 2023

jaredpar Aug 7, 2023

jasonmalinowski Aug 10, 2023

jasonmalinowski Aug 10, 2023

genlu Aug 11, 2023

jasonmalinowski left a comment

jasonmalinowski Aug 10, 2023

jasonmalinowski Aug 10, 2023

jaredpar Aug 11, 2023

jasonmalinowski Aug 10, 2023

jaredpar Aug 11, 2023

jasonmalinowski Aug 10, 2023

jaredpar Aug 11, 2023

jasonmalinowski Aug 10, 2023

jasonmalinowski Aug 10, 2023

jasonmalinowski Aug 10, 2023

		# Version 4.8.0

		### Changed `Assembly.Location` behavior in non-Windows

		using var stream = File.Open(assemblyPath, FileMode.Open, FileAccess.Read, FileShare.Read);
		return LoadFromStream(stream);

Fix analyzer conflicts on Linux #69406

Fix analyzer conflicts on Linux #69406

Conversation

jaredpar commented Aug 4, 2023 • edited Loading

jaredpar commented Aug 7, 2023

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

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

jaredpar commented Aug 4, 2023 •

edited

Loading