Skip to content

Start R language server only when needed and stop after 30s.#1708

Open
Fred-Wu wants to merge 1 commit into
REditorSupport:masterfrom
Fred-Wu:fix/lsp-lifecycle-management
Open

Start R language server only when needed and stop after 30s.#1708
Fred-Wu wants to merge 1 commit into
REditorSupport:masterfrom
Fred-Wu:fix/lsp-lifecycle-management

Conversation

@Fred-Wu

@Fred-Wu Fred-Wu commented May 14, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR updates the R language server lifecycle so that the server starts only when an R-related document is opened, and shuts down after all R-related documents are closed.

This avoids keeping the language server running unnecessarily while still preserving the existing session if another R-related file is opened shortly after the last one is closed.

Main Changes

  • The language server no longer starts eagerly. It now starts only when an R-related file is opened.
  • When the last R-related file is closed, a 30-second shutdown timer is started.
  • If another R-related file is opened before the timer finishes, the shutdown is cancelled and the existing language server session is kept alive.
  • Before shutting down, the timer performs a final check of the currently open editors. If any R-related file is still open, shutdown is aborted.
  • This lifecycle behaviour works for both single-server mode and multi-workspace-server mode.

…vity

- Start the language server only when an R file is opened.
- Stop the server automatically 30 seconds after the last R file is closed.
- Cancel the shutdown timer if a new R file is opened within those 30 seconds.
- Fix "server disconnected" errors by double-checking that no R files are open exactly when the 30-second timer finishes, preventing the server from being accidentally killed while in use.
@randy3k

randy3k commented Jun 26, 2026

Copy link
Copy Markdown
Member

Thanks for the contribution. I love the idea, but there are quite some change.
I asked both Claude and Gemini to review the PR. Below are what they found. Feel free to discard if the comments are not correct or not importment.


Overall Review
This is a fantastic PR! The architecture for deferring the language server start and introducing the 30-second idle stop is very well thought out. The deduplication logic for rapid starts and the smooth handling of single vs. multi-server modes are great improvements that will definitely help with performance and memory usage.

I did notice a few issues during review—one critical bug regarding Quarto documents and two smaller edge cases—that should be addressed before merging:

1. The Multi-Server Quarto Bug (Critical)

In closeMultiClient, there is a manual cleanup block that triggers when a Quarto document is closed. It loops through all servers, and if a server's remaining tracked documents are all Quarto chunks (.vdoc.r), it wipes the tracking set (this.trackedDocuments.delete(serverKey)) and schedules a stop.

This introduces a cross-document bug:

  • If a user has two Quarto files (A.qmd and B.qmd) open in the same workspace, closing A.qmd evaluates to true here (because B.vdoc.r is a Quarto chunk). The server tracking is wiped and the server is killed, unexpectedly breaking LSP features for the still-open B.qmd.
  • Furthermore, because isUntitledQuartoDocument returns true for any untitled r or rmd document, simply closing an unrelated Untitled-1.R script will trigger this dangerous Quarto cleanup loop.

Suggested Fix:
The safest and simplest fix is to remove this manual Quarto cleanup block entirely. The Quarto extension already disposes of its virtual documents when the parent .qmd is closed, which automatically triggers onDidCloseTextDocument for the .vdoc.r files, naturally untracking them.

2. Dropped scheme Filter in Single-Server Mode

In the old code, the single-server mode explicitly restricted document selectors to scheme: 'file', scheme: 'untitled', and scheme: 'vscode-notebook-cell'.

The new singleServerDocumentSelector() simplifies this to just { language: 'r' } and { language: 'rmd' }.
Because the scheme filter was dropped, the R language server will now attempt to spin up for any URI scheme—including git:// URIs. This means users might experience the heavy R server spinning up just from clicking to view a Git diff in the source control tab.

Suggested Fix:
Restore the scheme restrictions in singleServerDocumentSelector() to prevent unintended server starts.

3. Missing Client Disposal on Crash (Code Hygiene)

In spawnServer, the child process on('exit') callback now maps to handleClientExit. handleClientExit removes the client from this.clients, but never calls dispose() on it.

Because createClient pushes every new client into extensionContext.subscriptions, these crashed/stale clients will accumulate in the subscriptions array unboundedly until the extension is deactivated.

Suggested Fix:
To prevent accumulating stale event listeners, dispose of the client when it crashes by adding a cleanup call inside handleClientExit:

private handleClientExit(serverKey: string, client: LanguageClient): void {
    if (this.clients.get(serverKey) !== client) { return; }
    this.clients.delete(serverKey);
    this.initSet.delete(serverKey);
    this.cancelIdleStop(serverKey);
    void this.stopStartedClient(client); // Ensures the crashed client is disposed
}

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.

2 participants