diff --git a/src/conpty_console_list_agent.ts b/src/conpty_console_list_agent.ts index 181ccabb..68fc5d4b 100644 --- a/src/conpty_console_list_agent.ts +++ b/src/conpty_console_list_agent.ts @@ -10,6 +10,14 @@ import { loadNativeModule } from './utils'; const getConsoleProcessList = loadNativeModule('conpty_console_list').module.getConsoleProcessList; const shellPid = parseInt(process.argv[2], 10); -const consoleProcessList = getConsoleProcessList(shellPid); +let consoleProcessList: number[] = []; +if (shellPid > 0) { + try { + consoleProcessList = getConsoleProcessList(shellPid); + } catch { + // AttachConsole can fail if the process already exited or is invalid. + consoleProcessList = []; + } +} process.send!({ consoleProcessList }); process.exit(0); diff --git a/src/windowsPtyAgent.test.ts b/src/windowsPtyAgent.test.ts index dc2104b3..ccc34acd 100644 --- a/src/windowsPtyAgent.test.ts +++ b/src/windowsPtyAgent.test.ts @@ -4,7 +4,7 @@ */ import * as assert from 'assert'; -import { argsToCommandLine } from './windowsPtyAgent'; +import { argsToCommandLine, WindowsPtyAgent } from './windowsPtyAgent'; function check(file: string, args: string | string[], expected: string): void { assert.equal(argsToCommandLine(file, args), expected); @@ -91,4 +91,117 @@ if (process.platform === 'win32') { }); }); }); + + describe('WindowsPtyAgent', () => { + describe('connection timing (issue #763)', () => { + it('should defer conptyNative.connect() until worker is ready', function (done) { + this.timeout(10000); + + 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 + ); + + // The innerPid should be 0 initially since connect() is deferred + // until the worker signals ready. This verifies the fix for #763. + const initialPid = term.innerPid; + + // Wait for the connection to complete via ready_datapipe event + term.outSocket.on('ready_datapipe', () => { + // After worker is ready and connect() is called, innerPid should be set + // Use a small delay to ensure _completePtyConnection has run + setTimeout(() => { + assert.notStrictEqual(term.innerPid, 0, 'innerPid should be set after worker is ready'); + assert.strictEqual(initialPid, 0, 'innerPid should have been 0 before worker was ready'); + term.kill(); + done(); + }, 100); + }); + }); + + it('should successfully spawn a process after deferred connection', function (done) { + this.timeout(10000); + + const term = new WindowsPtyAgent( + 'cmd.exe', + '/c echo hello', + Object.keys(process.env).map(k => `${k}=${process.env[k]}`), + process.cwd(), + 80, + 30, + false, + false, + false + ); + + let output = ''; + term.outSocket.on('data', (data: string) => { + output += data; + }); + + // Wait for process to complete and verify output + setTimeout(() => { + assert.ok(output.includes('hello'), `Expected output to contain "hello", got: ${output}`); + term.kill(); + done(); + }, 2000); + }); + + it('should allow async work between construction and connection (non-blocking)', function (done) { + this.timeout(10000); + + // Track the sequence of events to verify non-blocking behavior + const events: string[] = []; + + 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 + ); + + events.push('constructor_returned'); + assert.strictEqual(term.innerPid, 0, 'innerPid should be 0 immediately after construction'); + + // Schedule async work - this MUST run before ready_datapipe if constructor is non-blocking + setImmediate(() => { + events.push('setImmediate_ran'); + // innerPid might still be 0 or might be set by now, depending on timing + // The key is that setImmediate ran, proving the event loop wasn't blocked + }); + + term.outSocket.on('ready_datapipe', () => { + events.push('ready_datapipe'); + + setTimeout(() => { + events.push('final_check'); + + // Verify the sequence: constructor returned, then async work could run + assert.ok(events.includes('constructor_returned'), 'constructor should have returned'); + assert.ok(events.includes('setImmediate_ran'), 'setImmediate should have run (event loop not blocked)'); + assert.ok(events.indexOf('constructor_returned') < events.indexOf('setImmediate_ran'), + 'constructor should return before setImmediate runs'); + + // Most importantly: innerPid should now be set + assert.notStrictEqual(term.innerPid, 0, 'innerPid should be set after connection'); + + term.kill(); + done(); + }, 100); + }); + }); + }); + }); } diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 277594d3..cdc80415 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -42,6 +42,8 @@ export class WindowsPtyAgent { public get innerPid(): number { return this._innerPid; } public get pty(): number { return this._pty; } + private _pendingPtyInfo: { pty: number, commandLine: string, cwd: string, env: string[] } | undefined; + constructor( file: string, args: ArgvOrCommandLine, @@ -78,9 +80,29 @@ export class WindowsPtyAgent { this._outSocket = new Socket(); this._outSocket.setEncoding('utf8'); // The conout socket must be ready out on another thread to avoid deadlocks + // 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); + + // 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); + this._conoutSocketWorker.onReady(() => { + clearTimeout(connectionTimeout); 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._outSocket.on('connect', () => { this._outSocket.emit('ready_datapipe'); @@ -93,8 +115,16 @@ export class WindowsPtyAgent { writable: true }); this._inSocket.setEncoding('utf8'); + } - const connect = conptyNative.connect(this._pty, commandLine, cwd, env, this._useConptyDll, c => this._$onProcessExit(c)); + private _completePtyConnection(): void { + if (!this._pendingPtyInfo) { + return; + } + const { pty, commandLine, cwd, env } = this._pendingPtyInfo; + this._pendingPtyInfo = undefined; + + const connect = conptyNative.connect(pty, commandLine, cwd, env, this._useConptyDll, c => this._$onProcessExit(c)); this._innerPid = connect.pid; } @@ -110,6 +140,9 @@ export class WindowsPtyAgent { } public kill(): void { + // Prevent deferred connection from completing after kill + this._pendingPtyInfo = undefined; + // Tell the agent to kill the pty, this releases handles to the process if (!this._useConptyDll) { this._inSocket.readable = false; @@ -136,6 +169,9 @@ export class WindowsPtyAgent { } private _getConsoleProcessList(): Promise { + if (this._innerPid <= 0) { + return Promise.resolve([]); + } return new Promise(resolve => { const agent = fork(path.join(__dirname, 'conpty_console_list_agent'), [ this._innerPid.toString() ]); agent.on('message', message => { diff --git a/src/windowsTerminal.test.ts b/src/windowsTerminal.test.ts index 29f0411d..28c102bc 100644 --- a/src/windowsTerminal.test.ts +++ b/src/windowsTerminal.test.ts @@ -104,25 +104,69 @@ if (process.platform === 'win32') { it('should kill the process tree', function (done: Mocha.Done): void { this.timeout(20000); const term = new WindowsTerminal('cmd.exe', [], { useConptyDll }); - // Start sub-processes - term.write('powershell.exe\r'); - term.write('node.exe\r'); - console.log('start poll for tree size'); - pollForProcessTreeSize(term.pid, 3, 500, 5000).then(list => { - assert.strictEqual(list[0].name.toLowerCase(), 'cmd.exe'); - assert.strictEqual(list[1].name.toLowerCase(), 'powershell.exe'); - assert.strictEqual(list[2].name.toLowerCase(), 'node.exe'); - term.kill(); - const desiredState: IProcessState = {}; - desiredState[list[0].pid] = false; - desiredState[list[1].pid] = false; - desiredState[list[2].pid] = false; - term.on('exit', () => { - pollForProcessState(desiredState, 1000, 5000).then(() => { - done(); - }).catch(done); - }); - }).catch(done); + const socket = (term as any)._socket; + let started = false; + const startPolling = (): void => { + if (started) { + return; + } + if (term.pid === 0) { + setTimeout(startPolling, 50); + return; + } + started = true; + // Start sub-processes + term.write('powershell.exe\r'); + term.write('node.exe\r'); + console.log('start poll for tree size'); + pollForProcessTreeSize(term.pid, 3, 500, 5000).then(list => { + assert.strictEqual(list[0].name.toLowerCase(), 'cmd.exe'); + assert.strictEqual(list[1].name.toLowerCase(), 'powershell.exe'); + assert.strictEqual(list[2].name.toLowerCase(), 'node.exe'); + term.kill(); + const desiredState: IProcessState = {}; + desiredState[list[0].pid] = false; + desiredState[list[1].pid] = false; + desiredState[list[2].pid] = false; + term.on('exit', () => { + pollForProcessState(desiredState, 1000, 5000).then(() => { + done(); + }).catch(done); + }); + }).catch(done); + }; + + if (term.pid > 0) { + startPolling(); + } else { + socket.once('ready_datapipe', () => setTimeout(startPolling, 50)); + } + }); + }); + + describe('pid', () => { + it('should be 0 before ready and set after ready_datapipe (issue #763)', function (done) { + this.timeout(10000); + const term = new WindowsTerminal('cmd.exe', '/c echo test', { useConptyDll }); + + // pid may be 0 immediately after construction due to deferred connection + const initialPid = term.pid; + + // Access internal socket to listen for ready_datapipe + const socket = (term as any)._socket; + socket.on('ready_datapipe', () => { + // After ready_datapipe, pid should be set to a valid non-zero value + setTimeout(() => { + assert.notStrictEqual(term.pid, 0, 'pid should be set after ready_datapipe'); + assert.strictEqual(typeof term.pid, 'number', 'pid should be a number'); + // If initial was 0, it should now be different (proves the fix works) + if (initialPid === 0) { + assert.notStrictEqual(term.pid, initialPid, 'pid should be updated from initial value'); + } + term.on('exit', () => done()); + term.kill(); + }, 100); + }); }); }); diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index 8516750b..0553c333 100644 --- a/src/windowsTerminal.ts +++ b/src/windowsTerminal.ts @@ -59,6 +59,8 @@ export class WindowsTerminal extends Terminal { // The forked windows terminal is not available until `ready` event is // emitted. this._socket.on('ready_datapipe', () => { + // Update pid now that the agent has connected + this._pid = this._agent.innerPid; // Run deferreds and set ready state once the first data event is received. this._socket.once('data', () => {