comment clarification

src/client.rs

@@ -299,18 +299,10 @@ fn select_workspace_root<'a>(
 
 /// Receive messages from channel and write them to the client input socket
 async fn input_task(mut rx: mpsc::Receiver<Message>, mut writer: LspWriter<OwnedWriteHalf>) {
-    // Unlike the output task, here we first wait on the channel which is going
-    // to block until the language server sends a notification, however if
-    // we're the last client and have just closed the server is unlikely to send
-    // any. This results in the last client often falsely hanging while the gc
-    // task depends on the input channels being closed to detect a disconnected
-    // client.
-    //
-    // When a client sends a shutdown request we receive a message on the
-    // `close_rx`, send the reply and close the connection. If no shutdown
-    // request was received but the client closed `close_rx` channel will be
-    // dropped (unlike the normal rx channel which is shared) and the connection
-    // will close without sending any response.
+    // The other end of this channel is held by the `output_task` _and_ in the
+    // `Instance` itself, this task depends on the `output_task` to detect a
+    // client disconnect and call `Instance::cleanup_client`, otherwise we're
+    // going to hang forever here.
     while let Some(message) = rx.recv().await {
         if let Err(err) = writer.write_message(&message).await {
             match err.kind() {

src/instance.rs

@@ -1,3 +1,4 @@
+use std::collections::hash_map::Entry;
 use std::collections::{HashMap, HashSet};
 use std::io::ErrorKind;
 use std::ops::Deref;
@@ -390,22 +391,24 @@ pub async fn get_or_spawn(
     key: InstanceKey,
     init_req_params: lsp::InitializeParams,
 ) -> Result<Arc<Instance>> {
-    // We have locked the whole map so we can assume noone else tries to spawn
-    // the same instance again.
-    let map_clone = map.clone();
-    let mut map_lock = map_clone.lock().await;
-
-    if let Some(instance) = map_lock.0.get(&key) {
-        info!("reusing language server instance");
-        return Ok(instance.clone());
+    // We have locked a clone of an Arc of the map, we can assume noone else
+    // tries to spawn the same instance again. But we have to make sure `spawn`
+    // doesn't try to lock its copy as well. This is a bit unfortunate code
+    // organization but we want to have spawn in a separate tracing context and
+    // we want to include `wait_task` in it as well in it as well
+    match map.clone().lock().await.0.entry(key.clone()) {
+        Entry::Occupied(e) => {
+            info!("reusing language server instance");
+            Ok(e.get().clone())
+        }
+        Entry::Vacant(e) => {
+            let instance = spawn(key, init_req_params, map)
+                .await
+                .context("spawning instance")?;
+            e.insert(instance.clone());
+            Ok(instance)
+        }
     }
-
-    let instance = spawn(key.clone(), init_req_params, map)
-        .await
-        .context("spawning instance")?;
-    map_lock.0.insert(key, instance.clone());
-
-    Ok(instance)
 }
 
 #[instrument(name = "instance", fields(pid = field::Empty), skip_all, parent = None)]