Synchronize razor compiler assembly loading #11394

chsienki · 2025-01-16T00:56:00Z

This PR ensures that the razor compiler is only loaded a single time between Roslyn and Razor. Whichever succeeds in first loading the assembly will provide their copy to the other.

There is a doc in the first commit that explains the strategy. You should read this before reviewing the rest of the PR.

Note this is blocked on dotnet/roslyn#76752 but it wont require code changes other than updating the roslyn version.

- Add a new ALC that is 'razor compiler' aware - Atomically load the razor compiler or use a copy already loaded by roslyn

- When creating a new service instance, first load self into the razor alc, then create the service in the razor alc - This ensures we have full control over how dependencies are resolved.

chsienki · 2025-01-16T00:57:25Z

@dotnet/razor-compiler and @dotnet/razor-tooling for reviews please :)

docs/AssemblyLoadingStrategy.md

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/RazorAssemblyLoadContext.cs

333fred · 2025-01-16T01:14:17Z

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/RazorAssemblyLoadContext.cs

+                    else
+                    {
+                        // Roslyn won the race, we need to find the compiler assembly it loaded.
+                        while (true)


You mentioned in the doc that we'd need to error eventually in case Roslyn errored. When does that happen?

Thats a good point. When writing the doc, I figured we'd need to do that, but at implementation I realized it's impossible to do so deterministically as-is, as its essentially a halting problem: we don't know if its not-loaded or failed to load and so another Yield might actually succeed. We could stick some arbitrary amount of retries in, but while unlikely, we could still prematurely assume failure if we hit the retry limit because the other thread just didn't get scheduled. We would need to add some other synchronization primitive to roslyn that allows you to query the status of a given assembly load.

The only way we could get into this state where it really has failed to load is if the assembly is missing or corrupted. Roslyn and razor both load the same assembly from disk, so even if we could detect that the load failed in Roslyn, it's just going to fail again in Razor anyway.

Given that its an error case either way, I'm inclined to just update the doc to note this and leave it as-is. If we think that's not acceptable let's file a bug and not block this work on it as its not trivial to do.

davidwengier

I don't think we need Remote.Razor to target netstandard any more, except possibly for tests. But also don't think its anything for you to worry about in this PR :)

Co-authored-by: Fred Silberberg <[email protected]>

jaredpar · 2025-01-16T20:04:04Z

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/RazorAssemblyLoadContext.cs

+
+    private object _loaderLock = new();
+
+    public static readonly RazorAssemblyLoadContext Instance = new();


Should the static one be collectible? If it is collected that basically invalidates the static

Yeah, it probably shouldn't be

jaredpar · 2025-01-16T21:04:49Z

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/RazorAssemblyLoadContext.cs

+    public RazorAssemblyLoadContext()
+        : base(isCollectible: true)
+    {
+        var thisAssembly = GetType().Assembly;


Suggested change

var thisAssembly = GetType().Assembly;

var thisAssembly = typeof(RazorAssemblyLoadContext).Assembly;

jaredpar · 2025-01-16T21:05:30Z

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/RazorAssemblyLoadContext.cs

+    {
+        var thisAssembly = GetType().Assembly;
+        _parent = GetLoadContext(thisAssembly);
+        _baseDirectory = Path.GetDirectoryName(thisAssembly.Location) ?? "";


Think we should throw here on null vs. using "". If this is null then something is very wrong.

jaredpar · 2025-01-16T21:09:20Z

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/RazorAssemblyLoadContext.cs

+                        // Roslyn won the race, we need to find the compiler assembly it loaded.
+                        while (true)
+                        {
+                            foreach (var alc in AssemblyLoadContext.All)


Consider what happens if another AssemblyLoadContext decides to load razor. This strategy of searching all of the ALC could lead us to picking that instance vs. the one that Roslyn loaded. Why not just search the Roslyn ALC directly and grab the Razor DLL from there

var roslynAlc = AssemblyLoadContext.GetLoadContext(typeof(Microsoft.CodeAnalysis.Compilation).Assembly);

Hmm, but it's not loaded into the Roslyn ALC. It's loaded into a shadow copy ALC that's created for the specific directory. Ideally we would check that the path matches our base directory to know its the same assembly but we can't really do that because the shadow copy causes the path to move...

Hmm. What if we load the compiler into a metadata only ALC, then we can read the MVID. Then we can check the MVID against the rest of the ALCs to ensure we're picking the correct one?

Ah that's true ... but this should still wokr. Rather than iterating through the loaded assemblies just do the following:

var roslynAlc = AssemblyLoadContext.GetLoadContext(typeof(Microsoft.CodeAnalysis.Compilation).Assembly); var razorCompilerAssembly = roslynAlc.LoadFromAssemblyName(assemblyName);

This will go through all of the shadow copy madness in the roslyn ALC and return the assembly if it's available. If not then can proceed to insert the resolver.

Oh, that's a neat trick I hadn't thought about. That's a really nice simplification.

jaredpar · 2025-01-16T21:22:03Z

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/RazorAssemblyLoadContext.cs

+                                }
+                            }
+                            // we didn't find it, so it's possible that the Roslyn loader is still in the process of loading it. Yield and try again.
+                            Thread.Yield();


The other advantage of getting the specific roslyn ALC is I believe we can avoid this yield. Essentially, could we turn this method into effectively the following:

if (IsRazorCompiler(assemblyName)) { var roslynAlc = AssemblyLoadContext.GetLoadContext(typeof(Microsoft.CodeAnalysis.Compilation).Assembly); var roslynRazorCompiler = alc.Assemblies.SingleOrDefault(a => IsRazorCompiler(a.GetName())); if (roslynRazorCompiler is not null) { return roslynRazorCompiler; } // Either we set this or other thread did. Either way, we own the resolver after this. _ = RazorAnalyzerAssemblyResolver.TrySetAssemblyResolver(ResolveAssembly, assemblyName); return roslynAlc.LoadFromAssemblyName(assemblyName);

chsienki · 2025-01-17T18:53:36Z

Moving this to draft while we try a different approach.

chsienki added 4 commits January 14, 2025 14:53

Add doc explaining load strategy

9cfe6dc

Multi target ms.ca.remote.razor

12e651b

Add RazorAssemblyLoadContext

f78c9f9

- Add a new ALC that is 'razor compiler' aware - Atomically load the razor compiler or use a copy already loaded by roslyn

Update factory base to use RazorAlc

5fc0a71

- When creating a new service instance, first load self into the razor alc, then create the service in the razor alc - This ensures we have full control over how dependencies are resolved.

chsienki requested a review from a team as a code owner January 16, 2025 00:56

333fred reviewed Jan 16, 2025

View reviewed changes

davidwengier approved these changes Jan 16, 2025

View reviewed changes

Apply suggestions from code review

e1190b1

Co-authored-by: Fred Silberberg <[email protected]>

jaredpar reviewed Jan 16, 2025

View reviewed changes

chsienki marked this pull request as draft January 17, 2025 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize razor compiler assembly loading #11394

Synchronize razor compiler assembly loading #11394

chsienki commented Jan 16, 2025

chsienki commented Jan 16, 2025

333fred Jan 16, 2025

chsienki Jan 16, 2025

davidwengier left a comment

jaredpar Jan 16, 2025

chsienki Jan 16, 2025

jaredpar Jan 16, 2025

jaredpar Jan 16, 2025

jaredpar Jan 16, 2025

chsienki Jan 16, 2025

chsienki Jan 16, 2025

jaredpar Jan 16, 2025

chsienki Jan 16, 2025

jaredpar Jan 16, 2025

chsienki commented Jan 17, 2025


		private object _loaderLock = new();

		public static readonly RazorAssemblyLoadContext Instance = new();

	var thisAssembly = GetType().Assembly;
	var thisAssembly = typeof(RazorAssemblyLoadContext).Assembly;

Synchronize razor compiler assembly loading #11394

Are you sure you want to change the base?

Synchronize razor compiler assembly loading #11394

Conversation

chsienki commented Jan 16, 2025

chsienki commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier 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

chsienki commented Jan 17, 2025