Merge pull request #415 from microsoft/hang_fix

Host conout socket in a worker

README.md

@@ -77,16 +77,14 @@ npm run build
 
 ## Dependencies
 
-### Linux/Ubuntu
+Node.JS 12+ or Electron 8+ is required to use `node-pty`.
 
-```
+### 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.
@@ -102,7 +100,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
 

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

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",

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`;
+}

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 = () => {};

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<void>();
+  public get onReady(): IEvent<void> { return this._onReady.event; }
+
+  constructor(
+    private _conoutPipeName: string
+  ) {
+    const workerData: IWorkerData = { conoutPipeName: _conoutPipeName };
+    this._worker = new Worker(join(__dirname, 'worker/conoutSocketWorker.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<void> {
+    await this._worker.terminate();
+  }
+}

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 => {
@@ -178,6 +179,7 @@ export class WindowsPtyAgent {
         }
       });
     }
+    this._conoutSocketWorker.dispose();
   }
 
   private _getConsoleProcessList(): Promise<number[]> {
@@ -233,9 +235,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();
   }
 }

src/worker/conoutSocketWorker.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);
+});

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

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"