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

Delay server start if nothing opened to avoid blocking devkit #8026

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Delay server start if nothing opened to avoid blocking devkit #8026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89bf5c6
Delay server start if nothing opened to avoid blocking devkit
dibarbet Feb 25, 2025
57e153b
Merge branch 'main' into delay_server
JoeRobich Feb 27, 2025
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
114 changes: 87 additions & 27 deletions src/activateRoslyn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,24 +39,6 @@ export function activateRoslyn(
const roslynLanguageServerEvents = new RoslynLanguageServerEvents();
context.subscriptions.push(roslynLanguageServerEvents);

// Activate Razor. Needs to be activated before Roslyn so commands are registered in the correct order.
// Otherwise, if Roslyn starts up first, they could execute commands that don't yet exist on Razor's end.
//
// Flow:
// Razor starts up and registers dynamic file info commands ->
// Roslyn starts up and registers Razor-specific didOpen/didClose/didChange commands and sends request to Razor
// for dynamic file info once project system is ready ->
// Razor sends didOpen commands to Roslyn for generated docs and responds to request with dynamic file info
const razorLanguageServerStartedPromise = activateRazorExtension(
context,
context.extension.extensionPath,
eventStream,
reporter,
csharpDevkitExtension,
platformInfo,
/* useOmnisharpServer */ false
);

// Setup a listener for project initialization complete before we start the server.
const projectInitializationCompletePromise = new Promise<void>((resolve, _) => {
roslynLanguageServerEvents.onServerStateChange(async (e) => {
Expand All @@ -66,25 +48,22 @@ export function activateRoslyn(
});
});

// Start the server, but do not await the completion to avoid blocking activation.
const roslynLanguageServerStartedPromise = activateRoslynLanguageServer(
// Start the language server but do not wait for it to avoid blocking the extension activation.
const roslynLanguageServerStartedPromise = getLanguageServerPromise(
context,
platformInfo,
optionStream,
eventStream,
csharpChannel,
reporter,
roslynLanguageServerEvents
csharpDevkitExtension,
roslynLanguageServerEvents,
getCoreClrDebugPromise
);

debugSessionTracker.initializeDebugSessionHandlers(context);
tryGetCSharpDevKitExtensionExports(csharpDevkitExtension, csharpChannel);
const coreClrDebugPromise = getCoreClrDebugPromise(roslynLanguageServerStartedPromise);

const languageServerExport = new RoslynLanguageServerExport(roslynLanguageServerStartedPromise);
const exports: CSharpExtensionExports = {
initializationFinished: async () => {
await coreClrDebugPromise;
await razorLanguageServerStartedPromise;
await roslynLanguageServerStartedPromise;
await projectInitializationCompletePromise;
},
Expand All @@ -104,6 +83,87 @@ export function activateRoslyn(
return exports;
}

async function getLanguageServerPromise(
context: vscode.ExtensionContext,
platformInfo: PlatformInformation,
optionStream: Observable<void>,
eventStream: EventStream,
csharpChannel: vscode.LogOutputChannel,
reporter: TelemetryReporter,
csharpDevkitExtension: vscode.Extension<CSharpDevKitExports> | undefined,
roslynLanguageServerEvents: RoslynLanguageServerEvents,
getCoreClrDebugPromise: (languageServerStarted: Promise<any>) => Promise<void>
): Promise<RoslynLanguageServer> {
// It is possible we're getting asked to activate due to dev kit activating to create a new project.
// We do not want to slow down devkit activation by taking up time on the extension host main thread.
// So we can avoid starting the server until there is a workspace or file we can work with.
const waitForWorkspaceOrFilePromise = new Promise<void>((resolve, _) => {
// check if there is a workspace opened or a csharp file opened and resolve the promise.
// if neither are true, subscribe to vscode events to resolve the promise when a workspace or csharp file is opened.
if (
vscode.workspace.workspaceFolders ||
vscode.workspace.textDocuments.some((doc) => doc.languageId === 'csharp')
) {
resolve();
} else {
csharpChannel.info('Waiting for a workspace or C# file to be opened to activate the server.');
// Subscribe to VS Code events to resolve the promise when a workspace or C# file is opened.
const onDidOpenTextDocument = vscode.workspace.onDidOpenTextDocument((doc) => {
if (doc.languageId === 'csharp') {
resolve();
onDidOpenTextDocument.dispose();
}
});

const onDidChangeWorkspaceFolders = vscode.workspace.onDidChangeWorkspaceFolders((event) => {
if (event.added.length > 0) {
resolve();
onDidChangeWorkspaceFolders.dispose();
}
});

context.subscriptions.push(onDidOpenTextDocument, onDidChangeWorkspaceFolders);
}
});

// Delay startup of language servers until there is something we can actually work with.
await waitForWorkspaceOrFilePromise;

// Activate Razor. Needs to be activated before Roslyn so commands are registered in the correct order.
// Otherwise, if Roslyn starts up first, they could execute commands that don't yet exist on Razor's end.
//
// Flow:
// Razor starts up and registers dynamic file info commands ->
// Roslyn starts up and registers Razor-specific didOpen/didClose/didChange commands and sends request to Razor
// for dynamic file info once project system is ready ->
// Razor sends didOpen commands to Roslyn for generated docs and responds to request with dynamic file info
const razorLanguageServerStartedPromise = activateRazorExtension(
context,
context.extension.extensionPath,
eventStream,
reporter,
csharpDevkitExtension,
platformInfo,
/* useOmnisharpServer */ false
);

const roslynLanguageServerStartedPromise = activateRoslynLanguageServer(
context,
platformInfo,
optionStream,
csharpChannel,
reporter,
roslynLanguageServerEvents
);

const coreClrDebugPromise = getCoreClrDebugPromise(roslynLanguageServerStartedPromise);
debugSessionTracker.initializeDebugSessionHandlers(context);
tryGetCSharpDevKitExtensionExports(csharpDevkitExtension, csharpChannel);

await Promise.all([razorLanguageServerStartedPromise, roslynLanguageServerStartedPromise, coreClrDebugPromise]);
return roslynLanguageServerStartedPromise;
}

/**
* This method will try to get the CSharpDevKitExports through a thenable promise,
* awaiting `activate` will cause this extension's activation to hang.
Expand Down
2 changes: 2 additions & 0 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ export async function activate(
const installDependencies: IInstallDependencies = async (dependencies: AbsolutePathPackage[]) =>
downloadAndInstallPackages(dependencies, networkSettingsProvider, eventStream, isValidDownload);

// Fine to do this as part of activate as it is always a no-op in Roslyn mode (the debugger and Razor are shipped in-box)
// Only actually downloads files in O# mode or local development.
const runtimeDependenciesExist = await installRuntimeDependencies(
Copy link
Member Author

Choose a reason for hiding this comment

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

debugger and razor are shipped in box now. I suspect I can actually have this only run in O# mode, but I'll need to make sure debugger/razor are installed in the specific local development scripts. for a followup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Since I believe we still need to upload a platform-neutral .vsix, but nearly no one should be using it, could we use conditional compilation so that the platform neutral one still used this code, but the platform-specific one omitted the check except in O# mode.

Copy link
Member Author

@dibarbet dibarbet Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know of a way to do conditional compilation in TS easily (but I'm no TS expert). The only issue I found about was closed as not planned - microsoft/TypeScript#3538

You're right that the neutral build here would also be a problem, I had forgotten about that one. I don't think we actually even upload a platform neutral build to the marketplace anymore - perhaps we should consider not even building it.

Copy link
Contributor

Choose a reason for hiding this comment

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

One option would be to have a isInPlatformSpecificVSIX function with two implementations, and we change the compiler options to include the one we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe easier: we could add something to package.json

Copy link
Member Author

@dibarbet dibarbet Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Or maybe easier: we could add something to package.json

Didn't quite get this - you mean add some kind of mapping of platform -> packages to download in the package.json?

As an aside - was planning on tackling this particular issue in a followup anyway (install no-ops in the cases we care about already), wanted to check if you had any other issues with the idea and approach in this PR from the debugger side. I know there are activation events for debug events, but I do not know if they are applicable w/out a workspace or file @gregg-miskelly

Copy link
Contributor

@gregg-miskelly gregg-miskelly Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

wanted to check if you had any other issues with the idea and approach in this PR from the debugger side.

I don't have any problems with merging from the debugger side. I think the only possible issue from the debugger side would be if something would now fail, such as attempts to launch a project, because the language service is no longer running. But, as far as I know, we already have those bugs today because we already don't block waiting for activation. CC @WardenGnaw in case there is something I am not thinking of.

Didn't quite get this - you mean add some kind of mapping of platform -> packages to download in the package.json?

My thought was we could add a new property to package.json that we would twiddle as part of the build, and since we have access to the package.json content as part of activation, we could check this. Example: if (!context.extension.packageJSON.platformSpecificVSIX || useOmnisharpServer)

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline, these changes will affect the vscode commands that are defined in package.json but not yet registered yet.

context.extension.packageJSON,
context.extension.extensionPath,
Expand Down