Repository: autozimu/LanguageClient-neovim (language: rust)

Issue:
Multiple language servers get started (and terminated) if vim is started with `vim -p`
- Did you upgrade to latest plugin version? yes
- Did you upgrade to/compile latest binary? `languageclient 0.1.147`

## Describe the bug
Multiple php language servers are started up on vim startup when vim opens multiple php files with `vim -p`. This requires a lot of memory to initialize (causes swapping if there are dozens of files), and all but one don't end up being used

I believe this is a race condition in src/language_server_protocol.rs in languageClient_startServer.

The project that I'm editing with vim is very large, so the language server takes more than a second to respond to the language server `initialize` RPC request

```rust
        if self.get(|state| state.clients.contains_key(&Some(languageId.clone())))? {
            return Ok(json!({}));
        }
// much, much later, without a mutex getting acquired globally or for the language id
        self.update(|state| {
            state.clients.insert(Some(languageId.clone()), client);
            Ok(())
        })?;
```
## Environment
- vim 8.1-1-1778
- This plugin version (`git rev-parse --short HEAD`): c851073
- This plugin's binary version (`bin/languageclient --version`):  0.1.147
- Minimal vimrc content: TODO
- Language server link and version: https://github.com/TysonAndre/LanguageServer-phan-neovim (latest)

## To Reproduce
Steps to reproduce the behavior:
1. Set up phan for a large project and install the language server
1. Start vim, `vim -p file1.php file2.php file3.php file4.php`
1. `ps -ef|grep -i php`. Observe that initially, multiple language servers are running, but afterwards, there are defunct php processes with the `bin/languageclient` as the parent process.

## Current behavior
Multiple php language servers are started up on vim startup when vim opens multiple php files with `vim -p`. This requires a lot of memory to initialize (causes swapping if there are dozens of files), and all but one don't end up being used

## Expected behavior

A single php language server is started up (i.e. one per language, or one per server command in the future)

## Screenshots
If applicable, add screenshots to help explain your problem.

## Additional context
Add any other context about the problem here.



Discussion / hints:

Fix patch:
diff --git a/src/language_client.rs b/src/language_client.rs
index 224df21e6..f06885446 100644
--- a/src/language_client.rs
+++ b/src/language_client.rs
@@ -2,17 +2,42 @@ use super::*;
 use crate::vim::Vim;
 use std::ops::DerefMut;
 
-pub struct LanguageClient(pub Arc<Mutex<State>>);
+pub struct LanguageClient {
+    pub state_mutex: Arc<Mutex<State>>,
+    pub clients_mutex: Arc<Mutex<HashMap<LanguageId, Arc<Mutex<()>>>>>,
+}
 
 impl LanguageClient {
     // NOTE: Don't expose this as public.
     // MutexGuard could easily halt the program when one guard is not released immediately after use.
     fn lock(&self) -> Fallible<MutexGuard<State>> {
-        self.0
+        self.state_mutex
             .lock()
             .map_err(|err| format_err!("Failed to lock state: {:?}", err))
     }
 
+    // This fetches a mutex that is unique to the provided languageId.
+    //
+    // Here, we return a mutex instead of the mutex guard because we need to satisfy the borrow
+    // checker. Otherwise, there is no way to guarantee that the mutex in the hash map wouldn't be
+    // garbage collected as a result of another modification updating the hash map, while something was holding the lock
+    pub fn get_client_update_mutex(&self, languageId: LanguageId) -> Fallible<Arc<Mutex<()>>> {
+        let map_guard = self.clients_mutex.lock();
+        if map_guard.is_err() {
+            return Err(format_err!(
+                "Failed to lock client creation for languageId {:?}: {:?}",
+                languageId,
+                map_guard.unwrap_err()
+            ));
+        }
+        let mut map = map_guard.unwrap();
+        if !map.contains_key(&languageId) {
+            map.insert(languageId.clone(), Arc::new(Mutex::new(())));
+        }
+        let mutex: Arc<Mutex<()>> = map.get(&languageId).unwrap().clone();
+        Ok(mutex)
+    }
+
     pub fn get<T>(&self, f: impl FnOnce(&State) -> T) -> Fallible<T> {
         Ok(f(self.lock()?.deref()))
     }
diff --git a/src/language_server_protocol.rs b/src/language_server_protocol.rs
index 7c0a88aaf..050344f3f 100644
--- a/src/language_server_protocol.rs
+++ b/src/language_server_protocol.rs
@@ -25,7 +25,10 @@ impl LanguageClient {
 
     pub fn loop_call(&self, rx: &crossbeam_channel::Receiver<Call>) -> Fallible<()> {
         for call in rx.iter() {
-            let language_client = Self(self.0.clone());
+            let language_client = LanguageClient {
+                state_mutex: self.state_mutex.clone(),
+                clients_mutex: self.clients_mutex.clone(), // not sure if useful to clone this
+            };
             thread::spawn(move || {
                 if let Err(err) = language_client.handle_call(call) {
                     error!("Error handling request:\n{:?}", err);
@@ -2827,6 +2830,29 @@ impl LanguageClient {
         let cmdparams = vim_cmd_args_to_value(&cmdargs)?;
         let params = params.combine(&cmdparams);
 
+        // When multiple buffers get opened up concurrently,
+        // startServer gets called concurrently.
+        // This lock ensures that at most one language server is starting up at a time per
+        // languageId.
+        // We keep the mutex in scope to satisfy the borrow checker.
+        // This ensures that the mutex isn't garbage collected while the MutexGuard is held.
+        //
+        // - e.g. prevents starting multiple servers with `vim -p`.
+        // - This continues to allow distinct language servers to start up concurrently
+        //   by languageId (e.g. java and rust)
+        // - Revisit this when more than one server is allowed per languageId.
+        //   (ensure that the mutex is acquired by what starts the group of servers)
+        //
+        // TODO: May want to lock other methods that update the list of clients.
+        let mutex_for_language_id = self.get_client_update_mutex(Some(languageId.clone()))?;
+        let _raii_lock: MutexGuard<()> = mutex_for_language_id.lock().map_err(|err| {
+            format_err!(
+                "Failed to lock client creation for languageId {:?}: {:?}",
+                languageId,
+                err
+            )
+        })?;
+
         if self.get(|state| state.clients.contains_key(&Some(languageId.clone())))? {
             return Ok(json!({}));
         }
diff --git a/src/main.rs b/src/main.rs
index bdcedf0a9..94d49fdbc 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -61,7 +61,10 @@ fn main() -> Fallible<()> {
     let _ = args.get_matches();
 
     let (tx, rx) = crossbeam_channel::unbounded();
-    let language_client = language_client::LanguageClient(Arc::new(Mutex::new(State::new(tx)?)));
+    let language_client = language_client::LanguageClient {
+        state_mutex: Arc::new(Mutex::new(State::new(tx)?)),
+        clients_mutex: Arc::new(Mutex::new(HashMap::new())),
+    };
 
     language_client.loop_call(&rx)
 }


Test patch: