src/goLanguageServer: protect language restart with mutex

We have used the languageServerStartInProgress flag to prevent
issuing another language start request while the previous language
start up process is in progress and is blocked. This prevented
the race of multiple language start calls. However, this can cause
important language client restart request to be skipped. Consider
this scenario:

- user modifies the setting to enable gopls and that triggers a call to
  startLanguageServerWithFallback. While this is waiting on
  startLanguageServer that will run with cfg.enabled = true.
- user modifies the stting to disable gopls, and that triggers another
  call to startLanguageServerWithFallback.
- the second startLanguageServerWithFallback will skip startLanguageServer
  because languageServerStartInProgress is true.
- As a result, we will fail to disable the language server.

This change fixes the issue by using a new Mutex to protect
startLanguageServer. With the change, the second call won't skip
startLanguageServer, but will be resumed when the previous
startLanguageServer call is finished.

This change also fixes the bug in src/goStatus that produced incorrect
language server status icon when language server is disabled.

Fixes #1132

Change-Id: I4435d41b843032ff8f675ea95aac002d9ba79b4b
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/288352
Run-TryBot: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: kokoro <[email protected]>
Trust: Hyang-Ah Hana Kim <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>

Author: hyangah

src/goLanguageServer.ts

@@ -65,6 +65,7 @@ import {
 	getWorkspaceFolderPath,
 	removeDuplicateDiagnostics
 } from './util';
+import { Mutex } from './utils/mutex';
 import { getToolFromToolPath } from './utils/pathUtils';
 
 export interface LanguageServerConfig {
@@ -89,9 +90,9 @@ let languageServerDisposable: vscode.Disposable;
 let latestConfig: LanguageServerConfig;
 export let serverOutputChannel: vscode.OutputChannel;
 export let languageServerIsRunning = false;
-// TODO: combine languageServerIsRunning & languageServerStartInProgress
-// as one languageServerStatus variable.
-let languageServerStartInProgress = false;
+
+const languageServerStartMutex = new Mutex();
+
 let serverTraceChannel: vscode.OutputChannel;
 let crashCount = 0;
 
@@ -122,11 +123,6 @@ export async function startLanguageServerWithFallback(ctx: vscode.ExtensionConte
 		}
 	}
 
-	if (!activation && languageServerStartInProgress) {
-		console.log('language server restart is already in progress...');
-		return;
-	}
-
 	const goConfig = getGoConfig();
 	const cfg = buildLanguageServerConfig(goConfig);
 
@@ -146,21 +142,21 @@ export async function startLanguageServerWithFallback(ctx: vscode.ExtensionConte
 			}
 		}
 	}
+	const unlock = await languageServerStartMutex.lock();
+	try {
+		const started = await startLanguageServer(ctx, cfg);
 
-	languageServerStartInProgress = true;
-
-	const started = await startLanguageServer(ctx, cfg);
-
-	// If the server has been disabled, or failed to start,
-	// fall back to the default providers, while making sure not to
-	// re-register any providers.
-	if (!started && defaultLanguageProviders.length === 0) {
-		registerDefaultProviders(ctx);
+		// If the server has been disabled, or failed to start,
+		// fall back to the default providers, while making sure not to
+		// re-register any providers.
+		if (!started && defaultLanguageProviders.length === 0) {
+			registerDefaultProviders(ctx);
+		}
+		languageServerIsRunning = started;
+		updateLanguageServerIconGoStatusBar(started, goConfig['useLanguageServer'] === true);
+	} finally {
+		unlock();
 	}
-
-	languageServerIsRunning = started;
-	updateLanguageServerIconGoStatusBar(started, goConfig['useLanguageServer'] === true);
-	languageServerStartInProgress = false;
 }
 
 // scheduleGoplsSuggestions sets timeouts for the various gopls-specific

src/goStatus.ts

@@ -129,10 +129,8 @@ export function updateLanguageServerIconGoStatusBar(started: boolean, enabled: b
 	let text = goEnvStatusbarItem.text;
 	let icon = '';
 	if (text.endsWith(languageServerIcon)) {
-		icon = languageServerIcon;
 		text = text.substring(0, text.length - languageServerIcon.length);
 	} else if (text.endsWith(languageServerErrorIcon)) {
-		icon = languageServerErrorIcon;
 		text = text.substring(0, text.length - languageServerErrorIcon.length);
 	}
 

src/utils/mutex.ts

@@ -0,0 +1,41 @@
+/*---------------------------------------------------------
+ * Copyright 2021 The Go Authors. All rights reserved.
+ * Licensed under the MIT License. See LICENSE in the project root for license information.
+ *--------------------------------------------------------*/
+
+'use strict';
+
+/* Mutex provides mutex feature by building a promise chain.
+
+  const m = new Mutex();
+
+  const unlock = await m.lock();
+  try {
+	  // critical section
+  } finally {
+	  unlock();
+  }
+*/
+export class Mutex {
+	private mutex = Promise.resolve();
+
+	public lock(): PromiseLike<() => void> {
+		// Based on https://spin.atomicobject.com/2018/09/10/javascript-concurrency/
+
+		let x: (unlock: () => void) => void;
+
+		// add to the promise chain of this mutex.
+		// When all the prior promises in the chain are resolved,
+		// x, which will be the resolve callback of promise B,
+		// will run and cause to unblock the waiter of promise B.
+		this.mutex = this.mutex.then(() => {
+			return new Promise(x);  // promise A
+		});
+
+		return new Promise((resolve) => {  // promise B
+			x = resolve;
+		});
+		// the returned Promise will resolve when all the previous
+		// promises chained in this.mutex resolve.
+	}
+}

test/unit/mutex.test.ts

@@ -0,0 +1,60 @@
+/*---------------------------------------------------------
+ * Copyright 2021 The Go Authors. All rights reserved.
+ * Licensed under the MIT License. See LICENSE in the project root for license information.
+ *--------------------------------------------------------*/
+
+import * as assert from 'assert';
+import { Mutex } from '../../src/utils/mutex';
+
+suite('Mutex Tests', () => {
+	test('works for basic concurrent access', async () => {
+		const m = new Mutex();
+
+		let cnt = 0;
+		const worker = async (delay: number, count: number) => {
+			for (let i = 0; i < count; i++) {
+				const unlock = await m.lock();
+				try {
+					const cntCopy = cnt;
+					await sleep(delay);
+					cnt = cntCopy + 1;
+				} finally {
+					unlock();
+				}
+			}
+		};
+
+		await Promise.all([worker(3, 5), worker(1, 10)]);
+		assert.strictEqual(cnt, 15);
+	});
+
+	test('works when lock holders throw errors', async () => {
+		const m = new Mutex();
+
+		let cnt = 0;
+		const worker = async (delay: number) => {
+			const unlock = await m.lock();
+			try {
+				const cntCopy = cnt;
+				await sleep(delay);
+				cnt = cntCopy + 1;
+				throw new Error('ooops');
+			} finally {
+				unlock();
+			}
+		};
+
+		const safeWorker = async (delay: number) => {
+			try {
+				await worker(delay);
+			} catch (e) {
+				// swallow the exception
+			}
+		};
+
+		await Promise.all([safeWorker(3), safeWorker(2), safeWorker(1), safeWorker(0)]);
+		assert.strictEqual(cnt, 4);
+	});
+});
+
+function sleep(ms: number) { return new Promise((resolve) => setTimeout(resolve, ms)); }