From 05aa5b854b230c2f6ba182c0f2c94f3de9a47204 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Thu, 23 Jan 2020 09:26:08 +0100 Subject: Remove RWLock from check watcher. @matklad mentioned this might be a good idea. So the general idea is that we don't really need the lock, as we can just clone the check watcher state when creating a snapshot. We can then use `Arc::get_mut` to get mutable access to the state from `WorldState` when needed. Running with this it seems to improve responsiveness a bit while cargo is running, but I have no hard numbers to prove it. In any case, a serialization point less is always better when we're trying to be responsive. --- crates/ra_cargo_watch/src/lib.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'crates/ra_cargo_watch/src/lib.rs') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index 7f4c9280c..bbe634603 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -7,7 +7,6 @@ use lsp_types::{ Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin, WorkDoneProgressEnd, WorkDoneProgressReport, }; -use parking_lot::RwLock; use std::{ collections::HashMap, path::PathBuf, @@ -38,7 +37,7 @@ pub struct CheckOptions { #[derive(Debug)] pub struct CheckWatcher { pub task_recv: Receiver, - pub state: Arc>, + pub state: Arc, cmd_send: Option>, handle: Option>, } @@ -46,7 +45,7 @@ pub struct CheckWatcher { impl CheckWatcher { pub fn new(options: &CheckOptions, workspace_root: PathBuf) -> CheckWatcher { let options = options.clone(); - let state = Arc::new(RwLock::new(CheckState::new())); + let state = Arc::new(CheckState::new()); let (task_send, task_recv) = unbounded::(); let (cmd_send, cmd_recv) = unbounded::(); @@ -59,7 +58,7 @@ impl CheckWatcher { /// Returns a CheckWatcher that doesn't actually do anything pub fn dummy() -> CheckWatcher { - let state = Arc::new(RwLock::new(CheckState::new())); + let state = Arc::new(CheckState::new()); CheckWatcher { task_recv: never(), cmd_send: None, handle: None, state } } @@ -87,7 +86,7 @@ impl std::ops::Drop for CheckWatcher { } } -#[derive(Debug)] +#[derive(Clone, Debug)] pub struct CheckState { diagnostic_collection: HashMap>, suggested_fix_collection: HashMap>, -- cgit v1.2.3 From c34571c19e727654139a27c5b9d656485fb516f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Tue, 28 Jan 2020 01:27:43 +0200 Subject: Buffer reads from cargo check's stdout --- crates/ra_cargo_watch/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'crates/ra_cargo_watch/src/lib.rs') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index bbe634603..e7b700c10 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -9,6 +9,7 @@ use lsp_types::{ }; use std::{ collections::HashMap, + io::BufReader, path::PathBuf, process::{Command, Stdio}, sync::Arc, @@ -347,7 +348,9 @@ impl WatchThread { // which will break out of the loop, and continue the shutdown let _ = message_send.send(CheckEvent::Begin); - for message in cargo_metadata::parse_messages(command.stdout.take().unwrap()) { + for message in + cargo_metadata::parse_messages(BufReader::new(command.stdout.take().unwrap())) + { let message = match message { Ok(message) => message, Err(err) => { -- cgit v1.2.3 From fdc04ef92066fcf3936664d5f5c80d6f088aac29 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Tue, 28 Jan 2020 14:33:52 +0100 Subject: Don't do check progress update for fresh crates --- crates/ra_cargo_watch/src/lib.rs | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'crates/ra_cargo_watch/src/lib.rs') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index e7b700c10..934379dcf 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -359,6 +359,14 @@ impl WatchThread { } }; + // Skip certain kinds of messages to only spend time on what's useful + match &message { + Message::CompilerArtifact(artifact) if artifact.fresh => continue, + Message::BuildScriptExecuted(_) => continue, + Message::Unknown => continue, + _ => {} + } + match message_send.send(CheckEvent::Msg(message)) { Ok(()) => {} Err(_err) => { -- cgit v1.2.3 From 35025f097532b3f927f802a4130f7cd8a854f134 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Tue, 28 Jan 2020 14:48:50 +0100 Subject: Modify ordering of drops in check watcher to only ever have one cargo Due to the way drops are ordered when assigning to a mutable variable we were launching a new cargo sub-process before letting the old one quite. By explicitly replacing the original watcher with a dummy first, we ensure it is dropped and the process is completed, before we start the new process. --- crates/ra_cargo_watch/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'crates/ra_cargo_watch/src/lib.rs') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index e7b700c10..1a35151ee 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -216,8 +216,10 @@ impl CheckWatcherThread { self.last_update_req.take(); task_send.send(CheckTask::ClearDiagnostics).unwrap(); - // By replacing the watcher, we drop the previous one which - // causes it to shut down automatically. + // Replace with a dummy watcher first so we drop the original and wait for completion + std::mem::replace(&mut self.watcher, WatchThread::dummy()); + + // Then create the actual new watcher self.watcher = WatchThread::new(&self.options, &self.workspace_root); } } -- cgit v1.2.3 From 8ffbe86dfd24ffcc11ec37bceca9102260d59db2 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Wed, 29 Jan 2020 13:40:27 +0100 Subject: Parse cargo output a line at a time. We previously used serde's stream deserializer to read json blobs from the cargo output. It has an issue though: If the deserializer encounters invalid input, it gets stuck reporting the same error again and again because it is unable to foward over the input until it reaches a new valid object. Reading a line at a time and manually deserializing fixes this issue, because cargo makes sure to only outpu one json blob per line, so should we encounter invalid input, we can just skip a line and continue. The main reason this would happen is stray printf-debugging in procedural macros, so we still report that an error occured, but we handle it gracefully now. Fixes #2935 --- crates/ra_cargo_watch/src/lib.rs | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) (limited to 'crates/ra_cargo_watch/src/lib.rs') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index 9af9c347d..e015692fa 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -9,7 +9,7 @@ use lsp_types::{ }; use std::{ collections::HashMap, - io::BufReader, + io::{BufRead, BufReader}, path::PathBuf, process::{Command, Stdio}, sync::Arc, @@ -350,13 +350,29 @@ impl WatchThread { // which will break out of the loop, and continue the shutdown let _ = message_send.send(CheckEvent::Begin); - for message in - cargo_metadata::parse_messages(BufReader::new(command.stdout.take().unwrap())) - { + // We manually read a line at a time, instead of using serde's + // stream deserializers, because the deserializer cannot recover + // from an error, resulting in it getting stuck, because we try to + // be resillient against failures. + // + // Because cargo only outputs one JSON object per line, we can + // simply skip a line if it doesn't parse, which just ignores any + // erroneus output. + let stdout = BufReader::new(command.stdout.take().unwrap()); + for line in stdout.lines() { + let line = match line { + Ok(line) => line, + Err(err) => { + log::error!("Couldn't read line from cargo: {:?}", err); + continue; + } + }; + + let message = serde_json::from_str::(&line); let message = match message { Ok(message) => message, Err(err) => { - log::error!("Invalid json from cargo check, ignoring: {}", err); + log::error!("Invalid json from cargo check, ignoring ({}): {} ", err, line); continue; } }; -- cgit v1.2.3 From 4ec5f6e25850b3064b258739eabefdeb8a4bd1b5 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Wed, 29 Jan 2020 13:51:20 +0100 Subject: Change error output to make a bit more sense --- crates/ra_cargo_watch/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'crates/ra_cargo_watch/src/lib.rs') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index e015692fa..ea7ddc86b 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -363,7 +363,7 @@ impl WatchThread { let line = match line { Ok(line) => line, Err(err) => { - log::error!("Couldn't read line from cargo: {:?}", err); + log::error!("Couldn't read line from cargo: {}", err); continue; } }; @@ -372,7 +372,11 @@ impl WatchThread { let message = match message { Ok(message) => message, Err(err) => { - log::error!("Invalid json from cargo check, ignoring ({}): {} ", err, line); + log::error!( + "Invalid json from cargo check, ignoring ({}): {:?} ", + err, + line + ); continue; } }; -- cgit v1.2.3 From 5559d6b8a88750a78d91ff9c6cc1a1a736d22042 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 1 Feb 2020 16:25:44 +0100 Subject: Prevent child cargo process from messing with our stdin By default, `spawn` inherits stderr/stdout/stderr of the parent process, and so, if child, for example does fcntl(O_NONBLOCK), weird stuff happens to us. Closes https://github.com/rust-analyzer/lsp-server/pull/10 --- crates/ra_cargo_watch/src/lib.rs | 1 + 1 file changed, 1 insertion(+) (limited to 'crates/ra_cargo_watch/src/lib.rs') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index ea7ddc86b..a718a5e52 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -343,6 +343,7 @@ impl WatchThread { .args(&args) .stdout(Stdio::piped()) .stderr(Stdio::null()) + .stdin(Stdio::null()) .spawn() .expect("couldn't launch cargo"); -- cgit v1.2.3 From 790788d5f4013d8d92f110bc12a581d18cf4b6ae Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 31 Jan 2020 19:23:25 +0100 Subject: Rework how we send diagnostics to client. The previous way of sending from the thread pool suffered from stale diagnostics due to being canceled before we could clear the old ones. The key change is moving to sending diagnostics from the main loop thread, but doing all the hard work in the thread pool. This should provide the best of both worlds, with little to no of the downsides. This should hopefully fix a lot of issues, but we'll need testing in each individual issue to be sure. --- crates/ra_cargo_watch/src/lib.rs | 110 ++++++--------------------------------- 1 file changed, 17 insertions(+), 93 deletions(-) (limited to 'crates/ra_cargo_watch/src/lib.rs') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index a718a5e52..f07c34549 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -4,22 +4,20 @@ use cargo_metadata::Message; use crossbeam_channel::{never, select, unbounded, Receiver, RecvError, Sender}; use lsp_types::{ - Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin, WorkDoneProgressEnd, - WorkDoneProgressReport, + CodeAction, CodeActionOrCommand, Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin, + WorkDoneProgressEnd, WorkDoneProgressReport, }; use std::{ - collections::HashMap, io::{BufRead, BufReader}, path::PathBuf, process::{Command, Stdio}, - sync::Arc, thread::JoinHandle, time::Instant, }; mod conv; -use crate::conv::{map_rust_diagnostic_to_lsp, MappedRustDiagnostic, SuggestedFix}; +use crate::conv::{map_rust_diagnostic_to_lsp, MappedRustDiagnostic}; pub use crate::conv::url_from_path_with_drive_lowercasing; @@ -38,7 +36,6 @@ pub struct CheckOptions { #[derive(Debug)] pub struct CheckWatcher { pub task_recv: Receiver, - pub state: Arc, cmd_send: Option>, handle: Option>, } @@ -46,7 +43,6 @@ pub struct CheckWatcher { impl CheckWatcher { pub fn new(options: &CheckOptions, workspace_root: PathBuf) -> CheckWatcher { let options = options.clone(); - let state = Arc::new(CheckState::new()); let (task_send, task_recv) = unbounded::(); let (cmd_send, cmd_recv) = unbounded::(); @@ -54,13 +50,12 @@ impl CheckWatcher { let mut check = CheckWatcherThread::new(options, workspace_root); check.run(&task_send, &cmd_recv); }); - CheckWatcher { task_recv, cmd_send: Some(cmd_send), handle: Some(handle), state } + CheckWatcher { task_recv, cmd_send: Some(cmd_send), handle: Some(handle) } } /// Returns a CheckWatcher that doesn't actually do anything pub fn dummy() -> CheckWatcher { - let state = Arc::new(CheckState::new()); - CheckWatcher { task_recv: never(), cmd_send: None, handle: None, state } + CheckWatcher { task_recv: never(), cmd_send: None, handle: None } } /// Schedule a re-start of the cargo check worker. @@ -87,84 +82,13 @@ impl std::ops::Drop for CheckWatcher { } } -#[derive(Clone, Debug)] -pub struct CheckState { - diagnostic_collection: HashMap>, - suggested_fix_collection: HashMap>, -} - -impl CheckState { - fn new() -> CheckState { - CheckState { - diagnostic_collection: HashMap::new(), - suggested_fix_collection: HashMap::new(), - } - } - - /// Clear the cached diagnostics, and schedule updating diagnostics by the - /// server, to clear stale results. - pub fn clear(&mut self) -> Vec { - let cleared_files: Vec = self.diagnostic_collection.keys().cloned().collect(); - self.diagnostic_collection.clear(); - self.suggested_fix_collection.clear(); - cleared_files - } - - pub fn diagnostics_for(&self, uri: &Url) -> Option<&[Diagnostic]> { - self.diagnostic_collection.get(uri).map(|d| d.as_slice()) - } - - pub fn fixes_for(&self, uri: &Url) -> Option<&[SuggestedFix]> { - self.suggested_fix_collection.get(uri).map(|d| d.as_slice()) - } - - pub fn add_diagnostic_with_fixes(&mut self, file_uri: Url, diagnostic: DiagnosticWithFixes) { - for fix in diagnostic.suggested_fixes { - self.add_suggested_fix_for_diagnostic(fix, &diagnostic.diagnostic); - } - self.add_diagnostic(file_uri, diagnostic.diagnostic); - } - - fn add_diagnostic(&mut self, file_uri: Url, diagnostic: Diagnostic) { - let diagnostics = self.diagnostic_collection.entry(file_uri).or_default(); - - // If we're building multiple targets it's possible we've already seen this diagnostic - let is_duplicate = diagnostics.iter().any(|d| are_diagnostics_equal(d, &diagnostic)); - if is_duplicate { - return; - } - - diagnostics.push(diagnostic); - } - - fn add_suggested_fix_for_diagnostic( - &mut self, - mut suggested_fix: SuggestedFix, - diagnostic: &Diagnostic, - ) { - let file_uri = suggested_fix.location.uri.clone(); - let file_suggestions = self.suggested_fix_collection.entry(file_uri).or_default(); - - let existing_suggestion: Option<&mut SuggestedFix> = - file_suggestions.iter_mut().find(|s| s == &&suggested_fix); - if let Some(existing_suggestion) = existing_suggestion { - // The existing suggestion also applies to this new diagnostic - existing_suggestion.diagnostics.push(diagnostic.clone()); - } else { - // We haven't seen this suggestion before - suggested_fix.diagnostics.push(diagnostic.clone()); - file_suggestions.push(suggested_fix); - } - } -} - #[derive(Debug)] pub enum CheckTask { /// Request a clearing of all cached diagnostics from the check watcher ClearDiagnostics, /// Request adding a diagnostic with fixes included to a file - AddDiagnostic(Url, DiagnosticWithFixes), + AddDiagnostic { url: Url, diagnostic: Diagnostic, fixes: Vec }, /// Request check progress notification to client Status(WorkDoneProgress), @@ -279,10 +203,17 @@ impl CheckWatcherThread { None => return, }; - let MappedRustDiagnostic { location, diagnostic, suggested_fixes } = map_result; + let MappedRustDiagnostic { location, diagnostic, fixes } = map_result; + let fixes = fixes + .into_iter() + .map(|fix| { + CodeAction { diagnostics: Some(vec![diagnostic.clone()]), ..fix }.into() + }) + .collect(); - let diagnostic = DiagnosticWithFixes { diagnostic, suggested_fixes }; - task_send.send(CheckTask::AddDiagnostic(location.uri, diagnostic)).unwrap(); + task_send + .send(CheckTask::AddDiagnostic { url: location.uri, diagnostic, fixes }) + .unwrap(); } CheckEvent::Msg(Message::BuildScriptExecuted(_msg)) => {} @@ -294,7 +225,7 @@ impl CheckWatcherThread { #[derive(Debug)] pub struct DiagnosticWithFixes { diagnostic: Diagnostic, - suggested_fixes: Vec, + fixes: Vec, } /// WatchThread exists to wrap around the communication needed to be able to @@ -429,10 +360,3 @@ impl std::ops::Drop for WatchThread { } } } - -fn are_diagnostics_equal(left: &Diagnostic, right: &Diagnostic) -> bool { - left.source == right.source - && left.severity == right.severity - && left.range == right.range - && left.message == right.message -} -- cgit v1.2.3