language_extension: Hold LspStore weakly in LspAccess::ViaLspStore (#60558)
# Objective Fix a Windows CI flake in `extension_host extension_store_test::test_extension_store_with_test_extension`, which panicked in GPUI's leak detector with three leaked `LspStore` handles ([example run](https://github.com/zed-industries/zed/actions/runs/28871529605/job/85635418351)). ## Solution `LspAccess::ViaLspStore` held a strong `Entity<LspStore>`, cloned into three `ExtensionHostProxy` registrations by `language_extension::init`. The proxy sits inside an `Arc` cycle (proxy → `LanguageServerRegistryProxy` → `LanguageRegistry` → `ExtensionLspAdapter` → `WasmExtension` → `WasmHost` → proxy), so whenever an extension LSP adapter was registered at app-drop time, the cycle pinned the `LspStore` entity after its owning `Project` dropped. The flake was purely timing: extension reload toggles the adapter registration, and an unclean LSP pipe shutdown on Windows shifted teardown into the pinned window. `ViaLspStore` now holds a `WeakEntity<LspStore>`, upgraded at its sole use site (`remove_language_server`), skipping the stop task when the store is gone — matching the existing `ViaWorkspaces` semantics. A dead store is the expected terminal state after the owning project drops, so no error is logged or propagated. `Project`/`HeadlessProject` remain the sole long-term owners of `LspStore`, which is already the convention everywhere else (e.g. `json_schema_store`, the `lsp_store` message handlers). ## Testing - Ran `cargo nextest run -p extension_host extension_store_test::test_extension_store_with_test_extension` locally: passes. - The flake is timing-dependent (reproduced on Windows CI), so a local pass doesn't prove absence; the fix removes the only strong non-owner handle, which the leak detector reported. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - N/A --------- Co-authored-by: zed-zippy[bot] <234243425+zed-zippy[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
042d22ce9c
commit
07fa666c4e
4 files changed
+21
-14
No files matched your search
@@ -769,7 +769,11 @@ async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) {
|
||||
theme_extension::init(proxy.clone(), theme_registry.clone(), cx.executor());
|
||||
let language_registry = project.read_with(cx, |project, _cx| project.languages().clone());
|
||||
language_extension::init(
|
||||
LspAccess::ViaLspStore(project.update(cx, |project, _| project.lsp_store())),
|
||||
LspAccess::ViaLspStore(
|
||||
project
|
||||
.update(cx, |project, _| project.lsp_store())
|
||||
.downgrade(),
|
||||
),
|
||||
proxy.clone(),
|
||||
language_registry.clone(),
|
||||
);
|
||||
|
||||
@@ -79,16 +79,19 @@ impl ExtensionLanguageServerProxy for LanguageServerRegistryProxy {
|
||||
|
||||
let mut tasks = Vec::new();
|
||||
match &self.lsp_access {
|
||||
LspAccess::ViaLspStore(lsp_store) => lsp_store.update(cx, |lsp_store, cx| {
|
||||
let stop_task = lsp_store.stop_language_servers_for_buffers(
|
||||
Vec::new(),
|
||||
HashSet::from_iter([LanguageServerSelector::Name(
|
||||
language_server_name.clone(),
|
||||
)]),
|
||||
cx,
|
||||
);
|
||||
tasks.push(stop_task);
|
||||
}),
|
||||
LspAccess::ViaLspStore(lsp_store) => {
|
||||
if let Ok(stop_task) = lsp_store.update(cx, |lsp_store, cx| {
|
||||
lsp_store.stop_language_servers_for_buffers(
|
||||
Vec::new(),
|
||||
HashSet::from_iter([LanguageServerSelector::Name(
|
||||
language_server_name.clone(),
|
||||
)]),
|
||||
cx,
|
||||
)
|
||||
}) {
|
||||
tasks.push(stop_task);
|
||||
}
|
||||
}
|
||||
LspAccess::ViaWorkspaces(lsp_store_provider) => {
|
||||
if let Ok(lsp_stores) = lsp_store_provider(cx) {
|
||||
for lsp_store in lsp_stores {
|
||||
|
||||
@@ -5,13 +5,13 @@ use std::sync::Arc;
|
||||
|
||||
use anyhow::Result;
|
||||
use extension::{ExtensionGrammarProxy, ExtensionHostProxy, ExtensionLanguageProxy};
|
||||
use gpui::{App, Entity};
|
||||
use gpui::{App, Entity, WeakEntity};
|
||||
use language::{LanguageMatcher, LanguageName, LanguageRegistry, LoadedLanguage};
|
||||
use project::LspStore;
|
||||
|
||||
#[derive(Clone)]
|
||||
pub enum LspAccess {
|
||||
ViaLspStore(Entity<LspStore>),
|
||||
ViaLspStore(WeakEntity<LspStore>),
|
||||
ViaWorkspaces(Arc<dyn Fn(&mut App) -> Result<Vec<Entity<LspStore>>> + Send + Sync + 'static>),
|
||||
Noop,
|
||||
}
|
||||
|
||||
@@ -254,7 +254,7 @@ impl HeadlessProject {
|
||||
|
||||
cx.subscribe(&lsp_store, Self::on_lsp_store_event).detach();
|
||||
language_extension::init(
|
||||
language_extension::LspAccess::ViaLspStore(lsp_store.clone()),
|
||||
language_extension::LspAccess::ViaLspStore(lsp_store.downgrade()),
|
||||
proxy.clone(),
|
||||
languages.clone(),
|
||||
);
|
||||
|
||||
Reference in new issue
Block a user