From e430d82efc2378de91a07c27a183a60ab468d426 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 9 Jun 2020 08:32:20 -0700 Subject: [PATCH 1/5] Host conout socket in a worker Conpty can send a final screen update after ClosePseudoConsole which waits for the socket to be drained before continuing. When both the socket draining and closing occurs on the same thread a deadlock can occur which locks up VS Code's UI, the only ways to fix are to close the client or kill the misbehaving conhost instance. This change moves the handling of the conout named pipe to a Worker which bumps the minimum Node version requirement to 12 (10 with a flag). This change may also have positive performance benefits as called out in microsoft/vscode#74620 The worker change also happens for winpty, it seems to work there too. Fixes #375 --- README.md | 11 ++--- package.json | 2 +- src/shared/conout.ts | 15 +++++++ src/terminal.ts | 1 - src/windowsConoutConnection.ts | 78 ++++++++++++++++++++++++++++++++++ src/windowsPtyAgent.ts | 19 ++++----- src/worker/conoutWorker.ts | 18 ++++++++ test/spam-close.js | 33 ++++++++++++++ yarn.lock | 8 ++-- 9 files changed, 162 insertions(+), 23 deletions(-) create mode 100644 src/shared/conout.ts create mode 100644 src/windowsConoutConnection.ts create mode 100644 src/worker/conoutWorker.ts create mode 100644 test/spam-close.js diff --git a/README.md b/README.md index 0a77e20ca..0de001810 100644 --- a/README.md +++ b/README.md @@ -69,16 +69,14 @@ npm run build ## Dependencies -### Linux/Ubuntu +Node.JS 12+ is required to use `node-pty`, v10 also works but requires the workers flag to be enabled on Windows (`node --experimental-worker`). -``` +### Linux (apt) + +```sh sudo apt install -y make python build-essential ``` -The following are also needed: - -- Node.JS 10+ - ### macOS Xcode is needed to compile the sources, this can be installed from the App Store. @@ -94,7 +92,6 @@ npm install --global --production windows-build-tools The following are also needed: - [Windows SDK](https://developer.microsoft.com/en-us/windows/downloads/windows-10-sdk) - only the "Desktop C++ Apps" components are needed to be installed -- Node.JS 10+ ## Debugging diff --git a/package.json b/package.json index 8c69fca68..bbdc2541b 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,7 @@ }, "devDependencies": { "@types/mocha": "^7.0.2", - "@types/node": "8", + "@types/node": "12", "@typescript-eslint/eslint-plugin": "^2.27.0", "@typescript-eslint/parser": "^2.27.0", "cross-env": "^5.1.4", diff --git a/src/shared/conout.ts b/src/shared/conout.ts new file mode 100644 index 000000000..7a7e05f85 --- /dev/null +++ b/src/shared/conout.ts @@ -0,0 +1,15 @@ +/** + * Copyright (c) 2020, Microsoft Corporation (MIT License). + */ + +export interface IWorkerData { + conoutPipeName: string; +} + +export const enum ConoutWorkerMessage { + READY = 1 +} + +export function getWorkerPipeName(conoutPipeName: string): string { + return `${conoutPipeName}-worker`; +} diff --git a/src/terminal.ts b/src/terminal.ts index 8af0a511a..63330aa5f 100644 --- a/src/terminal.ts +++ b/src/terminal.ts @@ -187,7 +187,6 @@ export abstract class Terminal implements ITerminal { public abstract get slave(): Socket; protected _close(): void { - this._socket.writable = false; this._socket.readable = false; this.write = () => {}; this.end = () => {}; diff --git a/src/windowsConoutConnection.ts b/src/windowsConoutConnection.ts new file mode 100644 index 000000000..42f02170f --- /dev/null +++ b/src/windowsConoutConnection.ts @@ -0,0 +1,78 @@ +/** + * Copyright (c) 2020, Microsoft Corporation (MIT License). + */ + +import { Worker } from 'worker_threads'; +import { Socket } from 'net'; +import { IDisposable } from './types'; +import { IWorkerData, ConoutWorkerMessage, getWorkerPipeName } from './shared/conout'; +import { join } from 'path'; +import { IEvent, EventEmitter2 } from './eventEmitter2'; + +/** + * The amount of time to wait for additional data after the conpty shell process has exited before + * shutting down the worker and sockets. The timer will be reset if a new data event comes in after + * the timer has started. + */ +const FLUSH_DATA_INTERVAL = 1000; + +/** + * Connects to and manages the lifecycle of the conout socket. This socket must be drained on + * another thread in order to avoid deadlocks where Conpty waits for the out socket to drain + * when `ClosePseudoConsole` is called. This happens when data is being written to the terminal when + * the pty is closed. + * + * See also: + * - https://github.com/microsoft/node-pty/issues/375 + * - https://github.com/microsoft/vscode/issues/76548 + * - https://github.com/microsoft/terminal/issues/1810 + * - https://docs.microsoft.com/en-us/windows/console/closepseudoconsole + */ +export class ConoutConnection implements IDisposable { + private _worker: Worker; + private _drainTimeout: NodeJS.Timeout; + private _isDisposed: boolean = false; + + private _onReady = new EventEmitter2(); + public get onReady(): IEvent { return this._onReady.event; } + + constructor( + private _conoutPipeName: string + ) { + const workerData: IWorkerData = { conoutPipeName: _conoutPipeName }; + this._worker = new Worker(join(__dirname, 'worker/conoutWorker.js'), { workerData }); + this._worker.on('message', (message: ConoutWorkerMessage) => { + switch (message) { + case ConoutWorkerMessage.READY: + this._onReady.fire(); + return; + default: + console.warn('Unexpected ConoutWorkerMessage', message); + } + }); + } + + dispose(): void { + if (this._isDisposed) { + return; + } + this._isDisposed = true; + // Drain all data from the socket before closing + this._drainDataAndClose(); + } + + connectSocket(socket: Socket): void { + socket.connect(getWorkerPipeName(this._conoutPipeName)); + } + + private _drainDataAndClose(): void { + if (this._drainTimeout) { + clearTimeout(this._drainTimeout); + } + this._drainTimeout = setTimeout(() => this._destroySocket(), FLUSH_DATA_INTERVAL); + } + + private async _destroySocket(): Promise { + await this._worker.terminate(); + } +} diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 358f8c3e3..1ba6d1d07 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -9,6 +9,7 @@ import * as path from 'path'; import { Socket } from 'net'; import { ArgvOrCommandLine } from './types'; import { fork } from 'child_process'; +import { ConoutConnection } from './windowsConoutConnection'; let conptyNative: IConptyNative; let winptyNative: IWinptyNative; @@ -18,7 +19,7 @@ let winptyNative: IWinptyNative; * shutting down the socket. The timer will be reset if a new data event comes in after the timer * has started. */ -const FLUSH_DATA_INTERVAL = 20; +const FLUSH_DATA_INTERVAL = 1000; /** * This agent sits between the WindowsTerminal class and provides a common interface for both conpty @@ -32,6 +33,7 @@ export class WindowsPtyAgent { private _innerPidHandle: number; private _closeTimeout: NodeJS.Timer; private _exitCode: number | undefined; + private _conoutSocketWorker: ConoutConnection; private _fd: any; private _pty: number; @@ -115,17 +117,18 @@ export class WindowsPtyAgent { // Create terminal pipe IPC channel and forward to a local unix socket. this._outSocket = new Socket(); this._outSocket.setEncoding('utf8'); - this._outSocket.connect(term.conout, () => { - // TODO: Emit event on agent instead of socket? - - // Emit ready event. + // The conout socket must be ready out on another thread to avoid deadlocks + this._conoutSocketWorker = new ConoutConnection(term.conout); + this._conoutSocketWorker.onReady(() => { + this._conoutSocketWorker.connectSocket(this._outSocket); + }); + this._outSocket.on('connect', () => { this._outSocket.emit('ready_datapipe'); }); this._inSocket = new Socket(); this._inSocket.setEncoding('utf8'); this._inSocket.connect(term.conin); - // TODO: Wait for ready event? if (this._useConpty) { const connect = (this._ptyNative as IConptyNative).connect(this._pty, commandLine, cwd, env, c => this._$onProcessExit(c)); @@ -146,9 +149,7 @@ export class WindowsPtyAgent { public kill(): void { this._inSocket.readable = false; - this._inSocket.writable = false; this._outSocket.readable = false; - this._outSocket.writable = false; // Tell the agent to kill the pty, this releases handles to the process if (this._useConpty) { this._getConsoleProcessList().then(consoleProcessList => { @@ -233,9 +234,7 @@ export class WindowsPtyAgent { private _cleanUpProcess(): void { this._inSocket.readable = false; - this._inSocket.writable = false; this._outSocket.readable = false; - this._outSocket.writable = false; this._outSocket.destroy(); } } diff --git a/src/worker/conoutWorker.ts b/src/worker/conoutWorker.ts new file mode 100644 index 000000000..b58eb4168 --- /dev/null +++ b/src/worker/conoutWorker.ts @@ -0,0 +1,18 @@ +/** + * Copyright (c) 2020, Microsoft Corporation (MIT License). + */ + +import { parentPort, workerData } from 'worker_threads'; +import { Socket, createServer } from 'net'; +import { ConoutWorkerMessage, IWorkerData, getWorkerPipeName } from '../shared/conout'; + +const conoutPipeName = (workerData as IWorkerData).conoutPipeName; +const conoutSocket = new Socket(); +conoutSocket.setEncoding('utf8'); +conoutSocket.connect(conoutPipeName, () => { + const server = createServer(workerSocket => { + conoutSocket.pipe(workerSocket); + }); + server.listen(getWorkerPipeName(conoutPipeName)); + parentPort.postMessage(ConoutWorkerMessage.READY); +}); diff --git a/test/spam-close.js b/test/spam-close.js new file mode 100644 index 000000000..3a84ce418 --- /dev/null +++ b/test/spam-close.js @@ -0,0 +1,33 @@ +// This test creates a pty periodically, spamming it with echo calls and killing it shortly after. +// It's a test case for https://github.com/microsoft/node-pty/issues/375, the script will hang +// when it show this bug instead of continuing to create more processes. + +var os = require('os'); +var pty = require('..'); + +var isWindows = os.platform() === 'win32'; +var shell = isWindows ? 'cmd.exe' : 'bash'; + +let i = 0; + +setInterval(() => { + console.log(`creating pty ${++i}`); + var ptyProcess = pty.spawn(shell, [], { + name: 'xterm-256color', + cols: 80, + rows: 26, + cwd: isWindows ? process.env.USERPROFILE : process.env.HOME, + env: Object.assign({ TEST: "Environment vars work" }, process.env), + useConpty: true + }); + + ptyProcess.onData(data => console.log(` data: ${data.replace(/\x1b|\n|\r/g, '_')}`)); + + setInterval(() => { + ptyProcess.write('echo foo\r'.repeat(50)); + }, 10); + setTimeout(() => { + console.log(` killing ${ptyProcess.pid}...`); + ptyProcess.kill(); + }, 100); +}, 1200); diff --git a/yarn.lock b/yarn.lock index 2ba859050..24ce3d0a2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -43,10 +43,10 @@ resolved "https://registry.yarnpkg.com/@types/mocha/-/mocha-7.0.2.tgz#b17f16cf933597e10d6d78eae3251e692ce8b0ce" integrity sha512-ZvO2tAcjmMi8V/5Z3JsyofMe3hasRcaw88cto5etSVMwVQfeivGAlEYmaQgceUSVYFofVjT+ioHsATjdWcFt1w== -"@types/node@8": - version "8.10.49" - resolved "https://registry.yarnpkg.com/@types/node/-/node-8.10.49.tgz#f331afc5efed0796798e5591d6e0ece636969b7b" - integrity sha512-YX30JVx0PvSmJ3Eqr74fYLGeBxD+C7vIL20ek+GGGLJeUbVYRUW3EzyAXpIRA0K8c8o0UWqR/GwEFYiFoz1T8w== +"@types/node@12": + version "12.12.45" + resolved "https://registry.yarnpkg.com/@types/node/-/node-12.12.45.tgz#33d550d6da243652004b00cbf4f15997456a38e3" + integrity sha512-9w50wqeS0qQH9bo1iIRcQhDXRxoDzyAqCL5oJG+Nuu7cAoe6omGo+YDE0spAGK5sPrdLDhQLbQxq0DnxyndPKA== "@typescript-eslint/eslint-plugin@^2.27.0": version "2.27.0" From 6da48b0238d44c12eed6bb550564195e9d92d8dc Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 9 Jun 2020 08:34:22 -0700 Subject: [PATCH 2/5] Call ConoutSocketWorker.dispose --- src/windowsPtyAgent.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts index 1ba6d1d07..3129b0a03 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -179,6 +179,7 @@ export class WindowsPtyAgent { } }); } + this._conoutSocketWorker.dispose(); } private _getConsoleProcessList(): Promise { From a8d2b41334245ac0bd6aea0559be325e4864d821 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 9 Jun 2020 08:35:20 -0700 Subject: [PATCH 3/5] Rename file to match variable name --- src/windowsConoutConnection.ts | 2 +- src/worker/{conoutWorker.ts => conoutSocketWorker.ts} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename src/worker/{conoutWorker.ts => conoutSocketWorker.ts} (100%) diff --git a/src/windowsConoutConnection.ts b/src/windowsConoutConnection.ts index 42f02170f..ad16ff051 100644 --- a/src/windowsConoutConnection.ts +++ b/src/windowsConoutConnection.ts @@ -40,7 +40,7 @@ export class ConoutConnection implements IDisposable { private _conoutPipeName: string ) { const workerData: IWorkerData = { conoutPipeName: _conoutPipeName }; - this._worker = new Worker(join(__dirname, 'worker/conoutWorker.js'), { workerData }); + this._worker = new Worker(join(__dirname, 'worker/conoutSocketWorker.js'), { workerData }); this._worker.on('message', (message: ConoutWorkerMessage) => { switch (message) { case ConoutWorkerMessage.READY: diff --git a/src/worker/conoutWorker.ts b/src/worker/conoutSocketWorker.ts similarity index 100% rename from src/worker/conoutWorker.ts rename to src/worker/conoutSocketWorker.ts From ab5b172a30aa1015b73aefba5502ff203bf526f3 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 9 Jun 2020 09:32:21 -0700 Subject: [PATCH 4/5] Update pipelines Node version --- azure-pipelines.yml | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 987f49bd2..ca76c9a02 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -9,8 +9,6 @@ jobs: vmImage: 'ubuntu-16.04' strategy: matrix: - node_10_x: - node_version: 10.x node_12_x: node_version: 12.x steps: @@ -33,8 +31,6 @@ jobs: vmImage: 'macOS-10.15' strategy: matrix: - node_10_x: - node_version: 10.x node_12_x: node_version: 12.x steps: @@ -57,8 +53,6 @@ jobs: vmImage: 'vs2017-win2016' strategy: matrix: - node_10_x: - node_version: 10.x node_12_x: node_version: 12.x steps: @@ -87,7 +81,7 @@ jobs: steps: - task: NodeTool@0 inputs: - versionSpec: '8.x' + versionSpec: '12.x' displayName: 'Install Node.js' - script: | npm i From 3d299ba91d46450aaeee5c61f7016a9c38b1a98d Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 10 Jun 2020 06:50:24 -0700 Subject: [PATCH 5/5] Add min version for Electron --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 0de001810..ca82343cf 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ npm run build ## Dependencies -Node.JS 12+ is required to use `node-pty`, v10 also works but requires the workers flag to be enabled on Windows (`node --experimental-worker`). +Node.JS 12+ or Electron 8+ is required to use `node-pty`. ### Linux (apt)