Fix LSP deadlocks and hangs under heavy editor activity

Author: AJenbo

docs/CHANGELOG.md

@@ -58,6 +58,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **LSP no longer freezes under heavy editor activity.** Server-to-client requests (diagnostic refresh, progress token creation) could deadlock the service loop when the editor was simultaneously sending bursts of open/close/hover messages. All server-to-client requests are now either fire-and-forget or time-bounded, long-running handlers are cancellation-safe, and the process exits cleanly if the service loop ever terminates unexpectedly.
 - **Rename class preserves `self`, `static`, and `parent` keywords.** Renaming a class no longer replaces occurrences of `self::`, `static::`, or `parent::` with the new class name.
 - **Rename propagates into closures and arrow functions.** Renaming a variable now follows explicit `use ($var)` captures into closure bodies and implicit captures into arrow function bodies, instead of leaving those occurrences unchanged.
 - **Spurious function auto-imports.** Import statements like `use function is_array;` were misidentified as function declarations, polluting the completion list with phantom entries that inserted incorrect imports.

docs/todo/bugs.md

@@ -21,3 +21,4 @@ handle this automatically.
 **Tests:** Assertion lines were removed from
 `tests/psalm_assertions/method_call.php` (out of scope until
 upstream stubs land).
+

src/diagnostics/mod.rs

@@ -1728,9 +1728,18 @@ impl Backend {
         client.publish_diagnostics(uri, Vec::new(), None).await;
 
         if self.supports_pull_diagnostics.load(Ordering::Acquire) {
-            // Also send a refresh so the editor re-pulls (and gets
-            // empty results for the now-closed file).
-            let _ = client.workspace_diagnostic_refresh().await;
+            // Tell the editor to re-pull diagnostics.  We spawn this
+            // as a detached task instead of awaiting it because
+            // workspace_diagnostic_refresh is a server-to-client
+            // *request* that blocks until the client responds.  When
+            // the editor closes many files in a burst, each didClose
+            // handler would await a response while the client is busy
+            // sending more messages, deadlocking the tower-lsp
+            // service loop.
+            let client = client.clone();
+            tokio::spawn(async move {
+                let _ = client.workspace_diagnostic_refresh().await;
+            });
         }
     }
 }

src/main.rs

@@ -283,13 +283,25 @@ async fn main() {
                 let (read, write) = tokio::io::split(stream);
                 let (service, socket) = LspService::build(Backend::new).finish();
                 Server::new(read, write, socket).serve(service).await;
+                // The serve loop exited (client disconnected or an
+                // internal error occurred).  Exit the process so the
+                // editor can restart us instead of leaving a zombie
+                // that consumes no CPU but never responds.
+                tracing::warn!("tower-lsp serve loop exited (TCP), shutting down");
+                std::process::exit(0);
             } else {
                 // Default: run the LSP server over stdin/stdout.
                 let stdin = tokio::io::stdin();
                 let stdout = tokio::io::stdout();
 
                 let (service, socket) = LspService::build(Backend::new).finish();
                 Server::new(stdin, stdout, socket).serve(service).await;
+                // Same as above: the serve loop exited.  Without this
+                // explicit exit, the process hangs because the tokio
+                // blocking stdin reader thread keeps the runtime alive
+                // even though no tasks are consuming the data.
+                tracing::warn!("tower-lsp serve loop exited (stdio), shutting down");
+                std::process::exit(0);
             }
         }
     }

src/server.rs

@@ -597,24 +597,30 @@ impl LanguageServer for Backend {
 
         // Run on a blocking thread so the async runtime stays free to
         // flush progress notifications to the client.
+        //
+        // Wrapped in tokio::spawn for cancellation safety (see references handler).
         let backend = self.clone_for_blocking();
         let uri_clone = uri.clone();
-        let result = tokio::task::spawn_blocking(move || {
-            backend.handle_with_position(
-                "goto_implementation",
-                &uri_clone,
-                position,
-                |content, pos| {
-                    backend
-                        .resolve_implementation(&uri_clone, content, pos)
-                        .map(|locs| {
-                            locs.into_iter()
-                                .map(|l| backend.translate_location(l))
-                                .collect()
-                        })
-                        .and_then(wrap_locations)
-                },
-            )
+        let result = tokio::spawn(async move {
+            tokio::task::spawn_blocking(move || {
+                backend.handle_with_position(
+                    "goto_implementation",
+                    &uri_clone,
+                    position,
+                    |content, pos| {
+                        backend
+                            .resolve_implementation(&uri_clone, content, pos)
+                            .map(|locs| {
+                                locs.into_iter()
+                                    .map(|l| backend.translate_location(l))
+                                    .collect()
+                            })
+                            .and_then(wrap_locations)
+                    },
+                )
+            })
+            .await
+            .unwrap_or(Ok(None))
         })
         .await
         .unwrap_or(Ok(None));
@@ -720,18 +726,29 @@ impl LanguageServer for Backend {
 
         // Run on a blocking thread so the async runtime stays free to
         // flush progress notifications to the client.
+        //
+        // We wrap spawn_blocking inside tokio::spawn so the blocking
+        // task is always awaited to completion even if tower-lsp
+        // cancels this handler future via $/cancelRequest.  Without
+        // this wrapper, dropping the handler future detaches the
+        // spawn_blocking JoinHandle, and tower-lsp 0.20 may corrupt
+        // its internal state when the orphaned task completes.
         let backend = self.clone_for_blocking();
         let uri_clone = uri.clone();
-        let result = tokio::task::spawn_blocking(move || {
-            backend.handle_with_position("references", &uri_clone, position, |content, pos| {
-                backend
-                    .find_references(&uri_clone, content, pos, include_declaration)
-                    .map(|locs| {
-                        locs.into_iter()
-                            .map(|l| backend.translate_location(l))
-                            .collect()
-                    })
+        let result = tokio::spawn(async move {
+            tokio::task::spawn_blocking(move || {
+                backend.handle_with_position("references", &uri_clone, position, |content, pos| {
+                    backend
+                        .find_references(&uri_clone, content, pos, include_declaration)
+                        .map(|locs| {
+                            locs.into_iter()
+                                .map(|l| backend.translate_location(l))
+                                .collect()
+                        })
+                })
             })
+            .await
+            .unwrap_or(Ok(None))
         })
         .await
         .unwrap_or(Ok(None));
@@ -1055,9 +1072,14 @@ impl LanguageServer for Backend {
                 .await;
         }
 
-        let result = tokio::task::spawn_blocking(move || backend.subtypes_impl(&item))
-            .await
-            .unwrap_or(None);
+        // Wrapped in tokio::spawn for cancellation safety (see references handler).
+        let result = tokio::spawn(async move {
+            tokio::task::spawn_blocking(move || backend.subtypes_impl(&item))
+                .await
+                .unwrap_or(None)
+        })
+        .await
+        .unwrap_or(None);
 
         if let Some(ref tok) = token {
             self.progress_end(tok, Some("Done".to_string())).await;

src/util.rs

@@ -1719,11 +1719,21 @@ impl Backend {
         let params = WorkDoneProgressCreateParams {
             token: token.clone(),
         };
-        client
-            .send_request::<WorkDoneProgressCreate>(params)
-            .await
-            .ok()?;
-        Some(token)
+        // Use a timeout to avoid deadlocking the service loop.
+        // progress_create is a server-to-client request; if the
+        // client is busy (e.g. flooding didClose/hover), awaiting
+        // indefinitely could block the handler and starve other
+        // requests.  Progress reporting is best-effort, so timing
+        // out is harmless.
+        match tokio::time::timeout(
+            std::time::Duration::from_secs(2),
+            client.send_request::<WorkDoneProgressCreate>(params),
+        )
+        .await
+        {
+            Ok(Ok(())) => Some(token),
+            _ => None,
+        }
     }
 
     /// Send a `WorkDoneProgressBegin` notification for the given token.