Skip to content

Commit

Permalink
Fix spawning of too many TestHarnesses
Browse files Browse the repository at this point in the history
During benchmarking, each criterion run would spawn `TestHarness`, each with its own `lsp` process.
They weren't cleaned up between criterion runs, so eventually they would exhaust the system’s process pool.
Fixed this by explicitly killing and cleaning up the lsp at the end of a criterion run.
  • Loading branch information
rben01 committed Nov 19, 2024
1 parent 36e47d4 commit 3bca567
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 4 deletions.
27 changes: 24 additions & 3 deletions lsp/lsp-harness/src/jsonrpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ use std::{
use serde::{Deserialize, Serialize};

pub struct Server {
/// A handle to the underlying process.
///
/// Used when benchmarking so that we can kill the lsp; otherwise, repeated creation
/// of servers will exhaust the PID pool. Once a server is created, stdout and stdin
/// will have been moved out of the underlying process; access them through the
/// server's fields `write` (stdin) and `read` (stdout).
proc: std::process::Child,
/// For sending messages to the language server.
write: Box<dyn Write>,
/// For reading messages from the language server.
Expand Down Expand Up @@ -95,11 +102,14 @@ impl Server {
mut cmd: std::process::Command,
initialization_options: Option<serde_json::Value>,
) -> Result<Server> {
let lsp = cmd.stdin(Stdio::piped()).stdout(Stdio::piped()).spawn()?;
let mut lsp = cmd.stdin(Stdio::piped()).stdout(Stdio::piped()).spawn()?;
let stdin = lsp.stdin.take().unwrap();
let stdout = lsp.stdout.take().unwrap();

let mut lsp = Server {
write: Box::new(lsp.stdin.unwrap()),
read: Box::new(BufReader::new(lsp.stdout.unwrap())),
proc: lsp,
write: Box::new(stdin),
read: Box::new(BufReader::new(stdout)),
pending_notifications: Vec::new(),
id: 0,
};
Expand All @@ -117,6 +127,17 @@ impl Server {
Server::new_with_options(cmd, None)
}

/// Shut down the language server by calling `kill` on its `proc`.
///
/// This isn't ordinarily necessary, but is needed when benchmarking to avoid
/// exhausting the PID pool when thousands of servers are created.
pub fn die(mut self) -> Result<()> {
self.shutdown()?;
self.proc.kill()?;
self.proc.wait()?; // force cleanup of process (kill alone is insufficient)
Ok(())
}

/// Make the language server aware of a file.
pub fn send_file(&mut self, uri: Url, contents: &str) -> Result<()> {
self.send_notification::<DidOpenTextDocument>(DidOpenTextDocumentParams {
Expand Down
8 changes: 8 additions & 0 deletions lsp/lsp-harness/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,14 @@ impl TestHarness {
Self::new_with_options(None)
}

/// Shut down the underlying language server by calling `kill` on its `proc`.
///
/// This is used when benchmarking to avoid exhausting the computer's PID pool when
/// thousands of servers are created.
pub fn finish(self) -> anyhow::Result<()> {
self.srv.die()
}

pub fn request<T: LspRequest>(&mut self, params: T::Params)
where
T::Result: LspDebug,
Expand Down
4 changes: 3 additions & 1 deletion lsp/nls/benches/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ fn benchmark_one_test(c: &mut Criterion, path: &str) {
// If the input is big, nls will be blocked generating diagnostics. Let that
// finish before we try to benchmark a request.
harness.wait_for_diagnostics();
b.iter(|| harness.request_dyn(req.clone()))
b.iter(|| harness.request_dyn(req.clone()));

harness.finish().unwrap();
});
}
}
Expand Down

0 comments on commit 3bca567

Please sign in to comment.