Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

lsp: Correctly serialize errors for LSP requests + improve handling of unrecognized methods (#35738)

Browse source 
We used to not respond at all to requests that we didn't have a handler
for, which is yuck. It may have left the language server waiting for the
response for no good reason. The other (worse) finding is that we did
not have a full definition of an Error type for LSP, which made it so
that a spec-compliant language server would fail to deserialize our
response (with an error). This then could lead to all sorts of
funkiness, including hangs and crashes on the language server's part.

Co-authored-by: Lukas <lukas@zed.dev>
Co-authored-by: Remco Smits <djsmits12@gmail.com>

Co-authored-by: Anthony Eid <hello@anthonyeid.me>

Closes #ISSUE

Release Notes:

- Improved reporting of errors to language servers, which should improve
the stability of LSPs ran by Zed.

---------

Co-authored-by: Lukas <lukas@zed.dev>
Co-authored-by: Remco Smits <djsmits12@gmail.com>
Co-authored-by: Anthony Eid <hello@anthonyeid.me>
This commit is contained in:
Piotr Osiewicz 2025-08-06 20:27:48 +02:00 • committed by Joseph T. Lyons
parent 6dbd498cea
commit f31762f19b
2 changed files with 57 additions and 24 deletions
Download patch file Download diff file Expand all files Collapse all files

11
crates/lsp/src/input_handler.rs
View file

@ -13,14 +13,15 @@ use parking_lot::Mutex;
use smol::io::BufReader; use smol::io::BufReader;
use crate::{ use crate::{
AnyNotification, AnyResponse, CONTENT_LEN_HEADER, IoHandler, IoKind, RequestId, ResponseHandler, AnyResponse, CONTENT_LEN_HEADER, IoHandler, IoKind, NotificationOrRequest, RequestId,
ResponseHandler,
}; };
const HEADER_DELIMITER: &[u8; 4] = b"\r\n\r\n"; const HEADER_DELIMITER: &[u8; 4] = b"\r\n\r\n";
/// Handler for stdout of language server. /// Handler for stdout of language server.
pub struct LspStdoutHandler { pub struct LspStdoutHandler {
pub(super) loop_handle: Task<Result<()>>, pub(super) loop_handle: Task<Result<()>>,
pub(super) notifications_channel: UnboundedReceiver<AnyNotification>, pub(super) incoming_messages: UnboundedReceiver<NotificationOrRequest>,
} }
async fn read_headers<Stdout>(reader: &mut BufReader<Stdout>, buffer: &mut Vec<u8>) -> Result<()> async fn read_headers<Stdout>(reader: &mut BufReader<Stdout>, buffer: &mut Vec<u8>) -> Result<()>
@ -54,13 +55,13 @@ impl LspStdoutHandler {
let loop_handle = cx.spawn(Self::handler(stdout, tx, response_handlers, io_handlers)); let loop_handle = cx.spawn(Self::handler(stdout, tx, response_handlers, io_handlers));
Self { Self {
loop_handle, loop_handle,
notifications_channel, incoming_messages: notifications_channel,
} }
} }
async fn handler<Input>( async fn handler<Input>(
stdout: Input, stdout: Input,
notifications_sender: UnboundedSender<AnyNotification>, notifications_sender: UnboundedSender<NotificationOrRequest>,
response_handlers: Arc<Mutex<Option<HashMap<RequestId, ResponseHandler>>>>, response_handlers: Arc<Mutex<Option<HashMap<RequestId, ResponseHandler>>>>,
io_handlers: Arc<Mutex<HashMap<i32, IoHandler>>>, io_handlers: Arc<Mutex<HashMap<i32, IoHandler>>>,
) -> anyhow::Result<()> ) -> anyhow::Result<()>
@ -96,7 +97,7 @@ impl LspStdoutHandler {
} }
} }
if let Ok(msg) = serde_json::from_slice::<AnyNotification>(&buffer) { if let Ok(msg) = serde_json::from_slice::<NotificationOrRequest>(&buffer) {
notifications_sender.unbounded_send(msg)?; notifications_sender.unbounded_send(msg)?;
} else if let Ok(AnyResponse { } else if let Ok(AnyResponse {
id, error, result, .. id, error, result, ..

70
crates/lsp/src/lsp.rs
View file

@ -242,7 +242,7 @@ struct Notification<'a, T> {
/// Language server RPC notification message before it is deserialized into a concrete type. /// Language server RPC notification message before it is deserialized into a concrete type.
#[derive(Debug, Clone, Deserialize)] #[derive(Debug, Clone, Deserialize)]
struct AnyNotification { struct NotificationOrRequest {
#[serde(default)] #[serde(default)]
id: Option<RequestId>, id: Option<RequestId>,
method: String, method: String,
@ -252,7 +252,10 @@ struct AnyNotification {
#[derive(Debug, Serialize, Deserialize)] #[derive(Debug, Serialize, Deserialize)]
struct Error { struct Error {
code: i64,
message: String, message: String,
#[serde(default)]
data: Option<serde_json::Value>,
} }
pub trait LspRequestFuture<O>: Future<Output = ConnectionResult<O>> { pub trait LspRequestFuture<O>: Future<Output = ConnectionResult<O>> {
@ -364,6 +367,7 @@ impl LanguageServer {
notification.method, notification.method,
serde_json::to_string_pretty(&notification.params).unwrap(), serde_json::to_string_pretty(&notification.params).unwrap(),
); );
false
}, },
); );
@ -389,7 +393,7 @@ impl LanguageServer {
Stdin: AsyncWrite + Unpin + Send + 'static, Stdin: AsyncWrite + Unpin + Send + 'static,
Stdout: AsyncRead + Unpin + Send + 'static, Stdout: AsyncRead + Unpin + Send + 'static,
Stderr: AsyncRead + Unpin + Send + 'static, Stderr: AsyncRead + Unpin + Send + 'static,
F: FnMut(AnyNotification) + 'static + Send + Sync + Clone, F: Fn(&NotificationOrRequest) -> bool + 'static + Send + Sync + Clone,
{ {
let (outbound_tx, outbound_rx) = channel::unbounded::<String>(); let (outbound_tx, outbound_rx) = channel::unbounded::<String>();
let (output_done_tx, output_done_rx) = barrier::channel(); let (output_done_tx, output_done_rx) = barrier::channel();
@ -400,14 +404,34 @@ impl LanguageServer {
let io_handlers = Arc::new(Mutex::new(HashMap::default())); let io_handlers = Arc::new(Mutex::new(HashMap::default()));
let stdout_input_task = cx.spawn({ let stdout_input_task = cx.spawn({
let on_unhandled_notification = on_unhandled_notification.clone(); let unhandled_notification_wrapper = {
let response_channel = outbound_tx.clone();
async move |msg: NotificationOrRequest| {
let did_handle = on_unhandled_notification(&msg);
if !did_handle && let Some(message_id) = msg.id {
let response = AnyResponse {
jsonrpc: JSON_RPC_VERSION,
id: message_id,
error: Some(Error {
code: -32601,
message: format!("Unrecognized method `{}`", msg.method),
data: None,
}),
result: None,
};
if let Ok(response) = serde_json::to_string(&response) {
response_channel.send(response).await.ok();
}
}
}
};
let notification_handlers = notification_handlers.clone(); let notification_handlers = notification_handlers.clone();
let response_handlers = response_handlers.clone(); let response_handlers = response_handlers.clone();
let io_handlers = io_handlers.clone(); let io_handlers = io_handlers.clone();
async move |cx| { async move |cx| {
Self::handle_input( Self::handle_incoming_messages(
stdout, stdout,
on_unhandled_notification, unhandled_notification_wrapper,
notification_handlers, notification_handlers,
response_handlers, response_handlers,
io_handlers, io_handlers,
@ -433,7 +457,7 @@ impl LanguageServer {
stdout.or(stderr) stdout.or(stderr)
}); });
let output_task = cx.background_spawn({ let output_task = cx.background_spawn({
Self::handle_output( Self::handle_outgoing_messages(
stdin, stdin,
outbound_rx, outbound_rx,
output_done_tx, output_done_tx,
@ -479,9 +503,9 @@ impl LanguageServer {
self.code_action_kinds.clone() self.code_action_kinds.clone()
} }
async fn handle_input<Stdout, F>( async fn handle_incoming_messages<Stdout>(
stdout: Stdout, stdout: Stdout,
mut on_unhandled_notification: F, on_unhandled_notification: impl AsyncFn(NotificationOrRequest) + 'static + Send,
notification_handlers: Arc<Mutex<HashMap<&'static str, NotificationHandler>>>, notification_handlers: Arc<Mutex<HashMap<&'static str, NotificationHandler>>>,
response_handlers: Arc<Mutex<Option<HashMap<RequestId, ResponseHandler>>>>, response_handlers: Arc<Mutex<Option<HashMap<RequestId, ResponseHandler>>>>,
io_handlers: Arc<Mutex<HashMap<i32, IoHandler>>>, io_handlers: Arc<Mutex<HashMap<i32, IoHandler>>>,
@ -489,7 +513,6 @@ impl LanguageServer {
) -> anyhow::Result<()> ) -> anyhow::Result<()>
where where
Stdout: AsyncRead + Unpin + Send + 'static, Stdout: AsyncRead + Unpin + Send + 'static,
F: FnMut(AnyNotification) + 'static + Send,
{ {
use smol::stream::StreamExt; use smol::stream::StreamExt;
let stdout = BufReader::new(stdout); let stdout = BufReader::new(stdout);
@ -506,15 +529,19 @@ impl LanguageServer {
cx.background_executor().clone(), cx.background_executor().clone(),
); );
while let Some(msg) = input_handler.notifications_channel.next().await { while let Some(msg) = input_handler.incoming_messages.next().await {
{ let unhandled_message = {
let mut notification_handlers = notification_handlers.lock(); let mut notification_handlers = notification_handlers.lock();
if let Some(handler) = notification_handlers.get_mut(msg.method.as_str()) { if let Some(handler) = notification_handlers.get_mut(msg.method.as_str()) {
handler(msg.id, msg.params.unwrap_or(Value::Null), cx); handler(msg.id, msg.params.unwrap_or(Value::Null), cx);
None
} else { } else {
drop(notification_handlers); Some(msg)
on_unhandled_notification(msg);
} }
};
if let Some(msg) = unhandled_message {
on_unhandled_notification(msg).await;
} }
// Don't starve the main thread when receiving lots of notifications at once. // Don't starve the main thread when receiving lots of notifications at once.
@ -558,7 +585,7 @@ impl LanguageServer {
} }
} }
async fn handle_output<Stdin>( async fn handle_outgoing_messages<Stdin>(
stdin: Stdin, stdin: Stdin,
outbound_rx: channel::Receiver<String>, outbound_rx: channel::Receiver<String>,
output_done_tx: barrier::Sender, output_done_tx: barrier::Sender,
@ -1036,7 +1063,9 @@ impl LanguageServer {
jsonrpc: JSON_RPC_VERSION, jsonrpc: JSON_RPC_VERSION,
id, id,
value: LspResult::Error(Some(Error { value: LspResult::Error(Some(Error {
code: lsp_types::error_codes::REQUEST_FAILED,
message: error.to_string(), message: error.to_string(),
data: None,
})), })),
}, },
}; };
@ -1057,7 +1086,9 @@ impl LanguageServer {
id, id,
result: None, result: None,
error: Some(Error { error: Some(Error {
code: -32700, // Parse error
message: error.to_string(), message: error.to_string(),
data: None,
}), }),
}; };
if let Some(response) = serde_json::to_string(&response).log_err() { if let Some(response) = serde_json::to_string(&response).log_err() {
@ -1559,7 +1590,7 @@ impl FakeLanguageServer {
root, root,
Some(workspace_folders.clone()), Some(workspace_folders.clone()),
cx, cx,
|_| {}, |_| false,
); );
server.process_name = process_name; server.process_name = process_name;
let fake = FakeLanguageServer { let fake = FakeLanguageServer {
@ -1582,9 +1613,10 @@ impl FakeLanguageServer {
notifications_tx notifications_tx
.try_send(( .try_send((
msg.method.to_string(), msg.method.to_string(),
msg.params.unwrap_or(Value::Null).to_string(), msg.params.as_ref().unwrap_or(&Value::Null).to_string(),
)) ))
.ok(); .ok();
true
}, },
); );
server.process_name = name.as_str().into(); server.process_name = name.as_str().into();
@ -1862,7 +1894,7 @@ mod tests {
#[gpui::test] #[gpui::test]
fn test_deserialize_string_digit_id() { fn test_deserialize_string_digit_id() {
let json = r#"{"jsonrpc":"2.0","id":"2","method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///Users/mph/Devel/personal/hello-scala/","section":"metals"}]}}"#; let json = r#"{"jsonrpc":"2.0","id":"2","method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///Users/mph/Devel/personal/hello-scala/","section":"metals"}]}}"#;
let notification = serde_json::from_str::<AnyNotification>(json) let notification = serde_json::from_str::<NotificationOrRequest>(json)
.expect("message with string id should be parsed"); .expect("message with string id should be parsed");
let expected_id = RequestId::Str("2".to_string()); let expected_id = RequestId::Str("2".to_string());
assert_eq!(notification.id, Some(expected_id)); assert_eq!(notification.id, Some(expected_id));
@ -1871,7 +1903,7 @@ mod tests {
#[gpui::test] #[gpui::test]
fn test_deserialize_string_id() { fn test_deserialize_string_id() {
let json = r#"{"jsonrpc":"2.0","id":"anythingAtAll","method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///Users/mph/Devel/personal/hello-scala/","section":"metals"}]}}"#; let json = r#"{"jsonrpc":"2.0","id":"anythingAtAll","method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///Users/mph/Devel/personal/hello-scala/","section":"metals"}]}}"#;
let notification = serde_json::from_str::<AnyNotification>(json) let notification = serde_json::from_str::<NotificationOrRequest>(json)
.expect("message with string id should be parsed"); .expect("message with string id should be parsed");
let expected_id = RequestId::Str("anythingAtAll".to_string()); let expected_id = RequestId::Str("anythingAtAll".to_string());
assert_eq!(notification.id, Some(expected_id)); assert_eq!(notification.id, Some(expected_id));
@ -1880,7 +1912,7 @@ mod tests {
#[gpui::test] #[gpui::test]
fn test_deserialize_int_id() { fn test_deserialize_int_id() {
let json = r#"{"jsonrpc":"2.0","id":2,"method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///Users/mph/Devel/personal/hello-scala/","section":"metals"}]}}"#; let json = r#"{"jsonrpc":"2.0","id":2,"method":"workspace/configuration","params":{"items":[{"scopeUri":"file:///Users/mph/Devel/personal/hello-scala/","section":"metals"}]}}"#;
let notification = serde_json::from_str::<AnyNotification>(json) let notification = serde_json::from_str::<NotificationOrRequest>(json)
.expect("message with string id should be parsed"); .expect("message with string id should be parsed");
let expected_id = RequestId::Int(2); let expected_id = RequestId::Int(2);
assert_eq!(notification.id, Some(expected_id)); assert_eq!(notification.id, Some(expected_id));