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 golang/vscode-go#1132
Change-Id: I4435d41b843032ff8f675ea95aac002d9ba79b4b
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/288352
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/src/goLanguageServer.ts b/src/goLanguageServer.ts
index 89a3ebd..0a7cf93 100644
--- a/src/goLanguageServer.ts
+++ b/src/goLanguageServer.ts
@@ -65,6 +65,7 @@
getWorkspaceFolderPath,
removeDuplicateDiagnostics
} from './util';
+import { Mutex } from './utils/mutex';
import { getToolFromToolPath } from './utils/pathUtils';
export interface LanguageServerConfig {
@@ -89,9 +90,9 @@
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 @@
}
}
- if (!activation && languageServerStartInProgress) {
- console.log('language server restart is already in progress...');
- return;
- }
-
const goConfig = getGoConfig();
const cfg = buildLanguageServerConfig(goConfig);
@@ -146,21 +142,21 @@
}
}
}
+ 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
diff --git a/src/goStatus.ts b/src/goStatus.ts
index 1c8f5cd..f1e1aa5 100644
--- a/src/goStatus.ts
+++ b/src/goStatus.ts
@@ -129,10 +129,8 @@
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);
}
diff --git a/src/utils/mutex.ts b/src/utils/mutex.ts
new file mode 100644
index 0000000..6c69dee
--- /dev/null
+++ b/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.
+ }
+}
diff --git a/test/unit/mutex.test.ts b/test/unit/mutex.test.ts
new file mode 100644
index 0000000..9972034
--- /dev/null
+++ b/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)); }
