Skip to content

fix: MonacoLspClient should implement IDisposable#5339

Open
apatenotre wants to merge 1 commit into
microsoft:mainfrom
apatenotre:fix/dispose-lsp-feature-on-close
Open

fix: MonacoLspClient should implement IDisposable#5339
apatenotre wants to merge 1 commit into
microsoft:mainfrom
apatenotre:fix/dispose-lsp-feature-on-close

Conversation

@apatenotre
Copy link
Copy Markdown

@apatenotre apatenotre commented May 25, 2026

[LSP] Make MonacoLspClient disposable

Fixes #5340.

MonacoLspClient.createFeatures() already returns an IDisposable covering every Monaco provider registration the client installs, but the constructor discards it and the class has no dispose() method. Embedders with a component lifecycle (React, Vue, …) can't release the registrations, so each remount leaks providers, leading to a permanent "Loading…" entry in hover/code-action widgets contributed by the stale providers (whose transport is closed and whose hover(...)/codeAction(...) requests never resolve).

This PR stashes the disposable on the instance and exposes dispose().

Verification done locally

In a Next.js + Monaco app that builds against monaco-editor 0.55.1 (which bundles this client), I patched MonacoLspClient.prototype to mirror this fix and confirmed:

  1. Before the fix — close all editor tabs (component unmount, transport disposed) → reopen a tab → hover any symbol → Monaco hover widget shows the real server response plus a "Loading…" row that never resolves. Repeating close+reopen N times stacks N "Loading…" rows.
  2. After the fix — same repro, hover widget shows only the real server response. Stable across arbitrary close+reopen cycles.

Notes

  • No test added. monaco-lsp-client/ currently has no test infrastructure (no test/ folder, no test script in package.json, no test framework dep). Happy to add one if you pick a framework; the assertion is straightforward: stub monaco.languages.register*Provider to capture the returned disposables, construct + dispose the client, assert each captured disposable is now disposed.
  • I considered also adding cleanup for LspCapabilitiesRegistry / TextDocumentSynchronizer / LspConnection, but those don't register Monaco providers and don't contribute to the observed leak. Scoping this PR to the immediate user-visible bug.
  • dispose() is idempotent because DisposableStore.dispose() is idempotent (no-op after first call).

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] [LSP] MonacoLspClient never disposes its language-feature registrations

1 participant