client: reject start() if server process exits before connecting on pipe/socket transport

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', () => {

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);

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<MessageTransports> {
+	return new Promise<MessageTransports>((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<MessageTransports>(message);