From 9c36f5e34317a9d99c4e0ebd7c25447edbfc11af Mon Sep 17 00:00:00 2001 From: Yulong Date: Tue, 11 Jun 2019 05:18:06 +0800 Subject: [PATCH] [Code] stop respawn process after `maxRespawn` reached (#38392) --- x-pack/plugins/code/common/lsp_error_codes.ts | 1 + .../code/server/indexer/lsp_indexer.ts | 9 +++- .../code/server/lsp/abstract_launcher.test.ts | 32 ++++++++--- .../code/server/lsp/abstract_launcher.ts | 53 +++++++++++++------ x-pack/plugins/code/server/lsp/controller.ts | 26 ++++----- .../plugins/code/server/lsp/java_launcher.ts | 2 +- x-pack/plugins/code/server/lsp/proxy.ts | 24 ++++++--- x-pack/plugins/code/server/routes/lsp.ts | 12 ++++- .../plugins/code/server/utils/cancelable.ts | 14 +++-- 9 files changed, 122 insertions(+), 51 deletions(-) diff --git a/x-pack/plugins/code/common/lsp_error_codes.ts b/x-pack/plugins/code/common/lsp_error_codes.ts index c39384cefc7a27..b51361b06364d7 100644 --- a/x-pack/plugins/code/common/lsp_error_codes.ts +++ b/x-pack/plugins/code/common/lsp_error_codes.ts @@ -11,3 +11,4 @@ export const UnknownErrorCode: number = ErrorCodes.UnknownErrorCode; export const UnknownFileLanguage: number = -42404; export const LanguageServerNotInstalled: number = -42403; export const LanguageDisabled: number = -42402; +export const LanguageServerStartFailed: number = -42405; diff --git a/x-pack/plugins/code/server/indexer/lsp_indexer.ts b/x-pack/plugins/code/server/indexer/lsp_indexer.ts index cc9175db03fb36..fcb21cb19c3136 100644 --- a/x-pack/plugins/code/server/indexer/lsp_indexer.ts +++ b/x-pack/plugins/code/server/indexer/lsp_indexer.ts @@ -11,7 +11,10 @@ import { ResponseError } from 'vscode-jsonrpc'; import { ProgressReporter } from '.'; import { TEXT_FILE_LIMIT } from '../../common/file'; -import { LanguageServerNotInstalled } from '../../common/lsp_error_codes'; +import { + LanguageServerNotInstalled, + LanguageServerStartFailed, +} from '../../common/lsp_error_codes'; import { toCanonicalUrl } from '../../common/uri_util'; import { Document, IndexStats, IndexStatsKey, LspIndexRequest, RepositoryUri } from '../../model'; import { GitOperations } from '../git_operations'; @@ -253,6 +256,10 @@ export class LspIndexer extends AbstractIndexer { if (error instanceof ResponseError && error.code === LanguageServerNotInstalled) { // TODO maybe need to report errors to the index task and warn user later this.log.debug(`Index symbols or references error due to language server not installed`); + } else if (error instanceof ResponseError && error.code === LanguageServerStartFailed) { + this.log.debug( + `Index symbols or references error due to language server can't be started.` + ); } else { this.log.warn(`Index symbols or references error.`); this.log.warn(error); diff --git a/x-pack/plugins/code/server/lsp/abstract_launcher.test.ts b/x-pack/plugins/code/server/lsp/abstract_launcher.test.ts index ccc6f3176c2885..d9c2eea61b8e22 100644 --- a/x-pack/plugins/code/server/lsp/abstract_launcher.test.ts +++ b/x-pack/plugins/code/server/lsp/abstract_launcher.test.ts @@ -11,7 +11,7 @@ import fs from 'fs'; import { ServerOptions } from '../server_options'; import { createTestServerOption } from '../test_utils'; -import { AbstractLauncher } from './abstract_launcher'; +import { AbstractLauncher, ServerStartFailed } from './abstract_launcher'; import { RequestExpander } from './request_expander'; import { LanguageServerProxy } from './proxy'; import { ConsoleLoggerFactory } from '../utils/console_logger_factory'; @@ -34,6 +34,8 @@ class MockLauncher extends AbstractLauncher { super(name, targetHost, opt, new ConsoleLoggerFactory()); } + protected maxRespawn = 3; + createExpander( proxy: LanguageServerProxy, builtinWorkspace: boolean, @@ -76,13 +78,13 @@ class PassiveMockLauncher extends MockLauncher { name: string, targetHost: string, opt: ServerOptions, - private dieFirstTime: boolean = false + private dieBeforeStart: number = 0 ) { super(name, targetHost, opt); } startConnect(proxy: LanguageServerProxy) { - proxy.awaitServerConnection(); + proxy.awaitServerConnection().catch(this.log.debug); } async getPort() { @@ -98,9 +100,9 @@ class PassiveMockLauncher extends MockLauncher { }); this.childProcess.send(`port ${port}`); this.childProcess.send(`host ${this.targetHost}`); - if (this.dieFirstTime) { + if (this.dieBeforeStart > 0) { this.childProcess!.send('quit'); - this.dieFirstTime = false; + this.dieBeforeStart -= 1; } else { this.childProcess!.send('connect'); } @@ -184,7 +186,7 @@ test('launcher can reconnect if process died', async () => { expect(mockMonitor.mock.calls[2][0]).toBe('socket connected'); }); await proxy.exit(); - await delay(2000); + await delay(1000); }); test('passive launcher can start and end a process', async () => { @@ -204,7 +206,7 @@ test('passive launcher can start and end a process', async () => { }); test('passive launcher should restart a process if a process died before connected', async () => { - const launcher = new PassiveMockLauncher('mock', 'localhost', options, true); + const launcher = new PassiveMockLauncher('mock', 'localhost', options, 1); const proxy = await launcher.launch(false, 1, ''); await delay(100); await retryUtil(30000, () => { @@ -216,3 +218,19 @@ test('passive launcher should restart a process if a process died before connect await proxy.exit(); await delay(1000); }); + +test('launcher should mark proxy unusable after restart 2 times', async () => { + const launcher = new PassiveMockLauncher('mock', 'localhost', options, 3); + try { + await launcher.launch(false, 1, ''); + } catch (e) { + await retryUtil(30000, () => { + expect(mockMonitor.mock.calls[0][0]).toBe('process started'); + // restart 2 times + expect(mockMonitor.mock.calls[1][0]).toBe('process started'); + expect(mockMonitor.mock.calls[2][0]).toBe('process started'); + }); + expect(e).toEqual(ServerStartFailed); + await delay(1000); + } +}); diff --git a/x-pack/plugins/code/server/lsp/abstract_launcher.ts b/x-pack/plugins/code/server/lsp/abstract_launcher.ts index 5525eedd6dfc2f..21a6d5ba83b9ac 100644 --- a/x-pack/plugins/code/server/lsp/abstract_launcher.ts +++ b/x-pack/plugins/code/server/lsp/abstract_launcher.ts @@ -5,22 +5,31 @@ */ import { ChildProcess } from 'child_process'; +import { ResponseError } from 'vscode-jsonrpc'; import { ILanguageServerLauncher } from './language_server_launcher'; import { ServerOptions } from '../server_options'; import { LoggerFactory } from '../utils/log_factory'; import { Logger } from '../log'; import { LanguageServerProxy } from './proxy'; import { RequestExpander } from './request_expander'; +import { LanguageServerStartFailed } from '../../common/lsp_error_codes'; let seqNo = 1; +export const ServerStartFailed = new ResponseError( + LanguageServerStartFailed, + 'Launch language server failed.' +); + export abstract class AbstractLauncher implements ILanguageServerLauncher { running: boolean = false; private _currentPid: number = -1; private child: ChildProcess | null = null; private _startTime: number = -1; private _proxyConnected: boolean = false; - private readonly log: Logger; + protected readonly log: Logger; + private spawnTimes: number = 0; + private launchReject?: ((reason?: any) => void); protected constructor( readonly name: string, readonly targetHost: string, @@ -49,6 +58,7 @@ export abstract class AbstractLauncher implements ILanguageServerLauncher { }); } else { child = await this.spawnProcess(installationPath, port, log); + this.spawnTimes += 1; this.child = child; log.debug('spawned a child process ' + child.pid); this._currentPid = child.pid; @@ -79,11 +89,15 @@ export abstract class AbstractLauncher implements ILanguageServerLauncher { }); proxy.listen(); this.startConnect(proxy); - await new Promise(resolve => { + await new Promise((resolve, reject) => { proxy.onConnected(() => { this._proxyConnected = true; resolve(); }); + this.launchReject = err => { + proxy.exit().catch(this.log.debug); + reject(err); + }; }); return this.createExpander(proxy, builtinWorkspace, maxWorkspace); } @@ -106,6 +120,8 @@ export abstract class AbstractLauncher implements ILanguageServerLauncher { */ protected startupTimeout = 10000; + protected maxRespawn = 3; + /** * try reconnect the proxy when disconnected */ @@ -122,19 +138,26 @@ export abstract class AbstractLauncher implements ILanguageServerLauncher { if (child && !child.killed && !processExpired()) { this.startConnect(proxy); } else { - if (child && this.running) { - this.log.debug('killing the old process.'); - await this.killProcess(child); + if (this.spawnTimes < this.maxRespawn) { + if (child && this.running) { + this.log.debug('killing the old process.'); + await this.killProcess(child); + } + const port = await this.getPort(); + proxy.changePort(port); + this.child = await this.spawnProcess(installationPath, port, this.log); + this.spawnTimes += 1; + this.log.debug('spawned a child process ' + this.child.pid); + this._currentPid = this.child.pid; + this._startTime = Date.now(); + this.running = true; + this.onProcessExit(this.child, () => this.reconnect(proxy, installationPath, child)); + this.startConnect(proxy); + } else { + this.log.warn(`spawned process ${this.spawnTimes} times, mark this proxy unusable.`); + proxy.setError(ServerStartFailed); + this.launchReject!(ServerStartFailed); } - const port = await this.getPort(); - proxy.changePort(port); - this.child = await this.spawnProcess(installationPath, port, this.log); - this.log.debug('spawned a child process ' + this.child.pid); - this._currentPid = this.child.pid; - this._startTime = Date.now(); - this.running = true; - this.onProcessExit(this.child, () => this.reconnect(proxy, installationPath, child)); - this.startConnect(proxy); } } } @@ -142,7 +165,7 @@ export abstract class AbstractLauncher implements ILanguageServerLauncher { abstract async getPort(): Promise; startConnect(proxy: LanguageServerProxy) { - proxy.connect(); + proxy.connect().catch(this.log.debug); } /** diff --git a/x-pack/plugins/code/server/lsp/controller.ts b/x-pack/plugins/code/server/lsp/controller.ts index 2b891d2ffb73bf..ed87618120cc4b 100644 --- a/x-pack/plugins/code/server/lsp/controller.ts +++ b/x-pack/plugins/code/server/lsp/controller.ts @@ -73,21 +73,17 @@ export class LanguageServerController implements ILanguageServerHandler { ); } - public async handleRequest(request: LspRequest) { + public async handleRequest(request: LspRequest): Promise { const file = request.resolvedFilePath; if (file) { // #todo add test for this const lang = await detectLanguage(file.replace('file://', '')); if (await this.repoConfigController.isLanguageDisabled(request.documentUri!, lang)) { - return Promise.reject( - new ResponseError(LanguageDisabled, `language disabled for the file`) - ); + throw new ResponseError(LanguageDisabled, `language disabled for the file`); } - return this.dispatchRequest(lang, request); + return await this.dispatchRequest(lang, request); } else { - return Promise.reject( - new ResponseError(UnknownErrorCode, `can't detect language without a file`) - ); + throw new ResponseError(UnknownErrorCode, `can't detect language without a file`); } } @@ -102,19 +98,17 @@ export class LanguageServerController implements ILanguageServerHandler { this.installManager.installationPath(ls.definition) ); } - const handler = ls.languageServerHandlers as Promise; - return (await handler).handleRequest(request); + const handler = await (ls.languageServerHandlers as Promise); + return await handler.handleRequest(request); } else { const handler = await this.findOrCreateHandler(ls, request); handler.lastAccess = Date.now(); - return handler.handleRequest(request); + return await handler.handleRequest(request); } } else { - return Promise.reject( - new ResponseError( - UnknownFileLanguage, - `can't detect language from file:${request.resolvedFilePath}` - ) + throw new ResponseError( + UnknownFileLanguage, + `can't detect language from file:${request.resolvedFilePath}` ); } } diff --git a/x-pack/plugins/code/server/lsp/java_launcher.ts b/x-pack/plugins/code/server/lsp/java_launcher.ts index 8fb796be765512..877d5c374b6b4a 100644 --- a/x-pack/plugins/code/server/lsp/java_launcher.ts +++ b/x-pack/plugins/code/server/lsp/java_launcher.ts @@ -40,7 +40,7 @@ export class JavaLauncher extends AbstractLauncher { } startConnect(proxy: LanguageServerProxy) { - proxy.awaitServerConnection(); + proxy.awaitServerConnection().catch(this.log.debug); } async getPort(): Promise { diff --git a/x-pack/plugins/code/server/lsp/proxy.ts b/x-pack/plugins/code/server/lsp/proxy.ts index 1f462d66188561..aa05afcb49acfb 100644 --- a/x-pack/plugins/code/server/lsp/proxy.ts +++ b/x-pack/plugins/code/server/lsp/proxy.ts @@ -42,6 +42,7 @@ export interface ILanguageServerHandler { } export class LanguageServerProxy implements ILanguageServerHandler { + private error: any | null = null; public get isClosed() { return this.closed; } @@ -78,6 +79,9 @@ export class LanguageServerProxy implements ILanguageServerHandler { } public receiveRequest(method: string, params: any, isNotification: boolean = false) { + if (this.error) { + return Promise.reject(this.error); + } const message: RequestMessage = { jsonrpc: '2.0', id: this.sequenceNumber++, @@ -105,6 +109,9 @@ export class LanguageServerProxy implements ILanguageServerHandler { workspaceFolders: [WorkspaceFolder], initOptions?: object ): Promise { + if (this.error) { + throw this.error; + } const clientConn = await this.tryConnect(); const rootUri = workspaceFolders[0].uri; const params = { @@ -200,9 +207,9 @@ export class LanguageServerProxy implements ILanguageServerHandler { server.removeListener('error', rej); this.logger.info('Wait langserver connection on port ' + this.targetPort); }); - onCancel!(() => { + onCancel!(error => { server.close(); - rej('canceled'); + rej(error); }); }); } @@ -235,7 +242,7 @@ export class LanguageServerProxy implements ILanguageServerHandler { } this.closed = false; if (!this.connectingPromise) { - this.connectingPromise = new Cancelable((resolve, reject, onCancel) => { + this.connectingPromise = new Cancelable(resolve => { this.socket = new net.Socket(); this.socket.on('connect', () => { @@ -257,9 +264,6 @@ export class LanguageServerProxy implements ILanguageServerHandler { this.targetPort, this.targetHost ); - onCancel!(() => { - reject('canceled'); - }); }); } return this.connectingPromise.promise; @@ -322,4 +326,12 @@ export class LanguageServerProxy implements ILanguageServerHandler { } } } + + public setError(error: any) { + if (this.connectingPromise) { + this.connectingPromise.cancel(error); + this.connectingPromise = null; + } + this.error = error; + } } diff --git a/x-pack/plugins/code/server/routes/lsp.ts b/x-pack/plugins/code/server/routes/lsp.ts index ae272d8bdf6496..7149751a383d38 100644 --- a/x-pack/plugins/code/server/routes/lsp.ts +++ b/x-pack/plugins/code/server/routes/lsp.ts @@ -10,7 +10,11 @@ import { groupBy, last } from 'lodash'; import { ResponseError } from 'vscode-jsonrpc'; import { ResponseMessage } from 'vscode-jsonrpc/lib/messages'; import { Location } from 'vscode-languageserver-types'; -import { ServerNotInitialized, UnknownFileLanguage } from '../../common/lsp_error_codes'; +import { + ServerNotInitialized, + UnknownFileLanguage, + LanguageServerStartFailed, +} from '../../common/lsp_error_codes'; import { parseLspUrl } from '../../common/uri_util'; import { GitOperations } from '../git_operations'; import { Logger } from '../log'; @@ -51,7 +55,11 @@ export function lspRoute( } catch (error) { if (error instanceof ResponseError) { // hide some errors; - if (error.code !== UnknownFileLanguage || error.code !== ServerNotInitialized) { + if ( + error.code !== UnknownFileLanguage || + error.code !== ServerNotInitialized || + error.code !== LanguageServerStartFailed + ) { log.debug(error); } return h diff --git a/x-pack/plugins/code/server/utils/cancelable.ts b/x-pack/plugins/code/server/utils/cancelable.ts index d0d1ca1198ee29..eb3ba9fc832e9e 100644 --- a/x-pack/plugins/code/server/utils/cancelable.ts +++ b/x-pack/plugins/code/server/utils/cancelable.ts @@ -5,7 +5,7 @@ */ type Resolve = (t: T) => void; type Reject = (error: any) => void; -type Cancel = () => void; +type Cancel = (error: any) => void; type OnCancel = (cancel: Cancel) => void; export class Cancelable { @@ -24,9 +24,17 @@ export class Cancelable { }); } - public cancel(): void { + public cancel(error = 'canceled'): void { if (this._cancel) { - this._cancel(); + this._cancel(error); + } else if (this.reject) { + this.reject(error); + } + } + + public error(error: any) { + if (this.reject) { + this.reject(error); } } }