Use a named pipe to communicate projectinfo in vscode #10521

ryzngard · 2024-06-22T00:10:00Z

This changes from a file watcher approach to using named pipes. The flow for notifications is now:

Set up a handler that gets the name to connect to for a named pipe. This is generated as a GUID to avoid squatting. The Roslyn side will get this information through the razor/initialize message.
Roslyn sets up the server, Razor sets up a client.
All notifications go through that stream. MessagePack serialization is used for ProjectInfo still to serialize to bytes.

This will require a dual insertion and VS Code changes.

based approaches. The pipe will send updates that are serialized by messagepack and then deserialize and enqueue project updates. This should help improve project updates. There's also numerous logging added to help diagnose where issues might be coming from if users are hitting them.

davidwengier

LGTM

...zor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorProjectInfoFactory.cs

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

jaredpar · 2024-06-24T15:01:11Z

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

+            return;
+        }
+
+        _logger.LogTrace("Opening named pipe server: {0}", pipeName);


Do all the processes write to a single log stream or does each one write to a different one? If different then strongly suggest you include the process id in any named pipe logging. Will save you hair pulling exercises in the future when trying to figure out which process is doing what 😄

In VS Code this goes to the vs code output window, so I think we'll be okay. We don't write to a file on disk :)

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

...rosoft.AspNetCore.Razor.LanguageServer/ProjectSystem/NamedPipeBasedRazorProjectInfoDriver.cs

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/StreamExtensions.cs

…tSystem/NamedPipeBasedRazorProjectInfoDriver.cs Co-authored-by: Jared Parsons <[email protected]>

…ream

…amed_pipe

...zor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorProjectInfoFactory.cs

...rosoft.AspNetCore.Razor.LanguageServer/ProjectSystem/NamedPipeBasedRazorProjectInfoDriver.cs

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/StreamExtensions.cs

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

jaredpar · 2024-06-25T21:41:46Z

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs

+        _stream = null;
+    }
+
+    public void NotifyDynamicFile(ProjectId projectId)


Can this method be called from multiple threads?

what all methods in this type can be called from multiple threads?

NotifyDynamicFile should be the only one. EnsureInitialized is not thread safe and not intended to be. Workspace_WorkspaceChanged is driven by workspace change events so ordered accordingly. UpdateCurrentProjectsAsync is driven by the work queue

EnsureInitialized is protected by a lock: https://github.com/dotnet/roslyn/blob/602687a61e2ec18a1a2b4e498779a4037809c7e3/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/RazorWorkspaceListenerInitializer.cs#L52

NotifyDynamicFile calls come from Roslyn's IDynamicFileInfoProvider, which guarantees non-overlapping calls I believe.

Mind if we talk offline for a bit? Might be quicker to share context and learn more about concerns

What che(ck|que) do you think you're writing here that might be a problem?

Just wanted to validate my statement that calls to IDynamicFileInfoProvider.GetDynamicFileInfoAsync won't overlap

You mean this one?

https://github.com/dotnet/roslyn/blob/9951cebdc4f5581a0a06c1355f9f8c033ba0cb70/src/Workspaces/Core/Portable/Workspace/Host/SourceFiles/IDynamicFileInfoProvider.cs#L19-L26

We absolutely will call that on multiple threads simultaneously.

Glad I checked!

AFAICT tell we should be okay still. This code hasn't changed in this PR, just moved. I can do another pass later to see if we can better clean this up, as well as better hook up the SolutionChanged event that wasn't handled correctly before

...rosoft.AspNetCore.Razor.LanguageServer/ProjectSystem/NamedPipeBasedRazorProjectInfoDriver.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectSystem/RazorConnectHandler.cs

…tSystem/RazorConnectHandler.cs Co-authored-by: Jared Parsons <[email protected]>

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs

jasonmalinowski

I skimmed some of the Roslyn workspace interactions and nothing terrible jumped out.

jasonmalinowski · 2024-06-26T16:52:13Z

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs

+        _stream = null;
+    }
+
+    public void NotifyDynamicFile(ProjectId projectId)


What che(ck|que) do you think you're writing here that might be a problem? Nothing jumps out as overly problematic.

jasonmalinowski · 2024-06-26T22:38:29Z

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs

+
+        // Schedule a task, in case adding a dynamic file is the last thing that happens
+        _logger.LogTrace("{projectId} scheduling task due to dynamic file", projectId);
+        _workQueue.AddWork(new UpdateWork(projectId));


At first I was a bit worried what would happen if the project was removed while still handling this; I guess we'll see the removal after this event handler, and as long as the work queue doesn't reorder the remove operation then you're good.

We use the _workspace.CurrentSolution and double check the project is still present. I believe that should suffice to make sure, and on the other end if we receive a remove that we never knew about we just ignore.

jasonmalinowski · 2024-06-26T22:40:21Z

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs

+    {
+        switch (e.Kind)
+        {
+            case WorkspaceChangeKind.SolutionChanged:


SolutionChanged could also theoretically have projects removed.

hmm... we weren't handling that before. I'm guessing that was wrong. Do we need to do a diff of the ProjectIds?

jasonmalinowski · 2024-06-26T22:54:59Z

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs

+            case WorkspaceChangeKind.AnalyzerConfigDocumentRemoved:
+            case WorkspaceChangeKind.AnalyzerConfigDocumentReloaded:
+            case WorkspaceChangeKind.AnalyzerConfigDocumentChanged:
+                var projectId = e.ProjectId ?? e.DocumentId?.ProjectId;


Was this just defensively written or are there cases where we might actually have a null ProjectId and a non-null DocumentId? I wouldn't think that was possible, but maybe?

Looks like I added it without much fanfare in #8607, so its possibly overly defensive, but this was written when we were having to be a bit secretive so if there was a real scenario, we didn't write it down anywhere.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectSystem/RazorConnectHandler.cs

src/Razor/src/rzls/RazorConnectHandler.cs

…fd;lasjdfkl;jsakl;jdf;lkasj

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/StreamExtensions.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Protocol/RazorConnectParams.cs

src/Razor/src/rzls/NamedPipeBasedRazorProjectInfoDriver.cs

davidwengier · 2024-06-27T11:32:48Z

src/Razor/src/rzls/NamedPipeBasedRazorProjectInfoDriver.cs

+
+    private async Task ReadFromStreamAsync(CancellationToken cancellationToken = default)
+    {
+        Logger?.LogTrace($"Starting read from named pipe.");


In CreateNamedPipeAsync Logger is not nullable, but here it could be? And also below this

interesting. I don't know if it's a bug or not, but if I remove all of the ?. this warning goes away

src/Razor/src/rzls/NamedPipeBasedRazorProjectInfoDriver.cs

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs

davidwengier

Happy for any of these comments to be in a follow up if its easier, they're all nits/style related really.

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/StreamExtensions.cs

.../src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/AbstractRazorProjectInfoDriver.cs

…Workspace/RazorWorkspaceListenerBase.cs Co-authored-by: David Wengier <[email protected]>

…amed_pipe

ryzngard added 4 commits June 21, 2024 15:53

Clean up and move some logic into extension methods

1dbd46d

Use pooled arrays

1718f43

Remove file based project info settings

cbe4b94

ryzngard requested review from a team as code owners June 22, 2024 00:10

ryzngard added 3 commits June 21, 2024 17:10

Woops

b872a10

Make asynchronous

16c6010

Fix tests and add serialization test

c5a5faa

davidwengier approved these changes Jun 24, 2024

View reviewed changes

...zor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorProjectInfoFactory.cs Outdated Show resolved Hide resolved

...azor/src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListener.cs Outdated Show resolved Hide resolved

jaredpar reviewed Jun 24, 2024

View reviewed changes

ryzngard and others added 6 commits June 24, 2024 12:35

Update src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Projec…

d641652

…tSystem/NamedPipeBasedRazorProjectInfoDriver.cs Co-authored-by: Jared Parsons <[email protected]>

PR feedback. Allocate less. Bump some logs to information. Dispose st…

c48aac9

…ream

Merge branch 'named_pipe' of https://github.com/ryzngard/razor into n…

c32f25f

…amed_pipe

Add ownership validation

98c99c6

Stackalloc

7c006a8

Stackalloc for writing size

4a445c2

ryzngard mentioned this pull request Jun 24, 2024

Consume new Razor EA dotnet/roslyn#74134

Merged

usings...

2985b2e

ryzngard mentioned this pull request Jun 24, 2024

Send named pipe name to roslyn and razor dotnet/vscode-csharp#7273

Merged

ryzngard added 2 commits June 24, 2024 16:47

Dispose named pipe

7cde45f

Clean up and move to abstract to make testing easier

3aee1bf

davidwengier approved these changes Jun 25, 2024

View reviewed changes

PR feedback and publicapi fixes

c8fe2dc

jaredpar reviewed Jun 25, 2024

View reviewed changes

Update src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Projec…

2a675c3

…tSystem/RazorConnectHandler.cs Co-authored-by: Jared Parsons <[email protected]>

jaredpar reviewed Jun 26, 2024

View reviewed changes

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed Jun 26, 2024

View reviewed changes

ryzngard added 2 commits June 26, 2024 16:31

Move to RZLS

6c4c79e

Remove because cut/paste doesn't in VS

6717529

ryzngard added 3 commits June 26, 2024 16:46

Be more clear about async work. Use local functions to help scope

537b019

Copilot forgot my using!

e915417

Clean up loop expectation

60f3661

ryzngard commented Jun 27, 2024

View reviewed changes

src/Razor/src/rzls/RazorConnectHandler.cs Outdated Show resolved Hide resolved

ryzngard added 4 commits June 26, 2024 17:27

Fix tests

1979f0e

Update PublicAPI.Unshipped.txt

05ab5ff

Productive discussion! Let's gooooo!

3fa3205

LKJDLKSJFLKSJDFLKJDFLJSLFKDJSLDKFJ:LKj:LjKL:DJl;jak;ljfsdkl;ajfa;lksj…

22c52d5

…fd;lasjdfkl;jsakl;jdf;lkasj

davidwengier reviewed Jun 27, 2024

View reviewed changes

Be more clear about disposal state

7103fb1

jaredpar approved these changes Jun 27, 2024

View reviewed changes

.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs Outdated Show resolved Hide resolved

ryzngard added 3 commits June 27, 2024 12:28

Use disposal token

d548c02

PR feedback

e6cc53a

Merge branch 'main' into named_pipe

36dc320

davidwengier approved these changes Jun 27, 2024

View reviewed changes

ryzngard and others added 3 commits June 27, 2024 14:29

Update src/Razor/src/Microsoft.AspNetCore.Razor.ExternalAccess.Roslyn…

81de399

…Workspace/RazorWorkspaceListenerBase.cs Co-authored-by: David Wengier <[email protected]>

Feedback

88a359c

Merge branch 'named_pipe' of https://github.com/ryzngard/razor into n…

1f9ae6d

…amed_pipe

ryzngard enabled auto-merge (squash) June 27, 2024 21:54

ryzngard merged commit 06ba187 into dotnet:main Jun 27, 2024
12 checks passed

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

ryzngard deleted the named_pipe branch June 27, 2024 22:44

This was referenced Jul 4, 2024

[Automated] PRs inserted in VS build main-35104.06 #10584

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35108.50 #10594

Closed

RikkiGibson modified the milestones: Next, 17.12 Preview 1 Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a named pipe to communicate projectinfo in vscode #10521

Use a named pipe to communicate projectinfo in vscode #10521

ryzngard commented Jun 22, 2024

davidwengier left a comment

jaredpar Jun 24, 2024

ryzngard Jun 24, 2024

jaredpar Jun 25, 2024

ryzngard Jun 25, 2024

jaredpar Jun 25, 2024

ryzngard Jun 25, 2024

davidwengier Jun 25, 2024

ryzngard Jun 26, 2024

davidwengier Jun 26, 2024

jasonmalinowski Jun 27, 2024 •

edited

Loading

davidwengier Jun 27, 2024

ryzngard Jun 27, 2024

jasonmalinowski left a comment

jasonmalinowski Jun 26, 2024

jasonmalinowski Jun 26, 2024

ryzngard Jun 26, 2024

jasonmalinowski Jun 26, 2024

ryzngard Jun 27, 2024

jasonmalinowski Jun 26, 2024

davidwengier Jun 26, 2024

davidwengier Jun 27, 2024 •

edited

Loading

ryzngard Jun 27, 2024

ryzngard Jun 27, 2024

davidwengier left a comment

Use a named pipe to communicate projectinfo in vscode #10521

Use a named pipe to communicate projectinfo in vscode #10521

Conversation

ryzngard commented Jun 22, 2024

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

jasonmalinowski Jun 27, 2024 • edited Loading

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

davidwengier Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

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

jasonmalinowski Jun 27, 2024 •

edited

Loading

davidwengier Jun 27, 2024 •

edited

Loading