language: Move language server update to background when it takes too long (#43164)

Smit Barmase and Piotr Osiewicz created

Closes https://github.com/zed-industries/zed/issues/42360

If updating a language server takes longer than 10 seconds, we now fall
back to launching the currently installed version (if exists) and
continue downloading the update in the background.

Release Notes:

- Improved language server updates for slow connection, now Zed launches
existing server if the update is taking too long.

---------

Co-authored-by: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com>

Change summary

crates/language/src/language.rs                        | 151 ++++++-----
crates/language_extension/src/extension_lsp_adapter.rs |  79 +++---
crates/languages/src/css.rs                            |  17 -
crates/languages/src/json.rs                           |  14 
crates/languages/src/tailwind.rs                       |  17 -
crates/languages/src/yaml.rs                           |  17 -
crates/project/src/lsp_store.rs                        |  27 +
7 files changed, 163 insertions(+), 159 deletions(-)

Detailed changes

crates/language/src/language.rs 🔗

@@ -28,6 +28,8 @@ use anyhow::{Context as _, Result};
 use async_trait::async_trait;
 use collections::{HashMap, HashSet, IndexSet};
 use futures::Future;
+use futures::future::LocalBoxFuture;
+use futures::lock::OwnedMutexGuard;
 use gpui::{App, AsyncApp, Entity, SharedString};
 pub use highlight_map::HighlightMap;
 use http_client::HttpClient;
@@ -51,7 +53,6 @@ use std::{
     mem,
     ops::{DerefMut, Range},
     path::{Path, PathBuf},
-    pin::Pin,
     str,
     sync::{
         Arc, LazyLock,
@@ -152,7 +153,14 @@ pub struct Location {
 }
 
 type ServerBinaryCache = futures::lock::Mutex<Option<(bool, LanguageServerBinary)>>;
-
+type DownloadableLanguageServerBinary = LocalBoxFuture<'static, Result<LanguageServerBinary>>;
+pub type LanguageServerBinaryLocations = LocalBoxFuture<
+    'static,
+    (
+        Result<LanguageServerBinary>,
+        Option<DownloadableLanguageServerBinary>,
+    ),
+>;
 /// Represents a Language Server, with certain cached sync properties.
 /// Uses [`LspAdapter`] under the hood, but calls all 'static' methods
 /// once at startup, and caches the results.
@@ -162,7 +170,7 @@ pub struct CachedLspAdapter {
     pub disk_based_diagnostics_progress_token: Option<String>,
     language_ids: HashMap<LanguageName, String>,
     pub adapter: Arc<dyn LspAdapter>,
-    cached_binary: ServerBinaryCache,
+    cached_binary: Arc<ServerBinaryCache>,
 }
 
 impl Debug for CachedLspAdapter {
@@ -209,18 +217,15 @@ impl CachedLspAdapter {
         toolchains: Option<Toolchain>,
         binary_options: LanguageServerBinaryOptions,
         cx: &mut AsyncApp,
-    ) -> Result<LanguageServerBinary> {
-        let mut cached_binary = self.cached_binary.lock().await;
-        self.adapter
-            .clone()
-            .get_language_server_command(
-                delegate,
-                toolchains,
-                binary_options,
-                &mut cached_binary,
-                cx,
-            )
-            .await
+    ) -> LanguageServerBinaryLocations {
+        let cached_binary = self.cached_binary.clone().lock_owned().await;
+        self.adapter.clone().get_language_server_command(
+            delegate,
+            toolchains,
+            binary_options,
+            cached_binary,
+            cx.clone(),
+        )
     }
 
     pub fn code_action_kinds(&self) -> Option<Vec<CodeActionKind>> {
@@ -513,14 +518,14 @@ pub trait DynLspInstaller {
         pre_release: bool,
         cx: &mut AsyncApp,
     ) -> Result<LanguageServerBinary>;
-    fn get_language_server_command<'a>(
+    fn get_language_server_command(
         self: Arc<Self>,
         delegate: Arc<dyn LspAdapterDelegate>,
         toolchains: Option<Toolchain>,
         binary_options: LanguageServerBinaryOptions,
-        cached_binary: &'a mut Option<(bool, LanguageServerBinary)>,
-        cx: &'a mut AsyncApp,
-    ) -> Pin<Box<dyn 'a + Future<Output = Result<LanguageServerBinary>>>>;
+        cached_binary: OwnedMutexGuard<Option<(bool, LanguageServerBinary)>>,
+        cx: AsyncApp,
+    ) -> LanguageServerBinaryLocations;
 }
 
 #[async_trait(?Send)]
@@ -562,15 +567,16 @@ where
             binary
         }
     }
-    fn get_language_server_command<'a>(
+    fn get_language_server_command(
         self: Arc<Self>,
         delegate: Arc<dyn LspAdapterDelegate>,
         toolchain: Option<Toolchain>,
         binary_options: LanguageServerBinaryOptions,
-        cached_binary: &'a mut Option<(bool, LanguageServerBinary)>,
-        cx: &'a mut AsyncApp,
-    ) -> Pin<Box<dyn 'a + Future<Output = Result<LanguageServerBinary>>>> {
+        mut cached_binary: OwnedMutexGuard<Option<(bool, LanguageServerBinary)>>,
+        mut cx: AsyncApp,
+    ) -> LanguageServerBinaryLocations {
         async move {
+            let cached_binary_deref = cached_binary.deref_mut();
             // First we check whether the adapter can give us a user-installed binary.
             // If so, we do *not* want to cache that, because each worktree might give us a different
             // binary:
@@ -584,7 +590,7 @@ where
             // for each worktree we might have open.
             if binary_options.allow_path_lookup
                 && let Some(binary) = self
-                    .check_if_user_installed(delegate.as_ref(), toolchain, cx)
+                    .check_if_user_installed(delegate.as_ref(), toolchain, &mut cx)
                     .await
             {
                 log::info!(
@@ -593,62 +599,77 @@ where
                     binary.path,
                     binary.arguments
                 );
-                return Ok(binary);
+                return (Ok(binary), None);
             }
 
-            anyhow::ensure!(
-                binary_options.allow_binary_download,
-                "downloading language servers disabled"
-            );
+            if !binary_options.allow_binary_download {
+                return (
+                    Err(anyhow::anyhow!("downloading language servers disabled")),
+                    None,
+                );
+            }
 
-            if let Some((pre_release, cached_binary)) = cached_binary
+            if let Some((pre_release, cached_binary)) = cached_binary_deref
                 && *pre_release == binary_options.pre_release
             {
-                return Ok(cached_binary.clone());
+                return (Ok(cached_binary.clone()), None);
             }
 
             let Some(container_dir) = delegate.language_server_download_dir(&self.name()).await
             else {
-                anyhow::bail!("no language server download dir defined")
+                return (
+                    Err(anyhow::anyhow!("no language server download dir defined")),
+                    None,
+                );
             };
 
-            let mut binary = self
-                .try_fetch_server_binary(
-                    &delegate,
-                    container_dir.to_path_buf(),
-                    binary_options.pre_release,
-                    cx,
-                )
-                .await;
+            let last_downloaded_binary = self
+                .cached_server_binary(container_dir.to_path_buf(), delegate.as_ref())
+                .await
+                .context(
+                    "did not find existing language server binary, falling back to downloading",
+                );
+            let download_binary = async move {
+                let mut binary = self
+                    .try_fetch_server_binary(
+                        &delegate,
+                        container_dir.to_path_buf(),
+                        binary_options.pre_release,
+                        &mut cx,
+                    )
+                    .await;
+
+                if let Err(error) = binary.as_ref() {
+                    if let Some(prev_downloaded_binary) = self
+                        .cached_server_binary(container_dir.to_path_buf(), delegate.as_ref())
+                        .await
+                    {
+                        log::info!(
+                            "failed to fetch newest version of language server {:?}. \
+                            error: {:?}, falling back to using {:?}",
+                            self.name(),
+                            error,
+                            prev_downloaded_binary.path
+                        );
+                        binary = Ok(prev_downloaded_binary);
+                    } else {
+                        delegate.update_status(
+                            self.name(),
+                            BinaryStatus::Failed {
+                                error: format!("{error:?}"),
+                            },
+                        );
+                    }
+                }
 
-            if let Err(error) = binary.as_ref() {
-                if let Some(prev_downloaded_binary) = self
-                    .cached_server_binary(container_dir.to_path_buf(), delegate.as_ref())
-                    .await
-                {
-                    log::info!(
-                        "failed to fetch newest version of language server {:?}. \
-                        error: {:?}, falling back to using {:?}",
-                        self.name(),
-                        error,
-                        prev_downloaded_binary.path
-                    );
-                    binary = Ok(prev_downloaded_binary);
-                } else {
-                    delegate.update_status(
-                        self.name(),
-                        BinaryStatus::Failed {
-                            error: format!("{error:?}"),
-                        },
-                    );
+                if let Ok(binary) = &binary {
+                    *cached_binary = Some((binary_options.pre_release, binary.clone()));
                 }
-            }
 
-            if let Ok(binary) = &binary {
-                *cached_binary = Some((binary_options.pre_release, binary.clone()));
+                binary
             }
-
-            binary
+            .boxed_local();
+            (last_downloaded_binary, Some(download_binary))
         }
         .boxed_local()
     }

crates/language_extension/src/extension_lsp_adapter.rs 🔗

@@ -1,17 +1,16 @@
 use std::ops::Range;
 use std::path::PathBuf;
-use std::pin::Pin;
 use std::sync::Arc;
 
 use anyhow::{Context as _, Result};
 use async_trait::async_trait;
 use collections::{HashMap, HashSet};
 use extension::{Extension, ExtensionLanguageServerProxy, WorktreeDelegate};
-use futures::{Future, FutureExt, future::join_all};
+use futures::{FutureExt, future::join_all, lock::OwnedMutexGuard};
 use gpui::{App, AppContext, AsyncApp, Task};
 use language::{
-    BinaryStatus, CodeLabel, DynLspInstaller, HighlightId, Language, LanguageName, LspAdapter,
-    LspAdapterDelegate, Toolchain,
+    BinaryStatus, CodeLabel, DynLspInstaller, HighlightId, Language, LanguageName,
+    LanguageServerBinaryLocations, LspAdapter, LspAdapterDelegate, Toolchain,
 };
 use lsp::{
     CodeActionKind, LanguageServerBinary, LanguageServerBinaryOptions, LanguageServerName,
@@ -155,47 +154,51 @@ impl ExtensionLspAdapter {
 
 #[async_trait(?Send)]
 impl DynLspInstaller for ExtensionLspAdapter {
-    fn get_language_server_command<'a>(
+    fn get_language_server_command(
         self: Arc<Self>,
         delegate: Arc<dyn LspAdapterDelegate>,
         _: Option<Toolchain>,
         _: LanguageServerBinaryOptions,
-        _: &'a mut Option<(bool, LanguageServerBinary)>,
-        _: &'a mut AsyncApp,
-    ) -> Pin<Box<dyn 'a + Future<Output = Result<LanguageServerBinary>>>> {
+        _: OwnedMutexGuard<Option<(bool, LanguageServerBinary)>>,
+        _: AsyncApp,
+    ) -> LanguageServerBinaryLocations {
         async move {
-            let delegate = Arc::new(WorktreeDelegateAdapter(delegate.clone())) as _;
-            let command = self
-                .extension
-                .language_server_command(
-                    self.language_server_id.clone(),
-                    self.language_name.clone(),
-                    delegate,
-                )
-                .await?;
-
-            let path = self.extension.path_from_extension(command.command.as_ref());
-
-            // TODO: This should now be done via the `zed::make_file_executable` function in
-            // Zed extension API, but we're leaving these existing usages in place temporarily
-            // to avoid any compatibility issues between Zed and the extension versions.
-            //
-            // We can remove once the following extension versions no longer see any use:
-            // - toml@0.0.2
-            // - zig@0.0.1
-            if ["toml", "zig"].contains(&self.extension.manifest().id.as_ref())
-                && path.starts_with(&self.extension.work_dir())
-            {
-                make_file_executable(&path)
-                    .await
-                    .context("failed to set file permissions")?;
-            }
+            let ret = maybe!(async move {
+                let delegate = Arc::new(WorktreeDelegateAdapter(delegate.clone())) as _;
+                let command = self
+                    .extension
+                    .language_server_command(
+                        self.language_server_id.clone(),
+                        self.language_name.clone(),
+                        delegate,
+                    )
+                    .await?;
+
+                let path = self.extension.path_from_extension(command.command.as_ref());
+
+                // TODO: This should now be done via the `zed::make_file_executable` function in
+                // Zed extension API, but we're leaving these existing usages in place temporarily
+                // to avoid any compatibility issues between Zed and the extension versions.
+                //
+                // We can remove once the following extension versions no longer see any use:
+                // - toml@0.0.2
+                // - zig@0.0.1
+                if ["toml", "zig"].contains(&self.extension.manifest().id.as_ref())
+                    && path.starts_with(&self.extension.work_dir())
+                {
+                    make_file_executable(&path)
+                        .await
+                        .context("failed to set file permissions")?;
+                }
 
-            Ok(LanguageServerBinary {
-                path,
-                arguments: command.args.into_iter().map(|arg| arg.into()).collect(),
-                env: Some(command.env.into_iter().collect()),
+                Ok(LanguageServerBinary {
+                    path,
+                    arguments: command.args.into_iter().map(|arg| arg.into()).collect(),
+                    env: Some(command.env.into_iter().collect()),
+                })
             })
+            .await;
+            (ret, None)
         }
         .boxed_local()
     }

crates/languages/src/css.rs 🔗

@@ -1,13 +1,11 @@
-use anyhow::{Context as _, Result};
+use anyhow::Result;
 use async_trait::async_trait;
-use futures::StreamExt;
 use gpui::AsyncApp;
 use language::{LspAdapter, LspAdapterDelegate, LspInstaller, Toolchain};
 use lsp::{LanguageServerBinary, LanguageServerName};
 use node_runtime::{NodeRuntime, VersionStrategy};
 use project::lsp_store::language_server_settings;
 use serde_json::json;
-use smol::fs;
 use std::{
     ffi::OsString,
     path::{Path, PathBuf},
@@ -176,19 +174,10 @@ async fn get_cached_server_binary(
     node: &NodeRuntime,
 ) -> Option<LanguageServerBinary> {
     maybe!(async {
-        let mut last_version_dir = None;
-        let mut entries = fs::read_dir(&container_dir).await?;
-        while let Some(entry) = entries.next().await {
-            let entry = entry?;
-            if entry.file_type().await?.is_dir() {
-                last_version_dir = Some(entry.path());
-            }
-        }
-        let last_version_dir = last_version_dir.context("no cached binary")?;
-        let server_path = last_version_dir.join(SERVER_PATH);
+        let server_path = container_dir.join(SERVER_PATH);
         anyhow::ensure!(
             server_path.exists(),
-            "missing executable in directory {last_version_dir:?}"
+            "missing executable in directory {server_path:?}"
         );
         Ok(LanguageServerBinary {
             path: node.binary_path().await?,

crates/languages/src/json.rs 🔗

@@ -301,20 +301,10 @@ async fn get_cached_server_binary(
     node: &NodeRuntime,
 ) -> Option<LanguageServerBinary> {
     maybe!(async {
-        let mut last_version_dir = None;
-        let mut entries = fs::read_dir(&container_dir).await?;
-        while let Some(entry) = entries.next().await {
-            let entry = entry?;
-            if entry.file_type().await?.is_dir() {
-                last_version_dir = Some(entry.path());
-            }
-        }
-
-        let last_version_dir = last_version_dir.context("no cached binary")?;
-        let server_path = last_version_dir.join(SERVER_PATH);
+        let server_path = container_dir.join(SERVER_PATH);
         anyhow::ensure!(
             server_path.exists(),
-            "missing executable in directory {last_version_dir:?}"
+            "missing executable in directory {server_path:?}"
         );
         Ok(LanguageServerBinary {
             path: node.binary_path().await?,

crates/languages/src/tailwind.rs 🔗

@@ -1,14 +1,12 @@
-use anyhow::{Context as _, Result};
+use anyhow::Result;
 use async_trait::async_trait;
 use collections::HashMap;
-use futures::StreamExt;
 use gpui::AsyncApp;
 use language::{LanguageName, LspAdapter, LspAdapterDelegate, LspInstaller, Toolchain};
 use lsp::{LanguageServerBinary, LanguageServerName};
 use node_runtime::{NodeRuntime, VersionStrategy};
 use project::lsp_store::language_server_settings;
 use serde_json::{Value, json};
-use smol::fs;
 use std::{
     ffi::OsString,
     path::{Path, PathBuf},
@@ -198,19 +196,10 @@ async fn get_cached_server_binary(
     node: &NodeRuntime,
 ) -> Option<LanguageServerBinary> {
     maybe!(async {
-        let mut last_version_dir = None;
-        let mut entries = fs::read_dir(&container_dir).await?;
-        while let Some(entry) = entries.next().await {
-            let entry = entry?;
-            if entry.file_type().await?.is_dir() {
-                last_version_dir = Some(entry.path());
-            }
-        }
-        let last_version_dir = last_version_dir.context("no cached binary")?;
-        let server_path = last_version_dir.join(SERVER_PATH);
+        let server_path = container_dir.join(SERVER_PATH);
         anyhow::ensure!(
             server_path.exists(),
-            "missing executable in directory {last_version_dir:?}"
+            "missing executable in directory {server_path:?}"
         );
         Ok(LanguageServerBinary {
             path: node.binary_path().await?,

crates/languages/src/yaml.rs 🔗

@@ -1,6 +1,5 @@
-use anyhow::{Context as _, Result};
+use anyhow::Result;
 use async_trait::async_trait;
-use futures::StreamExt;
 use gpui::AsyncApp;
 use language::{
     LspAdapter, LspAdapterDelegate, LspInstaller, Toolchain, language_settings::AllLanguageSettings,
@@ -10,7 +9,6 @@ use node_runtime::{NodeRuntime, VersionStrategy};
 use project::lsp_store::language_server_settings;
 use serde_json::Value;
 use settings::{Settings, SettingsLocation};
-use smol::fs;
 use std::{
     ffi::OsString,
     path::{Path, PathBuf},
@@ -171,19 +169,10 @@ async fn get_cached_server_binary(
     node: &NodeRuntime,
 ) -> Option<LanguageServerBinary> {
     maybe!(async {
-        let mut last_version_dir = None;
-        let mut entries = fs::read_dir(&container_dir).await?;
-        while let Some(entry) = entries.next().await {
-            let entry = entry?;
-            if entry.file_type().await?.is_dir() {
-                last_version_dir = Some(entry.path());
-            }
-        }
-        let last_version_dir = last_version_dir.context("no cached binary")?;
-        let server_path = last_version_dir.join(SERVER_PATH);
+        let server_path = container_dir.join(SERVER_PATH);
         anyhow::ensure!(
             server_path.exists(),
-            "missing executable in directory {last_version_dir:?}"
+            "missing executable in directory {server_path:?}"
         );
         Ok(LanguageServerBinary {
             path: node.binary_path().await?,

crates/project/src/lsp_store.rs 🔗

@@ -139,6 +139,7 @@ pub use worktree::{
 const SERVER_LAUNCHING_BEFORE_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(5);
 pub const SERVER_PROGRESS_THROTTLE_TIMEOUT: Duration = Duration::from_millis(100);
 const WORKSPACE_DIAGNOSTICS_TOKEN_START: &str = "id:";
+const SERVER_DOWNLOAD_TIMEOUT: Duration = Duration::from_secs(10);
 
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize)]
 pub enum ProgressToken {
@@ -599,14 +600,36 @@ impl LocalLspStore {
         };
 
         cx.spawn(async move |cx| {
-            let binary_result = adapter
+            let (existing_binary, maybe_download_binary) = adapter
                 .clone()
                 .get_language_server_command(delegate.clone(), toolchain, lsp_binary_options, cx)
+                .await
                 .await;
 
             delegate.update_status(adapter.name.clone(), BinaryStatus::None);
 
-            let mut binary = binary_result?;
+            let mut binary = match (existing_binary, maybe_download_binary) {
+                (binary, None) => binary?,
+                (Err(_), Some(downloader)) => downloader.await?,
+                (Ok(existing_binary), Some(downloader)) => {
+                    let mut download_timeout = cx
+                        .background_executor()
+                        .timer(SERVER_DOWNLOAD_TIMEOUT)
+                        .fuse();
+                    let mut downloader = downloader.fuse();
+                    futures::select! {
+                        _ = download_timeout => {
+                            // Return existing binary and kick the existing work to the background.
+                            cx.spawn(async move |_| downloader.await).detach();
+                            Ok(existing_binary)
+                        },
+                        downloaded_or_existing_binary = downloader => {
+                            // If download fails, this results in the existing binary.
+                            downloaded_or_existing_binary
+                        }
+                    }?
+                }
+            };
             let mut shell_env = delegate.shell_env().await;
 
             shell_env.extend(binary.env.unwrap_or_default());