Skip to content

Commit 9e55192

Browse files
the-mikedavisfacebook-github-bot
authored andcommitted
Fix Drop for Server deadlock behavior (#75)
Summary: This fixes two possible deadlocks that cause `elp server` to hang during exit. I was on the right track in #74 but there are actually two deadlocks: * The first happens because the logger backend has a clone of the `Sender` of the `Connection` (because the `LspLogger` clones it one). This can cause a deadlock when a message is _not_ logged following the drop of `Server` on `io_threads.join()` because the `Connection`'s thread for receiving messages from the `Connection`'s `Receiver` awaits messages indefinitely. The solution for this deadlock is to remove the `LspLogger` from the backend on `Drop` of the server (or any time earlier). That decrements the reference count on the `Sender` and causes the `Receiver`'s iterator to return `None` resulting in the termination of its thread. * The second I don't really understand fully. The `Drop for Server` hangs because the cache task pool's drop joins its threads (by design) and there is an outstanding task that seems to be frozen, I think related to Salsa's cancellation mechanism. Explicitly requesting cancellation during drop eliminates this problem. Plus it matches what rust-analyzer does. Fixes #36 Pull Request resolved: #75 Reviewed By: michalmuskala Differential Revision: D69115301 Pulled By: alanz fbshipit-source-id: 3b0a5b39f49d668199a2370f1b23bc78f0ae6d17
1 parent 5608951 commit 9e55192

File tree

2 files changed

+17
-1
lines changed

2 files changed

+17
-1
lines changed

crates/elp/src/server.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,16 @@ pub struct Server {
256256
vfs_config_version: u32,
257257
}
258258

259+
impl Drop for Server {
260+
fn drop(&mut self) {
261+
// Remove the LSP logger. This prevents a deadlock because the logger has a strong
262+
// reference to the `Connection`'s `Sender`.
263+
self.logger.remove_logger(LOGGER_NAME);
264+
// Cancel any ongoing analyses.
265+
self.analysis_host.request_cancellation();
266+
}
267+
}
268+
259269
impl Server {
260270
#[allow(clippy::too_many_arguments)]
261271
pub fn new(
@@ -603,7 +613,6 @@ impl Server {
603613
RequestDispatcher::new(self, req)
604614
.on_sync::<request::Shutdown>(|this, ()| {
605615
this.transition(Status::ShuttingDown);
606-
this.analysis_host.request_cancellation();
607616
Ok(())
608617
})?
609618
.on::<request::CodeActionRequest>(handlers::handle_code_action)

crates/elp_log/src/lib.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,13 @@ impl Logger {
7272
log::set_max_level(shared.max_level());
7373
}
7474

75+
pub fn remove_logger(&self, name: &str) {
76+
let mut shared = self.shared.write();
77+
shared.writers.remove(name.into());
78+
79+
log::set_max_level(shared.max_level());
80+
}
81+
7582
pub fn reconfigure(&self, name: &str, filter: Builder) {
7683
let mut shared = self.shared.write();
7784

0 commit comments

Comments
 (0)