diff --git a/src/windowsConoutConnection.ts b/src/windowsConoutConnection.ts index fa2d62de..aedf8ec9 100644 --- a/src/windowsConoutConnection.ts +++ b/src/windowsConoutConnection.ts @@ -28,14 +28,24 @@ const FLUSH_DATA_INTERVAL = 1000; * - https://github.com/microsoft/terminal/issues/1810 * - https://docs.microsoft.com/en-us/windows/console/closepseudoconsole */ -export class ConoutConnection implements IDisposable { +export interface IConoutConnection extends IDisposable { + readonly onReady: IEvent; + readonly onError: IEvent; + connectSocket(socket: Socket): void; +} + +export class ConoutConnection implements IConoutConnection { private _worker: Worker; private _drainTimeout: NodeJS.Timeout | undefined; private _isDisposed: boolean = false; + private _isReady: boolean = false; private _onReady = new EventEmitter2(); public get onReady(): IEvent { return this._onReady.event; } + private _onError = new EventEmitter2(); + public get onError(): IEvent { return this._onError.event; } + constructor( private _conoutPipeName: string, private _useConptyDll: boolean @@ -48,12 +58,23 @@ export class ConoutConnection implements IDisposable { this._worker.on('message', (message: ConoutWorkerMessage) => { switch (message) { case ConoutWorkerMessage.READY: + this._isReady = true; this._onReady.fire(); return; default: console.warn('Unexpected ConoutWorkerMessage', message); } }); + this._worker.on('error', error => { + if (!this._isDisposed && !this._isReady) { + this._onError.fire(error); + } + }); + this._worker.on('exit', code => { + if (!this._isDisposed && !this._isReady) { + this._onError.fire(new Error(`Conout worker exited before connecting (code ${code})`)); + } + }); } dispose(): void { diff --git a/src/windowsPtyAgent.test.ts b/src/windowsPtyAgent.test.ts index ccc34acd..9d697950 100644 --- a/src/windowsPtyAgent.test.ts +++ b/src/windowsPtyAgent.test.ts @@ -4,12 +4,50 @@ */ import * as assert from 'assert'; -import { argsToCommandLine, WindowsPtyAgent } from './windowsPtyAgent'; +import { Socket } from 'net'; +import { EventEmitter2, IEvent } from './eventEmitter2'; +import { argsToCommandLine, IWindowsPtyAgentOptions, WindowsPtyAgent } from './windowsPtyAgent'; +import { IConoutConnection } from './windowsConoutConnection'; function check(file: string, args: string | string[], expected: string): void { assert.equal(argsToCommandLine(file, args), expected); } +class TestConoutConnection implements IConoutConnection { + private readonly _onReady = new EventEmitter2(); + public get onReady(): IEvent { return this._onReady.event; } + + private readonly _onError = new EventEmitter2(); + public get onError(): IEvent { return this._onError.event; } + + public connectSocketCallCount = 0; + public isDisposed = false; + + public connectSocket(socket: Socket): void { + void socket; + this.connectSocketCallCount++; + } + + public dispose(): void { + this.isDisposed = true; + } + + public fireReady(): void { + this._onReady.fire(); + } + + public fireError(error: Error): void { + this._onError.fire(error); + } +} + +function createTestAgentOptions(connection: IConoutConnection, connectionTimeout: number): IWindowsPtyAgentOptions { + return { + connectionTimeout, + conoutConnectionFactory: () => connection + }; +} + if (process.platform === 'win32') { describe('argsToCommandLine', () => { describe('Plain strings', () => { @@ -94,6 +132,87 @@ if (process.platform === 'win32') { describe('WindowsPtyAgent', () => { describe('connection timing (issue #763)', () => { + it('should fail without connecting when the worker times out', async function () { + this.timeout(10000); + const connection = new TestConoutConnection(); + const term = new WindowsPtyAgent( + 'cmd.exe', + '/c echo test', + Object.keys(process.env).map(k => `${k}=${process.env[k]}`), + process.cwd(), + 80, + 30, + false, + false, + false, + createTestAgentOptions(connection, 10) + ); + + let eventLoopResponsive = false; + setImmediate(() => eventLoopResponsive = true); + const error = await new Promise(resolve => term.onError(resolve)); + + assert.strictEqual(error.message, 'Timed out waiting for ConPTY output worker'); + assert.strictEqual(eventLoopResponsive, true, 'event loop should remain responsive'); + assert.strictEqual(connection.connectSocketCallCount, 0); + assert.strictEqual(connection.isDisposed, true); + assert.strictEqual(term.innerPid, 0); + + connection.fireReady(); + assert.strictEqual(connection.connectSocketCallCount, 0, 'late readiness must be ignored'); + }); + + it('should fail when the worker errors before becoming ready', async () => { + const connection = new TestConoutConnection(); + const term = new WindowsPtyAgent( + 'cmd.exe', + '/c echo test', + Object.keys(process.env).map(k => `${k}=${process.env[k]}`), + process.cwd(), + 80, + 30, + false, + false, + false, + createTestAgentOptions(connection, 1000) + ); + + const expectedError = new Error('worker failed'); + const errorPromise = new Promise(resolve => term.onError(resolve)); + connection.fireError(expectedError); + const error = await errorPromise; + + assert.strictEqual(error, expectedError); + assert.strictEqual(connection.connectSocketCallCount, 0); + assert.strictEqual(connection.isDisposed, true); + assert.strictEqual(term.innerPid, 0); + }); + + it('should ignore worker events after kill before readiness', () => { + const connection = new TestConoutConnection(); + const term = new WindowsPtyAgent( + 'cmd.exe', + '/c echo test', + Object.keys(process.env).map(k => `${k}=${process.env[k]}`), + process.cwd(), + 80, + 30, + false, + false, + false, + createTestAgentOptions(connection, 1000) + ); + let errorCount = 0; + term.onError(() => errorCount++); + + term.kill(); + connection.fireReady(); + connection.fireError(new Error('late error')); + + assert.strictEqual(connection.connectSocketCallCount, 0); + assert.strictEqual(errorCount, 0); + }); + it('should defer conptyNative.connect() until worker is ready', function (done) { this.timeout(10000); diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 06450864..73f83d6e 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -9,7 +9,7 @@ import * as path from 'path'; import { fork } from 'child_process'; import { Socket } from 'net'; import { ArgvOrCommandLine } from './types'; -import { ConoutConnection } from './windowsConoutConnection'; +import { ConoutConnection, IConoutConnection } from './windowsConoutConnection'; import { EventEmitter2, IEvent } from './eventEmitter2'; import { loadNativeModule } from './utils'; @@ -21,6 +21,17 @@ let conptyNative: IConptyNative; * has started. */ const FLUSH_DATA_INTERVAL = 1000; +const CONNECTION_TIMEOUT = 5000; + +export interface IWindowsPtyAgentOptions { + readonly connectionTimeout: number; + readonly conoutConnectionFactory: (conoutPipeName: string, useConptyDll: boolean) => IConoutConnection; +} + +const defaultWindowsPtyAgentOptions: IWindowsPtyAgentOptions = { + connectionTimeout: CONNECTION_TIMEOUT, + conoutConnectionFactory: (conoutPipeName, useConptyDll) => new ConoutConnection(conoutPipeName, useConptyDll) +}; /** * This agent sits between the WindowsTerminal class and provides an interface for conpty. @@ -30,8 +41,9 @@ export class WindowsPtyAgent { private _outSocket: Socket; private _innerPid: number = 0; private _closeTimeout: NodeJS.Timer | undefined; + private _connectionTimeout: NodeJS.Timeout | undefined; private _exitCode: number | undefined; - private _conoutSocketWorker: ConoutConnection; + private _conoutSocketWorker: IConoutConnection; private _onError = new EventEmitter2(); public get onError(): IEvent { return this._onError.event; } @@ -57,7 +69,8 @@ export class WindowsPtyAgent { rows: number, debug: boolean, private _useConptyDll: boolean = false, - conptyInheritCursor: boolean = false + conptyInheritCursor: boolean = false, + options: IWindowsPtyAgentOptions = defaultWindowsPtyAgentOptions ) { if (!conptyNative) { conptyNative = loadNativeModule('conpty').module; @@ -87,27 +100,29 @@ export class WindowsPtyAgent { // We must wait for the worker to connect before calling conptyNative.connect() // to avoid blocking the Node.js event loop in ConnectNamedPipe. // See https://github.com/microsoft/node-pty/issues/763 - this._conoutSocketWorker = new ConoutConnection(term.conout, this._useConptyDll); + this._conoutSocketWorker = options.conoutConnectionFactory(term.conout, this._useConptyDll); // Store pending connection info - we'll complete the connection when worker is ready this._pendingPtyInfo = { pty: this._pty, commandLine, cwd, env }; - // Timeout to ensure connection completes even if worker fails to signal ready - const connectionTimeout = setTimeout(() => { - if (this._pendingPtyInfo) { - // Worker never signaled ready - complete connection anyway to avoid zombie state - this._completePtyConnection(); - } - }, 5000); + // Never call connect() before the worker is ready, as ConnectNamedPipe + // would block the Node.js event loop while waiting for the output client. + this._connectionTimeout = setTimeout(() => { + this._failPtyConnection(new Error('Timed out waiting for ConPTY output worker')); + }, options.connectionTimeout); this._conoutSocketWorker.onReady(() => { - clearTimeout(connectionTimeout); + if (!this._pendingPtyInfo) { + return; + } + this._clearConnectionTimeout(); this._conoutSocketWorker.connectSocket(this._outSocket); // Now that the worker has connected to the output pipe, we can safely call // conptyNative.connect() which calls ConnectNamedPipe - it won't block because // the client (worker) is already connected this._completePtyConnection(); }); + this._conoutSocketWorker.onError(error => this._failPtyConnection(error)); this._outSocket.on('connect', () => { this._outSocket.emit('ready_datapipe'); }); @@ -125,6 +140,7 @@ export class WindowsPtyAgent { if (!this._pendingPtyInfo) { return; } + this._clearConnectionTimeout(); const { pty, commandLine, cwd, env } = this._pendingPtyInfo; this._pendingPtyInfo = undefined; @@ -132,8 +148,8 @@ export class WindowsPtyAgent { const connect = conptyNative.connect(pty, commandLine, cwd, env, this._useConptyDll, c => this._$onProcessExit(c)); this._innerPid = connect.pid; } catch (err) { - // connect() runs from the conout worker's onReady callback (or its - // timeout fallback), so a throw here would otherwise surface as an + // connect() runs from the conout worker's onReady callback, so a throw + // here would otherwise surface as an // uncaughtException with no way for the consumer to observe it. const code = /error code: (\d+)/.exec((err as Error).message)?.[1]; this._exitCode = code ? parseInt(code, 10) : -1; @@ -145,6 +161,27 @@ export class WindowsPtyAgent { } } + private _failPtyConnection(error: Error): void { + if (!this._pendingPtyInfo) { + return; + } + this._clearConnectionTimeout(); + this._pendingPtyInfo = undefined; + this._exitCode = -1; + try { this._ptyNative.kill(this._pty, this._useConptyDll); } catch { /* already gone */ } + this._conoutSocketWorker.dispose(); + this._inSocket.destroy(); + this._outSocket.destroy(); + this._onError.fire(error); + } + + private _clearConnectionTimeout(): void { + if (this._connectionTimeout) { + clearTimeout(this._connectionTimeout); + this._connectionTimeout = undefined; + } + } + public resize(cols: number, rows: number): void { if (this._exitCode !== undefined) { throw new Error('Cannot resize a pty that has already exited'); @@ -158,6 +195,7 @@ export class WindowsPtyAgent { public kill(): void { // Prevent deferred connection from completing after kill + this._clearConnectionTimeout(); this._pendingPtyInfo = undefined; // Tell the agent to kill the pty, this releases handles to the process