Move RazorWorkspaceListener to AsyncBatchingWorkQueue #10480

ryzngard · 2024-06-12T21:11:58Z

In Linux + VS Code when creating a new project we're seeing work get dropped from the queue that would result in up to date information being written to disk. Looking at the logs + the bin file it's noticeable that the correct project information isn't there, and that results in Razor behaving poorly by not having the correct files in the correct projects, and often times lacking the accurate amount of taghelpers.

Switching to AsyncBatchingWorkQueue seems to improve reliability here. I also added more trace logging to help identify when information is being written, when files are moving, etc. A try/catch was added to make sure we log in case of an exception as well.

There are still issues on Linux, but this solves part of them.

DustinCampbell

Putting everything in a batch is probably much better for reliability than what we were doing before. There are still going to be disk contention problems though -- it won't cure them completely. In my mind, the solutions for that would be one of:

Process synchronization when writing reading from disk via a named mutex shared between the process this is loaded in and rzls. However, we should ensure that there's one mutex per Roslyn LSP/Razor LSP process pair to avoid sharing the same mutex between different VS Code instances.
Avoid the disk altogether by serializing to a named MemoryMapFile and somehow passing that name to the Razor LSP, which would delete it after deserializing.
Wait for co-hosting. 😄

src/Razor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/PublicAPI.Unshipped.txt

...azor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListener.cs

DustinCampbell · 2024-06-12T21:52:46Z

...azor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListener.cs

                break;
            default:
                break;
        }
    }

-    private void RemoveProject(Project? project)


It's a little worrisome that we drop project removes on the floor. We definitely watch for deletions of the project.razor.bin file in the language server. If a project is deleted from disk, do we leave the language server in a descent state? Or, does that project continue to live there with all of its documents and references, responding to Rename, Component search, completion, etc.?

This never handled removing the file, it just stopped the queue. I'm not actually sure what handles cleanup. Since we're working on _workspace.CurrentSolution I'm guessing it would be safe to just delete the file when a project is removed?

That's new, and maybe it's already done somewhere else? Not sure

One thought:

We could enqueue and update for the removed ProjectId and in the ABWQ work method we could clean up the file if it exists and the project is no longer in the solution?

This was probably unnecessary originally, but yeah I was just trying to remove the scheduler. We'd never get any more updates for the project anyway, so could have ignored removes then too.

Not sure I like the idea of removing the file though. I think in VS Code we are more likely to be processing requests before the file is written, so it does serve more like an offline cache, even if it is one that won't be being updated any more.

Yeah, I agree that this is probably OK. I saw all of the red and notice that all it did was get rid of the scheduler before. With #10475 I think VS Code will start reading in .bin even sooner, so you're right that VS Code will use it more as a cache.

Test failures lead to one thing remove did that I didn't notice: we only queue work for items that have been reported with NotifyDynamicFile. This PR breaks that. I'm adding remove back to fix

...azor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListener.cs

ryzngard · 2024-06-12T22:44:06Z

Putting everything in a batch is probably much better for reliability than what we were doing before. There are still going to be disk contention problems though

Yea. Disk contention is attempted to be reduced by writing to a temp file and then moving it to the final file. This is done in

razor/src/Razor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorProjectInfoSerializer.cs

Lines 144 to 150 in d8690ff

    
           var fileInfo = new FileInfo(configurationFilePath); 
        
           if (fileInfo.Exists) 
        
           { 
        
               fileInfo.Delete(); 
        
           } 
        
           File.Move(tempFileInfo.FullName, configurationFilePath);

ryzngard · 2024-06-12T23:40:13Z

...azor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListener.cs

    }

-    public void Dispose()
+    // Protected for testing
+#pragma warning disable RS0016 // Add public types and members to the declared API


Suppressing until I can figure out if it's user error or analyzer error.

Seems like neither. A protected method of a public type is part of the public API. I'd either remove the suppression, or create a TestAccessor so it doesn't need to be protected.

Is this a case where private protected is needed?

Yes! That's exactly what I was thinking. I just got my newfangled things mixed up

davidwengier · 2024-06-13T01:06:48Z

...azor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListener.cs

                break;
            default:
                break;
        }
    }

-    private void RemoveProject(Project? project)


This was probably unnecessary originally, but yeah I was just trying to remove the scheduler. We'd never get any more updates for the project anyway, so could have ignored removes then too.

Not sure I like the idea of removing the file though. I think in VS Code we are more likely to be processing requests before the file is written, so it does serve more like an offline cache, even if it is one that won't be being updated any more.

davidwengier · 2024-06-13T01:09:01Z

...azor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListener.cs

    }

-    public void Dispose()
+    // Protected for testing
+#pragma warning disable RS0016 // Add public types and members to the declared API


Seems like neither. A protected method of a public type is part of the public API. I'd either remove the suppression, or create a TestAccessor so it doesn't need to be protected.

DustinCampbell

With question about removes answered, this looks good to me!

Two major changes being introduced: dotnet/razor#10480 moves our project change listener to AsyncBatchingWorkQueue to improve stability dotnet/razor#10475 rearchitects our project syncing. Expected to have no impact on VS Code

ryzngard added 5 commits June 11, 2024 16:34

Add more logging

d8b5386

Add try/catch

b2d5b72

Move AsyncBatchingWorkQueue

6020a22

Switch RazorWorkspaceListener to AsyncBatchingWorkQueue

29d85e6

Pass in solution and add more logging

521800d

ryzngard requested a review from a team as a code owner June 12, 2024 21:11

ryzngard marked this pull request as draft June 12, 2024 21:13

Remove removal

8e1a06b

ryzngard marked this pull request as ready for review June 12, 2024 21:23

DustinCampbell reviewed Jun 12, 2024

View reviewed changes

ryzngard added 3 commits June 12, 2024 15:46

PR feedback and remove unused class

3c230aa

Remove distinct

7ae4edb

Suppress for now

39306ce

ryzngard commented Jun 12, 2024

View reviewed changes

davidwengier approved these changes Jun 13, 2024

View reviewed changes

DustinCampbell approved these changes Jun 13, 2024

View reviewed changes

Correct access modifier

7a4acf2

ryzngard enabled auto-merge (squash) June 13, 2024 20:25

ryzngard added 2 commits June 13, 2024 14:57

Fix tests and only do work for projects with dynamic file

d27e778

Merge branch 'main' into projectinfo_logging

8e43a1f

ryzngard merged commit 5457702 into dotnet:main Jun 14, 2024
12 checks passed

dotnet-policy-service bot added this to the Next milestone Jun 14, 2024

ryzngard deleted the projectinfo_logging branch June 14, 2024 05:06

ryzngard mentioned this pull request Jun 17, 2024

Bump razor to include fix for project serialization dotnet/roslyn#74021

Merged

ryzngard mentioned this pull request Jun 17, 2024

[BUG] Blazor Components are seen a unknown microsoft/vscode-dotnettools#49

Closed

This was referenced Jun 19, 2024

[Automated] PRs inserted in VS build main-35018.08 #10501

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35020.110 #10517

Closed

jjonescz modified the milestones: Next, 17.11 Preview 3 Jun 24, 2024

dotnet-bot mentioned this pull request Jun 27, 2024

[Automated] PRs inserted in VS build feature.dotnetVS-35026.97 #10550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move RazorWorkspaceListener to AsyncBatchingWorkQueue #10480

Move RazorWorkspaceListener to AsyncBatchingWorkQueue #10480

ryzngard commented Jun 12, 2024

DustinCampbell left a comment

DustinCampbell Jun 12, 2024

ryzngard Jun 12, 2024

ryzngard Jun 12, 2024

davidwengier Jun 13, 2024

DustinCampbell Jun 13, 2024

ryzngard Jun 13, 2024

ryzngard commented Jun 12, 2024

ryzngard Jun 12, 2024

davidwengier Jun 13, 2024

DustinCampbell Jun 13, 2024

ryzngard Jun 13, 2024

davidwengier Jun 13, 2024

davidwengier Jun 13, 2024

DustinCampbell left a comment

Move RazorWorkspaceListener to AsyncBatchingWorkQueue #10480

Move RazorWorkspaceListener to AsyncBatchingWorkQueue #10480

Conversation

ryzngard commented Jun 12, 2024

DustinCampbell 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

ryzngard commented Jun 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment