Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure named pipe awaits server start #7645

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Make sure named pipe awaits server start #7645

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/razor/src/razorLanguageServerClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export class RazorLanguageServerClient implements vscode.Disposable {
}

public async connectNamedPipe(pipeName: string): Promise<void> {
await this.startHandle;
await this.start();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this. You said that previous it could wait on startHandle before it had been created, due to delayed initialization, which makes sense, but this makes it look like this will actually start things up instead. Doesn't that break the delayed initialization?

This change makes sense if we know that connectNamedPipe is only called after something else has started the Razor bits, but if that were true then I don't see how the old code would have been problematic.

Or does start() have a wait inside it that waits for a file to appear in the workspace? That would make sense, but if so, perhaps a comment here mentioning that, and why its okay to call start here, would be good to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding this happens anytime roslyn would ask for dynamic file information from razor:

vscode-csharp/src/razor/src/document/razorDocumentManager.ts

Line 173 in ec1d55d

public async ensureRazorInitialized() {

At that point we should be starting up right? Otherwise dynamic file info will never be provided. If that's not the case then I could add this to check if razor is started, add a callback for after it started

Copy link
Contributor

@davidwengier davidwengier Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see it now. There is a gap where a there could be no Razor files in the VS Code workspace, but they do exist in a Roslyn project. Odd, but fair enough. I had a quick look at it seems fine if this happens to be where the language server starts. Sorry, I didn't pay enough attention to where you mentioned this in the PR description.


// Params must match https://github.com/dotnet/razor/blob/92005deac54f3e9d1a4d1d8f04389379cccfa354/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Protocol/RazorNamedPipeConnectParams.cs#L9
await this.sendNotification('razor/namedPipeConnect', { pipeName: pipeName });
Expand Down
Loading