Razor synchronize loader #76357

chsienki · 2024-12-10T17:40:40Z

This updates the way the razor assembly loader installs its handler to ensure we can synchronize correctly between Razor and Roslyn. The approach is to track which assemblies have been loaded and only install the filter if the requested assembly has not yet been loaded.

Most of the work is on the Razor side, the Roslyn side is deliberately simple with nothing hard coded so we aren't baking in any Razor specific logic or dependencies. A WIP doc explaining the process from both sides is available at https://gist.github.com/chsienki/f3b3b19bf6cf6d1f8e047e59242db648.

Also adds a method to retrieve only the documents from a specific source generator. Again, we don't bake in the razor generator, so that we can change that without needing to change roslyn.

chsienki · 2024-12-10T20:08:10Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

-        /// in debug builds. 
-        /// </summary>
-        private static bool s_razorRequested = false;
+        private static readonly List<AssemblyName>? s_assembliesRequested = [];


All access to this is done under a lock, and there's only a single instance, so didn't seem worth using an immutable variant or pooling it.

Why is this marked as nullable? Are you trying to account for a static constructor race here?

Just a hangover from an earlier version pre-refactoring that I hadn't cleaned up. Fixed.

chsienki · 2024-12-10T20:09:44Z

src/Tools/ExternalAccess/Razor/RazorProjectExtensions.cs

+        {
+            var results = await project.GetSourceGeneratedDocumentsAsync(cancellationToken).ConfigureAwait(false);
+
+            var generatorIdentity = new SourceGeneratorIdentity(assemblyName, assemblyPath, assemblyVersion, typeName);


SourceGeneratorIdentity isn't public, so have to pass through the requisite parts. We could make an IVT copy but doesn't seem worth it.

jaredpar · 2024-12-11T00:45:11Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

-        /// in debug builds. 
-        /// </summary>
-        private static bool s_razorRequested = false;
+        private static readonly List<AssemblyName>? s_assembliesRequested = [];


Why is this marked as nullable? Are you trying to account for a static constructor race here?

jaredpar · 2024-12-11T00:46:05Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

-                Debug.Assert(s_assemblyResolver == null, "Assembly resolver should not be set multiple times.");
-                Debug.Assert(!s_razorRequested, "A razor assembly has already been requested before setting the resolver.");
-                s_assemblyResolver = value;
+                if (s_assemblyResolver is not null && s_assembliesRequested is not null && !s_assembliesRequested.Contains(canaryAssembly))


Not sure how s_assembliesRequested could be null here. Even in a static ctor race s_resolverLock would also be null so we'd throw before this part.

Shouldn't the check be s_assemblyResolver is null? The field starts off as null and this is the only place that it is written to so as stated this will never be set (I believe).

jaredpar · 2024-12-11T00:48:04Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

+            lock (s_resolverLock)
+            {
+                s_assembliesRequested?.Add(assemblyName);
+                return s_assemblyResolver?.Invoke(assemblyName);


This is doing a callback inside of a lock. That usually gives me a bit of pause. Is there a specific reason it needs to be inside the lock? Did you consider say saving the field to a local and then invoking that outside the lock?

Hmm, we need to retrieve the resolver under the lock, but once we've done that I can't think of any reason why we need to actually execute it under the lock.

There's no concern about the resolver being invoked on different threads without synchronization, right? Just a need for the s_assembliesRequested update and s_assemblyResolver read to be synchronized?

Thinking about it, we don't even need to retrieve it under the lock. Before we enter the lock we either 1) have the resolver set or 2) we don't:

In the case of 1) the locking doesn't actually have any effect.

In the case of 2) if the assembly being requested is the canary assembly, then once we exit the lock the resolver can never be set, so it's also safe to just use it.

In the case of 2) where it's not the canary assembly being requested, we actually have a race condition. We exit the lock having added the load to the list, but the state of the resolver can still change. If the install call comes in in the time between exiting the lock and executing the resolver we'll use the resolver, if it doesn't we wont. Now, for our code this actually doesn't matter, really, we only want to execute the resolver for the canary assembly anyway (but we can't do that because we don't know ahead of time what it is).

We can make the last part deterministic by retrieving the loader under the lock and executing against the local value, but don't need to for it to be correct for our usage. I'm leaning towards making it deterministic just for ease-of-understanding the code though.

Ended up just making this deterministic in all cases so its easier to reason about.

RikkiGibson · 2024-12-11T01:02:09Z

Could you please link to related issues/PRs so that I can get the full context on the scenario this PR is addressing? Thanks!

chsienki · 2024-12-11T01:06:41Z

@RikkiGibson There isn't really a good issue/PR to address this, but I linked a doc in the description which should give you the context you need.

RikkiGibson · 2024-12-11T01:06:58Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

-                Debug.Assert(s_assemblyResolver == null, "Assembly resolver should not be set multiple times.");
-                Debug.Assert(!s_razorRequested, "A razor assembly has already been requested before setting the resolver.");
-                s_assemblyResolver = value;
+                if (s_assemblyResolver is not null && s_assembliesRequested is not null && !s_assembliesRequested.Contains(canaryAssembly))


I am confused as to how the s_assemblyResolver can ever become non-null, if entering the block that assigns it requires it to already be non-null.

Just a hangover from an earlier version pre-refactoring that I hadn't cleaned up. Fixed.

RikkiGibson · 2024-12-11T01:08:36Z

@RikkiGibson There isn't really a good issue/PR to address this, but I linked a doc in the description which should give you the context you need.

I took a look and things are mostly making sense. Just trying to get a full sense of the impact here. It sounds like we already had some logic to coordinate this generator load across roslyn and razor, it's just that there were reliability issues due to lack of synchronization. Is that right?

chsienki · 2024-12-11T01:34:51Z

@RikkiGibson

it's just that there were reliability issues due to lack of synchronization. Is that right?

We were previously assuming the Razor tooling would come up before Roslyn loaded the source generator, but it's actually a non-fixable race between which set of tooling gets loaded first, due to the way the service hub process comes up (and a bunch of other stuff like what files a user has open). It's completely legal for either razor or Roslyn to start first, so we have to have this synchronization to ensure we only load a single copy.

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

Co-authored-by: Jared Parsons <[email protected]>

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

Co-authored-by: Jan Jones <[email protected]>

RikkiGibson · 2024-12-12T19:12:51Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

            }
        }

        public Assembly? ResolveAssembly(AssemblyName assemblyName)
        {
-            s_razorRequested |= assemblyName.FullName.Contains("Razor");


Are there any potential problems with razor being out of sync with this EA? i.e. razor is older than roslyn, and razor expects the old assembly hook pattern to be used, or roslyn is older than razor, and razor expects the new hook pattern.

Maybe we just have to take care to dual insert and we'll be ok?

DustinCampbell · 2024-12-12T19:54:29Z

src/Tools/ExternalAccess/Razor/RazorProjectExtensions.cs

+
+            var generatorIdentity = new SourceGeneratorIdentity(assemblyName, assemblyPath, assemblyVersion, typeName);
+            return results.Where(s => s.Identity.Generator == generatorIdentity);
+


nit: extra blank line

DustinCampbell · 2024-12-12T19:58:37Z

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

-        /// in debug builds. 
-        /// </summary>
-        private static bool s_razorRequested = false;
+        private static readonly List<AssemblyName> s_assembliesRequested = [];


Since all of the code here just adds items to this collection and checks to see if items are present, should this be a HashSet<AssemblyName> rather than a List<AssemblyName>?

Good idea, thanks.

…i/roslyn into razor_synchronize_loader

chsienki added 2 commits December 10, 2024 09:39

Add syncronization to razor assembly loader

d198055

Add a method to get generator specific documents

e387c60

dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 10, 2024

chsienki commented Dec 10, 2024

View reviewed changes

chsienki marked this pull request as ready for review December 10, 2024 20:12

chsienki requested a review from a team as a code owner December 10, 2024 20:12

chsienki removed the Area-Infrastructure label Dec 10, 2024

jaredpar assigned jjonescz and RikkiGibson Dec 11, 2024

jaredpar added Area-Compilers and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 11, 2024

jaredpar reviewed Dec 11, 2024

View reviewed changes

RikkiGibson reviewed Dec 11, 2024

View reviewed changes

chsienki added 2 commits December 10, 2024 17:08

Cleanup nullability

c544a78

Invoke resolver after lock

bb90572

Capture the state of the resolver under the lock for determinism

24c3a1f

jaredpar reviewed Dec 11, 2024

View reviewed changes

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs Outdated Show resolved Hide resolved

Update src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

28191e4

Co-authored-by: Jared Parsons <[email protected]>

jjonescz reviewed Dec 11, 2024

View reviewed changes

src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs Outdated Show resolved Hide resolved

jjonescz approved these changes Dec 11, 2024

View reviewed changes

Update src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs

ad89968

Co-authored-by: Jan Jones <[email protected]>

build-analysis bot mentioned this pull request Dec 11, 2024

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

Open

3 tasks

RikkiGibson reviewed Dec 12, 2024

View reviewed changes

RikkiGibson approved these changes Dec 12, 2024

View reviewed changes

DustinCampbell approved these changes Dec 12, 2024

View reviewed changes

chsienki added 2 commits December 12, 2024 14:05

PR Feedback

481921f

Merge branch 'razor_synchronize_loader' of https://github.com/chsienk…

3de8e9e

…i/roslyn into razor_synchronize_loader

chsienki merged commit 9f69940 into dotnet:main Dec 12, 2024
25 checks passed

dotnet-policy-service bot added this to the Next milestone Dec 12, 2024

This was referenced Dec 14, 2024

[Automated] PRs inserted in VS build main-35613.82 #76426

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35617.104 #76481

Closed

dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Razor synchronize loader #76357

Razor synchronize loader #76357

chsienki commented Dec 10, 2024 •

edited

Loading

chsienki Dec 10, 2024 •

edited

Loading

jaredpar Dec 11, 2024

chsienki Dec 11, 2024

chsienki Dec 10, 2024

jaredpar Dec 11, 2024

jaredpar Dec 11, 2024

jaredpar Dec 11, 2024

chsienki Dec 11, 2024

RikkiGibson Dec 11, 2024

chsienki Dec 11, 2024

chsienki Dec 11, 2024

RikkiGibson commented Dec 11, 2024

chsienki commented Dec 11, 2024

RikkiGibson Dec 11, 2024

chsienki Dec 11, 2024

RikkiGibson commented Dec 11, 2024

chsienki commented Dec 11, 2024

RikkiGibson Dec 12, 2024

DustinCampbell Dec 12, 2024

DustinCampbell Dec 12, 2024

chsienki Dec 12, 2024


		var generatorIdentity = new SourceGeneratorIdentity(assemblyName, assemblyPath, assemblyVersion, typeName);
		return results.Where(s => s.Identity.Generator == generatorIdentity);

Razor synchronize loader #76357

Razor synchronize loader #76357

Conversation

chsienki commented Dec 10, 2024 • edited Loading

chsienki Dec 10, 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

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

RikkiGibson commented Dec 11, 2024

chsienki commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Dec 11, 2024

chsienki commented Dec 11, 2024

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 Dec 10, 2024 •

edited

Loading

chsienki Dec 10, 2024 •

edited

Loading