Removed errant proc.kill() (#2103)

* Removed errant `proc.kill()`

* Added `finish` to `benchmark_diagnostics` as well

Thought: do we want to just `impl Drop for TestHarness`? This might make the panicking situation tricky, whereas explicitly calling `harness.finish()`, while possible to forget, makes handling panics just work.

* `impl Drop for Server`

Ensures shutdown and cleanup so that we can't forget
This is important during benchmarking; Criterion creates thousands of servers, and if we don't shut them down and clean them up, we'll exhaust the system's process pool

lsp/lsp-harness/src/jsonrpc.rs

@@ -96,6 +96,24 @@ pub struct Response {
     error: Option<ResponseError>,
 }
 
+impl Drop for Server {
+    fn drop(&mut self) {
+        /// Shut down the language server gracefully.
+        pub fn shutdown(this: &mut Server) -> Result<()> {
+            this.send_request::<Shutdown>(())?;
+            this.send_notification::<Exit>(())?;
+
+            // needed to clean up the process, and in particular to return PIDs to the
+            // process pool to avoid having the system run out of processes during
+            // benchmarking
+            this.proc.wait()?;
+
+            Ok(())
+        }
+        shutdown(self).unwrap()
+    }
+}
+
 impl Server {
     /// Similar to `new`, but allows passing custom stuff
     pub fn new_with_options(
@@ -127,17 +145,6 @@ 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 {
@@ -174,12 +181,6 @@ impl Server {
         })
     }
 
-    /// Shut down the language server gracefully.
-    pub fn shutdown(&mut self) -> Result<()> {
-        self.send_request::<Shutdown>(())?;
-        self.send_notification::<Exit>(())
-    }
-
     fn initialize(&mut self, initialization_options: Option<serde_json::Value>) -> Result<()> {
         // `root_path` is deprecated, but we need ot initialize the struct
         // somehow. There is no `Default` implementation for `InitilizeParams`

lsp/lsp-harness/src/lib.rs

@@ -211,14 +211,6 @@ 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,

lsp/nls/benches/main.rs

@@ -67,8 +67,6 @@ fn benchmark_one_test(c: &mut Criterion, path: &str) {
             // finish before we try to benchmark a request.
             harness.wait_for_diagnostics();
             b.iter(|| harness.request_dyn(req.clone()));
-
-            harness.finish().unwrap();
         });
     }
 }
@@ -96,6 +94,7 @@ fn benchmark_diagnostics(c: &mut Criterion, path: &str) {
         let name = format!("init-diagnostics-{path}-{i:03}");
         c.bench_function(&name, |b| {
             let mut harness = TestHarness::new();
+
             b.iter(|| {
                 harness.send_file(f.uri.clone(), &f.contents);
                 loop {