From 25b784f00d40e20714709bf61e09046d4b9c8fd8 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Fri, 21 May 2021 07:35:37 +1000 Subject: [PATCH] chore(lsp): provide test for lsp deadlock issue (#10679) Resolves: #10587 --- .github/workflows/ci.yml | 2 +- cli/tests/integration_tests_lsp.rs | 148 +++++++++++++++- .../lsp/code_action_params_deadlock.json | 38 +++++ test_util/src/lsp.rs | 159 +++++++++++++----- 4 files changed, 301 insertions(+), 46 deletions(-) create mode 100644 cli/tests/lsp/code_action_params_deadlock.json diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7ac23cab0f..406d1bf2e8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -210,7 +210,7 @@ jobs: path: | ./target key: | - dummy # Cache never be created for this key. + s0mth1ng_rand0m # Cache never be created for this key. restore-keys: | a-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ hashFiles('Cargo.lock') }}- diff --git a/cli/tests/integration_tests_lsp.rs b/cli/tests/integration_tests_lsp.rs index a6d4f2d297..76041c502d 100644 --- a/cli/tests/integration_tests_lsp.rs +++ b/cli/tests/integration_tests_lsp.rs @@ -737,7 +737,7 @@ fn lsp_large_doc_changes() { .unwrap(); client .write_notification( - "textDocument/didChagne", + "textDocument/didChange", json!({ "textDocument": { "uri": "file:///a/file.ts", @@ -1420,6 +1420,152 @@ fn lsp_code_actions_deno_cache() { shutdown(&mut client); } +#[test] +fn lsp_code_actions_deadlock() { + let mut client = init("initialize_params.json"); + client + .write_notification( + "textDocument/didOpen", + load_fixture("did_open_params_large.json"), + ) + .unwrap(); + let (id, method, _) = client.read_request::().unwrap(); + assert_eq!(method, "workspace/configuration"); + client + .write_response(id, json!({ "enable": true })) + .unwrap(); + let (maybe_res, maybe_err) = client + .write_request::<_, _, Value>( + "textDocument/semanticTokens/full", + json!({ + "textDocument": { + "uri": "file:///a/file.ts" + } + }), + ) + .unwrap(); + assert!(maybe_err.is_none()); + assert!(maybe_res.is_some()); + for _ in 0..3 { + let (method, _) = client.read_notification::().unwrap(); + assert_eq!(method, "textDocument/publishDiagnostics"); + } + client + .write_notification( + "textDocument/didChange", + json!({ + "textDocument": { + "uri": "file:///a/file.ts", + "version": 2 + }, + "contentChanges": [ + { + "range": { + "start": { + "line": 444, + "character": 11 + }, + "end": { + "line": 444, + "character": 14 + } + }, + "text": "+++" + } + ] + }), + ) + .unwrap(); + client + .write_notification( + "textDocument/didChange", + json!({ + "textDocument": { + "uri": "file:///a/file.ts", + "version": 2 + }, + "contentChanges": [ + { + "range": { + "start": { + "line": 445, + "character": 4 + }, + "end": { + "line": 445, + "character": 4 + } + }, + "text": "// " + } + ] + }), + ) + .unwrap(); + client + .write_notification( + "textDocument/didChange", + json!({ + "textDocument": { + "uri": "file:///a/file.ts", + "version": 2 + }, + "contentChanges": [ + { + "range": { + "start": { + "line": 477, + "character": 4 + }, + "end": { + "line": 477, + "character": 9 + } + }, + "text": "error" + } + ] + }), + ) + .unwrap(); + // diagnostics only trigger after changes have elapsed in a separate thread, + // so we need to delay the next messages a little bit to attempt to create a + // potential for a deadlock with the codeAction + std::thread::sleep(std::time::Duration::from_millis(50)); + let (maybe_res, maybe_err) = client + .write_request::<_, _, Value>( + "textDocument/hover", + json!({ + "textDocument": { + "uri": "file:///a/file.ts", + }, + "position": { + "line": 609, + "character": 33, + } + }), + ) + .unwrap(); + assert!(maybe_err.is_none()); + assert!(maybe_res.is_some()); + let (maybe_res, maybe_err) = client + .write_request::<_, _, Value>( + "textDocument/codeAction", + load_fixture("code_action_params_deadlock.json"), + ) + .unwrap(); + assert!(maybe_err.is_none()); + assert!(maybe_res.is_some()); + + for _ in 0..3 { + let (method, _) = client.read_notification::().unwrap(); + assert_eq!(method, "textDocument/publishDiagnostics"); + } + + assert!(client.queue_is_empty()); + shutdown(&mut client); +} + #[test] fn lsp_completions() { let mut client = init("initialize_params.json"); diff --git a/cli/tests/lsp/code_action_params_deadlock.json b/cli/tests/lsp/code_action_params_deadlock.json new file mode 100644 index 0000000000..be0e317e1d --- /dev/null +++ b/cli/tests/lsp/code_action_params_deadlock.json @@ -0,0 +1,38 @@ +{ + "textDocument": { + "uri": "file:///a/file.ts" + }, + "range": { + "start": { + "line": 441, + "character": 33 + }, + "end": { + "line": 441, + "character": 42 + } + }, + "context": { + "diagnostics": [ + { + "range": { + "start": { + "line": 441, + "character": 33 + }, + "end": { + "line": 441, + "character": 42 + } + }, + "severity": 1, + "code": 7031, + "source": "deno-ts", + "message": "Binding element 'debugFlag' implicitly has an 'any' type." + } + ], + "only": [ + "quickfix" + ] + } +} diff --git a/test_util/src/lsp.rs b/test_util/src/lsp.rs index 52099ebe3f..7b9fc59650 100644 --- a/test_util/src/lsp.rs +++ b/test_util/src/lsp.rs @@ -2,6 +2,7 @@ use super::new_deno_dir; +use anyhow::Result; use lazy_static::lazy_static; use regex::Regex; use serde::de; @@ -9,6 +10,7 @@ use serde::Deserialize; use serde::Serialize; use serde_json::json; use serde_json::Value; +use std::collections::VecDeque; use std::io; use std::io::Write; use std::path::Path; @@ -61,7 +63,7 @@ impl<'a> From<&'a [u8]> for LspMessage { } } -fn read_message(reader: &mut R) -> Result, anyhow::Error> +fn read_message(reader: &mut R) -> Result> where R: io::Read + io::BufRead, { @@ -86,8 +88,12 @@ where } pub struct LspClient { - reader: io::BufReader, child: Child, + reader: io::BufReader, + /// Used to hold pending messages that have come out of the expected sequence + /// by the harness user which will be sent first when trying to consume a + /// message before attempting to read a new message. + msg_queue: VecDeque, request_id: u64, start: Instant, writer: io::BufWriter, @@ -106,8 +112,51 @@ impl Drop for LspClient { } } +fn notification_result( + method: String, + maybe_params: Option, +) -> Result<(String, Option)> +where + R: de::DeserializeOwned, +{ + let maybe_params = match maybe_params { + Some(params) => Some(serde_json::from_value(params)?), + None => None, + }; + Ok((method, maybe_params)) +} + +fn request_result( + id: u64, + method: String, + maybe_params: Option, +) -> Result<(u64, String, Option)> +where + R: de::DeserializeOwned, +{ + let maybe_params = match maybe_params { + Some(params) => Some(serde_json::from_value(params)?), + None => None, + }; + Ok((id, method, maybe_params)) +} + +fn response_result( + maybe_result: Option, + maybe_error: Option, +) -> Result<(Option, Option)> +where + R: de::DeserializeOwned, +{ + let maybe_result = match maybe_result { + Some(result) => Some(serde_json::from_value(result)?), + None => None, + }; + Ok((maybe_result, maybe_error)) +} + impl LspClient { - pub fn new(deno_exe: &Path) -> Result { + pub fn new(deno_exe: &Path) -> Result { let deno_dir = new_deno_dir(); let mut child = Command::new(deno_exe) .env("DENO_DIR", deno_dir.path()) @@ -125,6 +174,7 @@ impl LspClient { Ok(Self { child, + msg_queue: VecDeque::new(), reader, request_id: 1, start: Instant::now(), @@ -136,49 +186,79 @@ impl LspClient { self.start.elapsed() } - fn read(&mut self) -> Result { + pub fn queue_is_empty(&self) -> bool { + self.msg_queue.is_empty() + } + + pub fn queue_len(&self) -> usize { + self.msg_queue.len() + } + + fn read(&mut self) -> Result { let msg_buf = read_message(&mut self.reader)?; let msg = LspMessage::from(msg_buf.as_slice()); Ok(msg) } - pub fn read_notification( - &mut self, - ) -> Result<(String, Option), anyhow::Error> + pub fn read_notification(&mut self) -> Result<(String, Option)> where R: de::DeserializeOwned, { - loop { - if let LspMessage::Notification(method, maybe_params) = self.read()? { - if let Some(p) = maybe_params { - let params = serde_json::from_value(p)?; - return Ok((method, Some(params))); - } else { - return Ok((method, None)); + if !self.msg_queue.is_empty() { + let mut msg_queue = VecDeque::new(); + loop { + match self.msg_queue.pop_front() { + Some(LspMessage::Notification(method, maybe_params)) => { + return notification_result(method, maybe_params) + } + Some(msg) => msg_queue.push_back(msg), + _ => break, } } + self.msg_queue = msg_queue; + } + + loop { + match self.read() { + Ok(LspMessage::Notification(method, maybe_params)) => { + return notification_result(method, maybe_params) + } + Ok(msg) => self.msg_queue.push_back(msg), + Err(err) => return Err(err), + } } } - pub fn read_request( - &mut self, - ) -> Result<(u64, String, Option), anyhow::Error> + pub fn read_request(&mut self) -> Result<(u64, String, Option)> where R: de::DeserializeOwned, { - loop { - if let LspMessage::Request(id, method, maybe_params) = self.read()? { - if let Some(p) = maybe_params { - let params = serde_json::from_value(p)?; - return Ok((id, method, Some(params))); - } else { - return Ok((id, method, None)); + if !self.msg_queue.is_empty() { + let mut msg_queue = VecDeque::new(); + loop { + match self.msg_queue.pop_front() { + Some(LspMessage::Request(id, method, maybe_params)) => { + return request_result(id, method, maybe_params) + } + Some(msg) => msg_queue.push_back(msg), + _ => break, } } + self.msg_queue = msg_queue; + } + + loop { + match self.read() { + Ok(LspMessage::Request(id, method, maybe_params)) => { + return request_result(id, method, maybe_params) + } + Ok(msg) => self.msg_queue.push_back(msg), + Err(err) => return Err(err), + } } } - fn write(&mut self, value: Value) -> Result<(), anyhow::Error> { + fn write(&mut self, value: Value) -> Result<()> { let value_str = value.to_string(); let msg = format!( "Content-Length: {}\r\n\r\n{}", @@ -194,7 +274,7 @@ impl LspClient { &mut self, method: S, params: V, - ) -> Result<(Option, Option), anyhow::Error> + ) -> Result<(Option, Option)> where S: AsRef, V: Serialize, @@ -209,24 +289,19 @@ impl LspClient { self.write(value)?; loop { - if let LspMessage::Response(id, result, error) = self.read()? { - assert_eq!(id, self.request_id); - self.request_id += 1; - if let Some(r) = result { - let result = serde_json::from_value(r)?; - return Ok((Some(result), error)); - } else { - return Ok((None, error)); + match self.read() { + Ok(LspMessage::Response(id, maybe_result, maybe_error)) => { + assert_eq!(id, self.request_id); + self.request_id += 1; + return response_result(maybe_result, maybe_error); } + Ok(msg) => self.msg_queue.push_back(msg), + Err(err) => return Err(err), } } } - pub fn write_response( - &mut self, - id: u64, - result: V, - ) -> Result<(), anyhow::Error> + pub fn write_response(&mut self, id: u64, result: V) -> Result<()> where V: Serialize, { @@ -238,11 +313,7 @@ impl LspClient { self.write(value) } - pub fn write_notification( - &mut self, - method: S, - params: V, - ) -> Result<(), anyhow::Error> + pub fn write_notification(&mut self, method: S, params: V) -> Result<()> where S: AsRef, V: Serialize,