From 1ad5c33383f743d0a41ff0e401fdcab58ffcd056 Mon Sep 17 00:00:00 2001 From: Hana Date: Sat, 30 Jan 2021 17:03:20 -0500 Subject: [PATCH] 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 TryBot-Result: kokoro Trust: Hyang-Ah Hana Kim Reviewed-by: Rebecca Stambler --- src/goLanguageServer.ts | 38 ++++++++++++-------------- src/goStatus.ts | 2 -- src/utils/mutex.ts | 41 ++++++++++++++++++++++++++++ test/unit/mutex.test.ts | 60 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 23 deletions(-) create mode 100644 src/utils/mutex.ts create mode 100644 test/unit/mutex.test.ts diff --git a/src/goLanguageServer.ts b/src/goLanguageServer.ts index 89a3ebd081..0a7cf93b06 100644 --- a/src/goLanguageServer.ts +++ b/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 diff --git a/src/goStatus.ts b/src/goStatus.ts index 1c8f5cd7db..f1e1aa5796 100644 --- a/src/goStatus.ts +++ b/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); } diff --git a/src/utils/mutex.ts b/src/utils/mutex.ts new file mode 100644 index 0000000000..6c69deea60 --- /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 0000000000..9972034de4 --- /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)); }