diff --git a/server/CHANGELOG.md b/server/CHANGELOG.md index 2ef76faa..542cd42b 100644 --- a/server/CHANGELOG.md +++ b/server/CHANGELOG.md @@ -1,5 +1,9 @@ # Bash Language Server +## 4.2.2 + +- Reduce CPU usage by introduce a short execution delay and throttling for ShellCheck tasks https://github.com/bash-lsp/bash-language-server/pull/655 + ## 4.2.1 - Add support for resolving loop variables https://github.com/bash-lsp/bash-language-server/pull/653 diff --git a/server/package.json b/server/package.json index e048f564..6a1741b9 100644 --- a/server/package.json +++ b/server/package.json @@ -3,7 +3,7 @@ "description": "A language server for Bash", "author": "Mads Hartmann", "license": "MIT", - "version": "4.2.1", + "version": "4.2.2", "publisher": "mads-hartmann", "main": "./out/server.js", "typings": "./out/server.d.ts", diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index 7a33503c..8007a831 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.test.ts @@ -15,6 +15,12 @@ import { getMockConnection } from '../../../testing/mocks' import LspServer from '../server' import { CompletionItemDataType } from '../types' +// Skip ShellCheck throttle delay in test cases +jest.spyOn(global, 'setTimeout').mockImplementation((fn) => { + fn() + return 0 as any +}) + async function initializeServer( { rootPath }: { rootPath?: string } = { rootPath: pathToFileURL(FIXTURE_FOLDER).href }, ) { diff --git a/server/src/shellcheck/__tests__/index.test.ts b/server/src/shellcheck/__tests__/index.test.ts index bcf873c2..291b1250 100644 --- a/server/src/shellcheck/__tests__/index.test.ts +++ b/server/src/shellcheck/__tests__/index.test.ts @@ -7,11 +7,37 @@ import { Linter } from '../index' const mockConsole = getMockConnection().console +jest.useFakeTimers() + const FIXTURE_DOCUMENT_URI = `file://${FIXTURE_FOLDER}/foo.sh` function textToDoc(txt: string) { return TextDocument.create(FIXTURE_DOCUMENT_URI, 'bar', 0, txt) } +async function getLintingResult({ + additionalShellCheckArguments = [], + cwd, + document, + executablePath = 'shellcheck', + sourcePaths = [], +}: { + additionalShellCheckArguments?: string[] + cwd?: string + document: TextDocument + executablePath?: string + sourcePaths?: string[] +}): Promise<[Awaited>, Linter]> { + const linter = new Linter({ + console: mockConsole, + cwd, + executablePath, + }) + const promise = linter.lint(document, sourcePaths, additionalShellCheckArguments) + jest.runAllTimers() + const result = await promise + return [result, linter] +} + describe('linter', () => { it('default to canLint to true', () => { expect(new Linter({ console: mockConsole, executablePath: 'foo' }).canLint).toBe(true) @@ -19,14 +45,17 @@ describe('linter', () => { it('should set canLint to false when linting fails', async () => { const executablePath = '77b4d3f6-c87a-11ec-9b62-a3c90f66d29f' - const linter = new Linter({ - console: mockConsole, + + const [result, linter] = await getLintingResult({ + document: textToDoc(''), executablePath, }) - expect(await linter.lint(textToDoc(''), [])).toEqual({ + + expect(result).toEqual({ codeActions: [], diagnostics: [], }) + expect(linter.canLint).toBe(false) expect(mockConsole.warn).toBeCalledWith( expect.stringContaining( @@ -35,15 +64,14 @@ describe('linter', () => { ) }) - it('should lint when shellcheck present', async () => { + it('should lint when shellcheck is present', async () => { // prettier-ignore const shell = [ '#!/bin/bash', 'echo $foo', ].join('\n') - const linter = new Linter({ console: mockConsole, executablePath: 'shellcheck' }) - const result = await linter.lint(textToDoc(shell), []) + const [result] = await getLintingResult({ document: textToDoc(shell) }) expect(result).toMatchInlineSnapshot(` Object { "codeActions": Array [ @@ -153,12 +181,11 @@ describe('linter', () => { }) it('should correctly follow sources with correct cwd', async () => { - const linter = new Linter({ - console: mockConsole, + const [result] = await getLintingResult({ cwd: FIXTURE_FOLDER, - executablePath: 'shellcheck', + document: FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, }) - const result = await linter.lint(FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, []) + expect(result).toEqual({ codeActions: [], diagnostics: [], @@ -166,12 +193,11 @@ describe('linter', () => { }) it('should fail to follow sources with incorrect cwd', async () => { - const linter = new Linter({ - console: mockConsole, + const [result] = await getLintingResult({ cwd: path.resolve(path.join(FIXTURE_FOLDER, '../')), - executablePath: 'shellcheck', + document: FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, }) - const result = await linter.lint(FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, []) + expect(result).toMatchInlineSnapshot(` Object { "codeActions": Array [], @@ -222,14 +248,11 @@ describe('linter', () => { }) it('should follow sources with incorrect cwd if the execution path is passed', async () => { - const linter = new Linter({ - console: mockConsole, + const [result] = await getLintingResult({ cwd: path.resolve(path.join(FIXTURE_FOLDER, '../')), - executablePath: 'shellcheck', + document: FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, + sourcePaths: [path.resolve(FIXTURE_FOLDER)], }) - const result = await linter.lint(FIXTURE_DOCUMENT.SHELLCHECK_SOURCE, [ - path.resolve(FIXTURE_FOLDER), - ]) expect(result).toEqual({ codeActions: [], diagnostics: [], diff --git a/server/src/shellcheck/index.ts b/server/src/shellcheck/index.ts index d8b8c3a2..5495ec0e 100644 --- a/server/src/shellcheck/index.ts +++ b/server/src/shellcheck/index.ts @@ -5,6 +5,7 @@ import { spawn } from 'child_process' import * as LSP from 'vscode-languageserver/node' import { TextDocument } from 'vscode-languageserver-textdocument' +import { ThrottledDelayer } from '../util/async' import { analyzeShebang } from '../util/shebang' import { CODE_TO_TAGS, LEVEL_TO_SEVERITY } from './config' import { @@ -15,24 +16,33 @@ import { } from './types' const SUPPORTED_BASH_DIALECTS = ['sh', 'bash', 'dash', 'ksh'] - +const DEBOUNCE_MS = 300 type LinterOptions = { executablePath: string console: LSP.RemoteConsole cwd?: string } +type LintingResult = { + diagnostics: LSP.Diagnostic[] + codeActions: LSP.CodeAction[] +} + export class Linter { - public executablePath: string - private cwd: string private console: LSP.RemoteConsole + private cwd: string + public executablePath: string + private uriToDelayer: { + [uri: string]: ThrottledDelayer + } private _canLint: boolean constructor({ console, cwd, executablePath }: LinterOptions) { - this.executablePath = executablePath - this.cwd = cwd || process.cwd() this._canLint = true this.console = console + this.cwd = cwd || process.cwd() + this.executablePath = executablePath + this.uriToDelayer = Object.create(null) } public get canLint(): boolean { @@ -43,11 +53,28 @@ export class Linter { document: TextDocument, sourcePaths: string[], additionalShellCheckArguments: string[] = [], - ): Promise<{ diagnostics: LSP.Diagnostic[]; codeActions: LSP.CodeAction[] }> { + ): Promise { if (!this._canLint) { return { diagnostics: [], codeActions: [] } } + const { uri } = document + let delayer = this.uriToDelayer[uri] + if (!delayer) { + delayer = new ThrottledDelayer(DEBOUNCE_MS) + this.uriToDelayer[uri] = delayer + } + + return delayer.trigger(() => + this.executeLint(document, sourcePaths, additionalShellCheckArguments), + ) + } + + private async executeLint( + document: TextDocument, + sourcePaths: string[], + additionalShellCheckArguments: string[] = [], + ): Promise { const result = await this.runShellCheck( document, [...sourcePaths, dirname(fileURLToPath(document.uri))], @@ -57,6 +84,9 @@ export class Linter { return { diagnostics: [], codeActions: [] } } + // Clean up the debounced function + delete this.uriToDelayer[document.uri] + return mapShellCheckResult({ document, result }) } @@ -100,7 +130,7 @@ export class Linter { proc.stdout.on('data', (data) => (out += data)) proc.stderr.on('data', (data) => (err += data)) proc.stdin.on('error', () => { - // XXX: Ignore STDIN errors in case the process ends too quickly, before we try to + // NOTE: Ignore STDIN errors in case the process ends too quickly, before we try to // write. If we write after the process ends without this, we get an uncatchable EPIPE. // This is solved in Node >= 15.1 by the "on('spawn', ...)" event, but we need to // support earlier versions. @@ -108,7 +138,7 @@ export class Linter { proc.stdin.end(documentText) }) - // XXX: do we care about exit code? 0 means "ok", 1 possibly means "errors", + // NOTE: do we care about exit code? 0 means "ok", 1 possibly means "errors", // but the presence of parseable errors in the output is also sufficient to // distinguish. let exit @@ -141,6 +171,7 @@ export class Linter { return ShellCheckResultSchema.parse(raw) } } + function mapShellCheckResult({ document, result, diff --git a/server/src/util/async.ts b/server/src/util/async.ts new file mode 100644 index 00000000..1eaa27fe --- /dev/null +++ b/server/src/util/async.ts @@ -0,0 +1,190 @@ +/* eslint-disable @typescript-eslint/no-non-null-assertion */ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + * Origin: https://github.com/vscode-shellcheck/vscode-shellcheck/blob/master/src/utils/async.ts + *--------------------------------------------------------------------------------------------*/ + +export interface ITask { + (): T +} + +/** + * A helper to prevent accumulation of sequential async tasks. + * + * Imagine a mail man with the sole task of delivering letters. As soon as + * a letter submitted for delivery, he drives to the destination, delivers it + * and returns to his base. Imagine that during the trip, N more letters were submitted. + * When the mail man returns, he picks those N letters and delivers them all in a + * single trip. Even though N+1 submissions occurred, only 2 deliveries were made. + * + * The throttler implements this via the queue() method, by providing it a task + * factory. Following the example: + * + * var throttler = new Throttler(); + * var letters = []; + * + * function letterReceived(l) { + * letters.push(l); + * throttler.queue(() => { return makeTheTrip(); }); + * } + */ +export class Throttler { + private activePromise: Promise | null + private queuedPromise: Promise | null + private queuedPromiseFactory: ITask> | null + + constructor() { + this.activePromise = null + this.queuedPromise = null + this.queuedPromiseFactory = null + } + + public queue(promiseFactory: ITask>): Promise { + if (this.activePromise) { + this.queuedPromiseFactory = promiseFactory + + if (!this.queuedPromise) { + const onComplete = () => { + this.queuedPromise = null + + const result = this.queue(this.queuedPromiseFactory!) + this.queuedPromiseFactory = null + + return result + } + + this.queuedPromise = new Promise((resolve) => { + this.activePromise!.then(onComplete, onComplete).then(resolve) + }) + } + + return new Promise((resolve, reject) => { + this.queuedPromise!.then(resolve, reject) + }) + } + + this.activePromise = promiseFactory() + + return new Promise((resolve, reject) => { + this.activePromise!.then( + (result: T) => { + this.activePromise = null + resolve(result) + }, + (err: any) => { + this.activePromise = null + reject(err) + }, + ) + }) + } +} + +/** + * A helper to delay execution of a task that is being requested often. + * + * Following the throttler, now imagine the mail man wants to optimize the number of + * trips proactively. The trip itself can be long, so the he decides not to make the trip + * as soon as a letter is submitted. Instead he waits a while, in case more + * letters are submitted. After said waiting period, if no letters were submitted, he + * decides to make the trip. Imagine that N more letters were submitted after the first + * one, all within a short period of time between each other. Even though N+1 + * submissions occurred, only 1 delivery was made. + * + * The delayer offers this behavior via the trigger() method, into which both the task + * to be executed and the waiting period (delay) must be passed in as arguments. Following + * the example: + * + * var delayer = new Delayer(WAITING_PERIOD); + * var letters = []; + * + * function letterReceived(l) { + * letters.push(l); + * delayer.trigger(() => { return makeTheTrip(); }); + * } + */ +export class Delayer { + public defaultDelay: number + private timeout: NodeJS.Timer | null + private completionPromise: Promise | null + private onResolve: ((value: T | PromiseLike | undefined) => void) | null + private task: ITask | null + + constructor(defaultDelay: number) { + this.defaultDelay = defaultDelay + this.timeout = null + this.completionPromise = null + this.onResolve = null + this.task = null + } + + public trigger(task: ITask, delay: number = this.defaultDelay): Promise { + this.task = task + this.cancelTimeout() + + if (!this.completionPromise) { + this.completionPromise = new Promise((resolve) => { + this.onResolve = resolve + }).then(() => { + this.completionPromise = null + this.onResolve = null + + const result = this.task!() + this.task = null + + return result + }) + } + + this.timeout = setTimeout(() => { + this.timeout = null + this.onResolve!(undefined) + }, delay) + + return this.completionPromise + } + + public isTriggered(): boolean { + return this.timeout !== null + } + + public cancel(): void { + this.cancelTimeout() + + if (this.completionPromise) { + this.completionPromise = null + } + } + + private cancelTimeout(): void { + if (this.timeout !== null) { + clearTimeout(this.timeout) + this.timeout = null + } + } +} + +/** + * A helper to delay execution of a task that is being requested often, while + * preventing accumulation of consecutive executions, while the task runs. + * + * Simply combine the two mail man strategies from the Throttler and Delayer + * helpers, for an analogy. + */ +export class ThrottledDelayer extends Delayer> { + private throttler: Throttler + + constructor(defaultDelay: number) { + super(defaultDelay) + + this.throttler = new Throttler() + } + + public override trigger( + promiseFactory: ITask>, + delay?: number, + ): Promise> { + return super.trigger(() => this.throttler.queue(promiseFactory), delay) + } +} diff --git a/vscode-client/src/extension.ts b/vscode-client/src/extension.ts index 6e0aaf98..0552f9a9 100644 --- a/vscode-client/src/extension.ts +++ b/vscode-client/src/extension.ts @@ -37,6 +37,13 @@ export async function activate(context: ExtensionContext) { debug: debugServerExecutable, } + // NOTE: To debug a server running in a process, use the following instead: + // This requires the server to be globally installed. + // const serverOptions = { + // command: 'bash-language-server', + // args: ['start'], + // } + const clientOptions: LanguageClientOptions = { documentSelector: [ {