diff --git a/client-node-tests/src/integration.test.ts b/client-node-tests/src/integration.test.ts index 29e304a97..afa2afc14 100644 --- a/client-node-tests/src/integration.test.ts +++ b/client-node-tests/src/integration.test.ts @@ -309,6 +309,69 @@ suite('Socket transport', () => { // would time out. Reaching this line proves the actual bound port was handed over. await client.stop(); }); + + test('start() rejects if the server process exits before connecting', async () => { + const serverOptions: lsclient.ServerOptions = { + module: path.join(__dirname, './servers/exitBeforeConnectServer.js'), + transport: { kind: lsclient.TransportKind.socket, port: 0 }, + }; + const clientOptions: lsclient.LanguageClientOptions = {}; + const client = new lsclient.LanguageClient('socket exit before connect', 'Socket Transport (exits before connecting)', serverOptions, clientOptions); + // Before the fix start() would hang forever since the client only waited on + // the socket connection and never observed the server process exiting first. + await assert.rejects(async () => { + await client.start(); + }, /Server process exited before the connection could be established/); + }).timeout(5000); + + test('start() rejects if the executable server process exits before connecting', async () => { + const serverOptions: lsclient.ServerOptions = { + command: process.execPath, + args: [path.join(__dirname, './servers/exitBeforeConnectServer.js')], + // The tests run inside Electron, so process.execPath must be told to behave like node. + options: { env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' } }, + transport: { kind: lsclient.TransportKind.socket, port: 0 }, + }; + const clientOptions: lsclient.LanguageClientOptions = {}; + const client = new lsclient.LanguageClient('socket executable exit before connect', 'Socket Transport (executable exits before connecting)', serverOptions, clientOptions); + // Covers the Executable branch (the command + socket path from #1586), which is separate from the NodeModule branch above. + await assert.rejects(async () => { + await client.start(); + }, /Server process exited before the connection could be established/); + }).timeout(5000); +}); + +suite('Pipe transport', () => { + + test('start() rejects if the server process exits before connecting', async () => { + const serverOptions: lsclient.ServerOptions = { + module: path.join(__dirname, './servers/exitBeforeConnectServer.js'), + transport: lsclient.TransportKind.pipe, + }; + const clientOptions: lsclient.LanguageClientOptions = {}; + const client = new lsclient.LanguageClient('pipe exit before connect', 'Pipe Transport (exits before connecting)', serverOptions, clientOptions); + // Same underlying bug as the socket transport case: start() would hang forever + // instead of surfacing the server process exiting before it connected the pipe. + await assert.rejects(async () => { + await client.start(); + }, /Server process exited before the connection could be established/); + }).timeout(5000); + + test('start() rejects if the executable server process exits before connecting', async () => { + const serverOptions: lsclient.ServerOptions = { + command: process.execPath, + args: [path.join(__dirname, './servers/exitBeforeConnectServer.js')], + // The tests run inside Electron, so process.execPath must be told to behave like node. + options: { env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' } }, + transport: lsclient.TransportKind.pipe, + }; + const clientOptions: lsclient.LanguageClientOptions = {}; + const client = new lsclient.LanguageClient('pipe executable exit before connect', 'Pipe Transport (executable exits before connecting)', serverOptions, clientOptions); + // Covers the Executable branch (the command + pipe path from #1586), which is separate from the NodeModule branch above. + await assert.rejects(async () => { + await client.start(); + }, /Server process exited before the connection could be established/); + }).timeout(5000); }); suite('Client integration', () => { diff --git a/client-node-tests/src/servers/exitBeforeConnectServer.ts b/client-node-tests/src/servers/exitBeforeConnectServer.ts new file mode 100644 index 000000000..2ecadac57 --- /dev/null +++ b/client-node-tests/src/servers/exitBeforeConnectServer.ts @@ -0,0 +1,6 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +process.exit(42); diff --git a/client/src/node/main.ts b/client/src/node/main.ts index 3c64f7e87..a9606e76b 100644 --- a/client/src/node/main.ts +++ b/client/src/node/main.ts @@ -16,7 +16,7 @@ import * as Is from '../common/utils/is'; import { BaseLanguageClient, LanguageClientOptions as BaseLanguageClientOptions, MessageTransports, ShutdownMode } from '../common/client'; import { terminate } from './processes'; -import { StreamMessageReader, StreamMessageWriter, IPCMessageReader, IPCMessageWriter, createClientPipeTransport, generateRandomPipeName, createClientSocketTransport, InitializeParams} from 'vscode-languageserver-protocol/node'; +import { StreamMessageReader, StreamMessageWriter, IPCMessageReader, IPCMessageWriter, createClientPipeTransport, generateRandomPipeName, createClientSocketTransport, InitializeParams, MessageReader, MessageWriter} from 'vscode-languageserver-protocol/node'; // Import SemVer functions individually to avoid circular dependencies in SemVer import semverParse = require('semver/functions/parse'); @@ -420,9 +420,7 @@ export class LanguageClient extends BaseLanguageClient { this._serverProcess = process; pipeStderr(process.stderr, this.outputChannel); pipeStdout(process.stdout, this.outputChannel); - return transport.onConnected().then((protocol) => { - return { reader: protocol[0], writer: protocol[1] }; - }); + return waitForConnection(process, transport); }); } else if (Transport.isSocket(transport)) { return createClientSocketTransport(transport.port).then((transport) => { @@ -435,9 +433,7 @@ export class LanguageClient extends BaseLanguageClient { this._serverProcess = process; pipeStderr(process.stderr, this.outputChannel); pipeStdout(process.stdout, this.outputChannel); - return transport.onConnected().then((protocol) => { - return { reader: protocol[0], writer: protocol[1] }; - }); + return waitForConnection(process, transport); }); } } else { @@ -476,9 +472,7 @@ export class LanguageClient extends BaseLanguageClient { this._serverProcess = sp; pipeStderr(sp.stderr, this.outputChannel); pipeStdout(sp.stdout, this.outputChannel); - transport.onConnected().then((protocol) => { - resolve({ reader: protocol[0], writer: protocol[1] }); - }, reject); + waitForConnection(sp, transport).then(resolve, reject); }, reject); } else if (Transport.isSocket(transport)) { createClientSocketTransport(transport.port).then((transport) => { @@ -488,9 +482,7 @@ export class LanguageClient extends BaseLanguageClient { this._serverProcess = sp; pipeStderr(sp.stderr, this.outputChannel); pipeStdout(sp.stdout, this.outputChannel); - transport.onConnected().then((protocol) => { - resolve({ reader: protocol[0], writer: protocol[1] }); - }, reject); + waitForConnection(sp, transport).then(resolve, reject); }, reject); } }); @@ -530,9 +522,7 @@ export class LanguageClient extends BaseLanguageClient { this._isDetached = !!options.detached; pipeStderr(serverProcess.stderr, this.outputChannel); pipeStdout(serverProcess.stdout, this.outputChannel); - return transport.onConnected().then((protocol) => { - return { reader: protocol[0], writer: protocol[1] }; - }); + return waitForConnection(serverProcess, transport); }); } else if (Transport.isSocket(transport)) { return createClientSocketTransport(transport.port).then((transport) => { @@ -546,9 +536,7 @@ export class LanguageClient extends BaseLanguageClient { this._isDetached = !!options.detached; pipeStderr(serverProcess.stderr, this.outputChannel); pipeStdout(serverProcess.stdout, this.outputChannel); - return transport.onConnected().then((protocol) => { - return { reader: protocol[0], writer: protocol[1] }; - }); + return waitForConnection(serverProcess, transport); }); } } @@ -652,6 +640,36 @@ export class SettingMonitor { } } +function waitForConnection(childProcess: ChildProcess, transport: { onConnected(): Promise<[MessageReader, MessageWriter]>; close(): void }): Promise { + return new Promise((resolve, reject) => { + const removeListeners = () => { + childProcess.removeListener('exit', onExit); + childProcess.removeListener('error', onError); + }; + const onExit = (code: number | null, signal: NodeJS.Signals | null) => { + removeListeners(); + // The server never connected, so stop listening to release the pipe / port. + transport.close(); + reject(new Error(`Server process exited before the connection could be established (code: ${code}, signal: ${signal}).`)); + }; + // A failed spawn (e.g. invalid execPath) emits 'error' and never 'exit'. + const onError = (error: Error) => { + removeListeners(); + transport.close(); + reject(error); + }; + childProcess.once('exit', onExit); + childProcess.once('error', onError); + transport.onConnected().then((protocol) => { + removeListeners(); + resolve({ reader: protocol[0], writer: protocol[1] }); + }, (error) => { + removeListeners(); + reject(error); + }); + }); +} + function handleChildProcessStartError(process: ChildProcess, message: string) { if (process === null) { return Promise.reject(message);