Ensure the language client is always ready before using it - #14617
Ensure the language client is always ready before using it#14617Bob Brown (bobbrow) wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a TypeScript wrapper around the VS Code LanguageClient to ensure RPC calls into cpptools are gated on a single “ready” signal, aiming to prevent crashes caused by early event/RPC usage before the language client is usable.
Changes:
- Added a
LanguageClientwrapper that awaits readiness forsendRequest/sendNotification, and updated the core client startup flow to use it. - Removed many explicit
await client.readycall sites across providers/commands, relying on the wrapper to enforce readiness. - Removed the
enqueue-based sequencing from client/extension event wiring and performed minor provider cleanup.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Extension/src/LanguageServer/Providers/workspaceSymbolProvider.ts | Constructor simplification; relies on centralized readiness behavior. |
| Extension/src/LanguageServer/Providers/semanticTokensProvider.ts | Formatting-only style adjustment. |
| Extension/src/LanguageServer/Providers/renameProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/onTypeFormattingEditProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/HoverProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/foldingRangeProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/findAllReferencesProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/documentSymbolProvider.ts | Switches to using client.languageClient.sendRequest without manual readiness await. |
| Extension/src/LanguageServer/Providers/documentRangeFormattingEditProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/documentFormattingEditProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/CopilotHoverProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/codeActionProvider.ts | Removes explicit readiness await in favor of wrapper. |
| Extension/src/LanguageServer/Providers/callHierarchyProvider.ts | Removes explicit readiness awaits in favor of wrapper. |
| Extension/src/LanguageServer/protocolFilter.ts | Stops manually awaiting client.ready before sending visible-editor updates. |
| Extension/src/LanguageServer/languageClient.ts | New wrapper class that gates requests/notifications on a “ready” signal. |
| Extension/src/LanguageServer/extension.ts | Removes enqueue usage from editor event hooks; removes explicit ready awaits for some commands. |
| Extension/src/LanguageServer/codeAnalysis.ts | Swaps to wrapper LanguageClient and adjusts languageclient imports accordingly. |
| Extension/src/LanguageServer/clientCollection.ts | Removes await newClient.ready during crash-recovery ownership transfer. |
| Extension/src/LanguageServer/client.ts | Central refactor to use wrapper, remove enqueue, and shift readiness responsibility to the wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public async restartIntelliSenseForFile(document: vscode.TextDocument): Promise<void> { | ||
| await this.ready; | ||
| return this.languageClient.sendNotification(RestartIntelliSenseForFileNotification, this.languageClient.code2ProtocolConverter.asTextDocumentIdentifier(document)).catch(logAndReturn.undefined); |
There was a problem hiding this comment.
ready does not guard against the case you describe. In the unlikely event that a rescan is requested before the language client is ready, the exception should be caught and this will be a no-op. The language server would then proceed to initialize and parse the file for IntelliSense and it will appear as though the rescan happened.
There was a problem hiding this comment.
✨Copilot (agent8): This is still reachable on the current head. JavaScript evaluates this.languageClient.code2ProtocolConverter.asTextDocumentIdentifier(...) before sendNotification() is called, so the wrapper's await this.ready is never reached and the method-level .catch(...) is never attached. Because the containing function is async, the command returns a rejected Promise rather than crashing synchronously, but VS Code surfaces that as a command failure rather than a no-op. Restoring await this.ready before constructing the identifier would address it.
|
With these changes, I'm seeing an unusual number of |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
Extension/src/LanguageServer/client.ts:3116
restartIntelliSenseForFilereadsthis.languageClient.code2ProtocolConverterbefore any readiness await. If the language client wrapper hasn’t been initialized yet, that getter can throw synchronously during argument evaluation, bypassing the.catch(...)and crashing the event path. Await readiness before accessing the converters so the call is reliably deferred until the RPC client exists.
public async restartIntelliSenseForFile(document: vscode.TextDocument): Promise<void> {
return this.languageClient.sendNotification(RestartIntelliSenseForFileNotification, this.languageClient.code2ProtocolConverter.asTextDocumentIdentifier(document)).catch(logAndReturn.undefined);
Extension/src/LanguageServer/protocolFilter.ts:49
cppEditorsis captured before the language client is ready, butonDidChangeVisibleTextEditorsbuildsparamssynchronously and only awaits readiness insidesendNotification. If initialization is still in progress, the visible editor/selection snapshot may be stale by the time the notification is actually sent. Defer collecting editors until afterclient.readyresolves (as before) so the server receives a current snapshot.
const cppEditors: vscode.TextEditor[] = vscode.window.visibleTextEditors.filter(e => util.isCpp(e.document));
void client.onDidChangeVisibleTextEditors(cppEditors).catch(logAndReturn.undefined);
Extension/src/LanguageServer/extension.ts:192
- The
onDidChangeTextEditorVisibleRanges/onDidChangeVisibleTextEditorshandlers areasyncand can reject, but their returned Promises are currently ignored by the event registration callbacks. If either throws/rejects, it can surface as an unhandled promise rejection. Wrap the calls withvoid ... .catch(logAndReturn.undefined)like other event hooks in this file.
disposables.push(vscode.window.onDidChangeTextEditorVisibleRanges(e => onDidChangeTextEditorVisibleRanges(e)));
disposables.push(vscode.window.onDidChangeActiveTextEditor(e => onDidChangeActiveTextEditor(e)));
ui.didChangeActiveEditor(); // Handle already active documents (for non-cpp files that we don't register didOpen).
disposables.push(vscode.window.onDidChangeTextEditorSelection(e => onDidChangeTextEditorSelection(e)));
disposables.push(vscode.window.onDidChangeVisibleTextEditors(e => onDidChangeVisibleTextEditors(e)));
I can check... EDIT: I did not reproduce this problem, but put out a separate PR to filter unnecessary settings change events which dropped my count down to 0. |
|
Bob Brown (@bobbrow) Do you want us to try to check this in? |
Yes, I was hoping it would get a review and sign off for 1.34.0. |
Sean McManus (sean-mcmanus)
left a comment
Sean McManus (sean-mcmanus)
left a comment
There was a problem hiding this comment.
✨Copilot (agent8): I found one additional startup-ordering issue on the current head.
| this.innerLanguageClient = languageClient; | ||
| // Ideally this would be set earlier, but the task provider expects it to also mean that `this.innerConfiguration` is set. | ||
| this.languageClient.isStarted = true; | ||
| compilerDefaults = await this.requestCompiler(); |
There was a problem hiding this comment.
✨Copilot (agent8): [Moderate] This moves the global compiler-defaults request out of the existing "all clients configured" guard. ClientCollection creates one DefaultClient per workspace folder, so every client's init() now sends cpptools/queryCompilerDefaults before setupConfigurations(); previously the last client sent it once after all clients were configured. This multiplies startup work in multiroot workspaces and changes the ordering that the surrounding comment says is required. Could we use a separate RPC-started gate for the wrapper while keeping DefaultClient.ready and this once-only request at the end of initialization? A multiroot test that asserts one compiler-defaults request would also protect this sequencing.
There are some crash reports indicating that some events attempting to communicate with cpptools are firing before the language client is ready to service them. While investigating, I noticed that several events are not awaiting the
readyevent.This PR wraps the LanguageClient interface in a class that ensures the client is ready before making any RPC calls into cpptools. Some other minor cleanup in the provider classes is included. I did not root out all places where
DefaultClient.readyis being unnecessarily awaited as it will require more testing, but those remaining are harmless.Note: I did not see a need to keep the
enqueuefunctionality based on my findings. On the native side, we already defer all messages in a queue until configuration happens so we can let the messages come in the order they are queued on the TypeScript side. Historically, I remember there being a need for "blocking" the language client "Middleware" (protocolFilter.ts), but that was already removed a while ago.enqueueis incompatible with this change as it results in deadlocked awaiting (an enqueued task that makes an RPC call to cpptools).