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.

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

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Oct 9, 2024

Fixes microsoft/vscode-dotnettools#1470

In cases where the workspace does not contain a razor or cshtml file startup is delayed for razor.
This helps make sure we don't initialize when not needed. That also means that if a user opens a solution that contains a reference
to a project outside of the workspace then startup will be delayed. This is generally fine because it will be
initialized when needed, but there was an issue where the named pipe setup was trying to wait the startup task
that had not been created yet. An await on undefined immediately returns and the project system will never get
information about the project through the named pipe.

In cases where the workspace does not contain a razor or cshtml file startup is delayed for razor.
This helps make sure we don't initialize when not needed. That also means that if a user opens a solution that contains a reference
to a project outside of the workspace then startup will be delayed. This is generally fine because it will be
initialized when needed, but there was an issue where the named pipe setup was trying to wait the startup task
that had not been created yet. An await on undefined immediately returns and the project system will never get
information about the project through the named pipe.
@ryzngard ryzngard requested a review from a team as a code owner October 9, 2024 00:07
@@ -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:

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

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.

@ryzngard ryzngard merged commit 7b6600a into dotnet:main Oct 9, 2024
16 checks passed
@ryzngard ryzngard deleted the issues/1470 branch October 9, 2024 01:59
ryzngard added a commit to ryzngard/vscode-csharp that referenced this pull request Oct 9, 2024
In cases where the workspace does not contain a razor or cshtml file startup is delayed for razor.
This helps make sure we don't initialize when not needed. That also means that if a user opens a solution that contains a reference
to a project outside of the workspace then startup will be delayed. This is generally fine because it will be
initialized when needed, but there was an issue where the named pipe setup was trying to wait the startup task
that had not been created yet. An await on undefined immediately returns and the project system will never get
information about the project through the named pipe.
dibarbet added a commit that referenced this pull request Oct 10, 2024
[Cherry-Pick] Make sure named pipe awaits server start (#7645)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Project system ran into an unexpected problem
2 participants