Fix kill under conpty

binding.gyp

@@ -15,6 +15,15 @@
             'shlwapi.lib'
           ]
         },
+        {
+          'target_name': 'conpty_console_list',
+          'include_dirs' : [
+            '<!(node -e "require(\'nan\')")'
+          ],
+          'sources' : [
+            'src/win/conpty_console_list.cc'
+          ]
+        },
         {
           'target_name': 'pty',
           'include_dirs' : [

examples/killDeepTree/index.js

@@ -12,7 +12,7 @@ var ptyProcess = pty.spawn(shell, [], {
 });
 
 ptyProcess.on('data', function(data) {
-  console.log(data);
+  process.stdout.write(data);
 });
 
 ptyProcess.write('start notepad\r');

package.json

@@ -34,6 +34,7 @@
   ],
   "scripts": {
     "tsc": "tsc",
+    "watch": "tsc -w",
     "lint": "tslint 'src/**/*.ts'",
     "install": "node scripts/install.js",
     "postinstall": "node scripts/post-install.js",
@@ -46,9 +47,11 @@
   "devDependencies": {
     "@types/mocha": "^5.0.0",
     "@types/node": "^6.0.104",
+    "@types/ps-list": "^6.0.0",
     "cross-env": "^5.1.4",
     "mocha": "^5.0.5",
     "pollUntil": "^1.0.3",
+    "ps-list": "^6.0.0",
     "tslint": "^5.12.1",
     "tslint-consistent-codestyle": "^1.15.0",
     "typescript": "^3.2.2"

scripts/install.js

@@ -4,15 +4,35 @@ const os = require('os');
 const path = require('path');
 const spawn = require('child_process').spawn;
 
-const args = ['rebuild'];
-if (process.env.NODE_PTY_DEBUG) {
-  args.push('--debug');
-}
-const p = spawn(os.platform() === 'win32' ? 'node-gyp.cmd' : 'node-gyp', args, {
-  cwd: path.join(__dirname, '..'),
-  stdio: 'inherit'
-});
+installWindowsProcessTree().then(() => {
+  const gypArgs = ['rebuild'];
+  if (process.env.NODE_PTY_DEBUG) {
+    gypArgs.push('--debug');
+  }
+  const gypProcess = spawn(os.platform() === 'win32' ? 'node-gyp.cmd' : 'node-gyp', gypArgs, {
+    cwd: path.join(__dirname, '..'),
+    stdio: 'inherit'
+  });
 
-p.on('exit', function (code) {
-  process.exit(code);
+  gypProcess.on('exit', function (code) {
+    process.exit(code);
+  });
 });
+
+// windows-process-tree is an optional dev dependency which npm does not support
+function installWindowsProcessTree() {
+  return new Promise(resolve => {
+    if (process.platform !== 'win32') {
+      resolve();
+      return;
+    }
+    const npmArgs = ['i', '--no-save', 'windows-process-tree@0.2.3'];
+    const npmProcess = spawn(os.platform() === 'win32' ? 'npm.cmd' : 'npm', npmArgs, {
+      cwd: path.join(__dirname, '..'),
+      stdio: 'inherit'
+    });
+    npmProcess.on('exit', function (code) {
+      resolve();
+    });
+  });
+}

scripts/post-install.js

@@ -5,6 +5,8 @@ var RELEASE_DIR = path.join(__dirname, '..', 'build', 'Release');
 var BUILD_FILES = [
   path.join(RELEASE_DIR, 'conpty.node'),
   path.join(RELEASE_DIR, 'conpty.pdb'),
+  path.join(RELEASE_DIR, 'conpty_console_list.node'),
+  path.join(RELEASE_DIR, 'conpty_console_list.pdb'),
   path.join(RELEASE_DIR, 'pty.node'),
   path.join(RELEASE_DIR, 'pty.pdb'),
   path.join(RELEASE_DIR, 'winpty-agent.exe'),

src/conpty_console_list_agent.ts

@@ -0,0 +1,15 @@
+/**
+ * Copyright (c) 2019, Microsoft Corporation (MIT License).
+ *
+ * This module fetches the console process list for a particular PID. It must be
+ * called from a different process (child_process.fork) as there can only be a
+ * single console attached to a process.
+ */
+
+import { loadNative } from './utils';
+
+const getConsoleProcessList = loadNative('conpty_console_list').getConsoleProcessList;
+const shellPid = parseInt(process.argv[2], 10);
+const consoleProcessList = getConsoleProcessList(shellPid);
+process.send({ consoleProcessList });
+process.exit(0);

src/win/conpty.cc

@@ -419,6 +419,8 @@ static NAN_METHOD(PtyKill) {
     }
   }
 
+  CloseHandle(handle->hShell);
+
   return info.GetReturnValue().SetUndefined();
 }
 

src/win/conpty_console_list.cc

@@ -0,0 +1,43 @@
+/**
+ * Copyright (c) 2019, Microsoft Corporation (MIT License).
+ */
+
+#include <nan.h>
+#include <windows.h>
+
+static NAN_METHOD(ApiConsoleProcessList) {
+  if (info.Length() != 1 ||
+      !info[0]->IsNumber()) {
+    Nan::ThrowError("Usage: getConsoleProcessList(shellPid)");
+    return;
+  }
+
+  const SHORT pid = info[0]->Uint32Value();
+
+  if (!FreeConsole()) {
+    Nan::ThrowError("FreeConsole failed");
+  }
+  if (!AttachConsole(pid)) {
+    Nan::ThrowError("AttachConsole failed");
+  }
+  auto processList = std::vector<DWORD>(64);
+  auto processCount = GetConsoleProcessList(&processList[0], processList.size());
+  if (processList.size() < processCount) {
+      processList.resize(processCount);
+      processCount = GetConsoleProcessList(&processList[0], processList.size());
+  }
+  FreeConsole();
+
+  v8::Local<v8::Array> result = Nan::New<v8::Array>();
+  for (DWORD i = 0; i < processCount; i++) {
+    result->Set(i, Nan::New<v8::Number>(processList[i]));
+  }
+  info.GetReturnValue().Set(result);
+}
+
+extern "C" void init(v8::Handle<v8::Object> target) {
+  Nan::HandleScope scope;
+  Nan::SetMethod(target, "getConsoleProcessList", ApiConsoleProcessList);
+};
+
+NODE_MODULE(pty, init);

src/windowsPtyAgent.ts

@@ -9,6 +9,7 @@ import * as path from 'path';
 import { Socket } from 'net';
 import { ArgvOrCommandLine } from './types';
 import { loadNative } from './utils';
+import { fork } from 'child_process';
 
 let conptyNative: IConptyNative;
 let winptyNative: IWinptyNative;
@@ -54,7 +55,7 @@ export class WindowsPtyAgent {
     private _useConpty: boolean | undefined
   ) {
     if (this._useConpty === undefined || this._useConpty === true) {
-      this._useConpty = this._getWindowsBuildNumber() >= 17692;
+      this._useConpty = this._getWindowsBuildNumber() >= 18309;
     }
     if (this._useConpty) {
       if (!conptyNative) {
@@ -130,15 +131,25 @@ export class WindowsPtyAgent {
     this._outSocket.writable = false;
     // Tell the agent to kill the pty, this releases handles to the process
     if (this._useConpty) {
-      (this._ptyNative as IConptyNative).kill(this._pty);
+      this._getConsoleProcessList().then(consoleProcessList => {
+        consoleProcessList.forEach((pid: number) => {
+          try {
+            process.kill(pid);
+          } catch (e) {
+            // Ignore if process cannot be found (kill ESRCH error)
+          }
+        });
+        (this._ptyNative as IConptyNative).kill(this._pty);
+      });
     } else {
-      const processList: number[] = (this._ptyNative as IWinptyNative).getProcessList(this._pid);
       (this._ptyNative as IWinptyNative).kill(this._pid, this._innerPidHandle);
-      // Since pty.kill will kill most processes by itself and process IDs can be
-      // reused as soon as all handles to them are dropped, we want to immediately
-      // kill the entire console process list. If we do not force kill all
-      // processes here, node servers in particular seem to become detached and
-      // remain running (see Microsoft/vscode#26807).
+      // Since pty.kill closes the handle it will kill most processes by itself
+      // and process IDs can be reused as soon as all handles to them are
+      // dropped, we want to immediately kill the entire console process list.
+      // If we do not force kill all processes here, node servers in particular
+      // seem to become detached and remain running (see
+      // Microsoft/vscode#26807).
+      const processList: number[] = (this._ptyNative as IWinptyNative).getProcessList(this._pid);
       processList.forEach(pid => {
         try {
           process.kill(pid);
@@ -149,6 +160,22 @@ export class WindowsPtyAgent {
     }
   }
 
+  private _getConsoleProcessList(): Promise<number[]> {
+    return new Promise<number[]>(resolve => {
+      const agent = fork(path.join(__dirname, 'conpty_console_list_agent'), [ this._innerPid.toString() ]);
+      agent.on('message', message => {
+        clearTimeout(timeout);
+        resolve(message.consoleProcessList);
+      });
+      const timeout = setTimeout(() => {
+        // Something went wrong, just send back the shell PID
+        console.error('Could not fetch console process list');
+        agent.kill();
+        resolve([ this._innerPid ]);
+      }, 5000);
+    });
+  }
+
   public get exitCode(): number {
     if (this._useConpty) {
       return this._exitCode;

src/windowsTerminal.test.ts

@@ -3,10 +3,84 @@
  * Copyright (c) 2018, Microsoft Corporation (MIT License).
  */
 
+import * as cp from 'child_process';
 import * as fs from 'fs';
 import * as assert from 'assert';
 import { WindowsTerminal } from './windowsTerminal';
 import * as path from 'path';
+import * as psList from 'ps-list';
+
+let getProcessList: any;
+if (process.platform === 'win32') {
+  getProcessList = require('windows-process-tree').getProcessList;
+}
+
+interface IProcessState {
+  // Whether the PID must exist or must not exist
+  [pid: number]: boolean;
+}
+
+interface IWindowsProcessTreeResult {
+  name: string;
+  pid: number;
+}
+
+function pollForProcessState(desiredState: IProcessState, intervalMs: number = 100, timeoutMs: number = 2000): Promise<void> {
+  return new Promise<void>(resolve => {
+    let tries = 0;
+    const interval = setInterval(() => {
+      psList({ all: true }).then(ps => {
+        let success = true;
+        const pids = Object.keys(desiredState).map(k => parseInt(k, 10));
+        pids.forEach(pid => {
+          if (desiredState[pid]) {
+            if (!ps.some(p => p.pid === pid)) {
+              success = false;
+            }
+          } else {
+            if (ps.some(p => p.pid === pid)) {
+              success = false;
+            }
+          }
+        });
+        if (success) {
+          clearInterval(interval);
+          resolve();
+          return;
+        }
+        tries++;
+        if (tries * intervalMs >= timeoutMs) {
+          clearInterval(interval);
+          const processListing = pids.map(k => `${k}: ${desiredState[k]}`).join('\n');
+          assert.fail(`Bad process state, expected:\n${processListing}`);
+          resolve();
+        }
+      });
+    }, intervalMs);
+  });
+}
+
+function pollForProcessTreeSize(pid: number, size: number, intervalMs: number = 100, timeoutMs: number = 2000): Promise<IWindowsProcessTreeResult[]> {
+  return new Promise<IWindowsProcessTreeResult[]>(resolve => {
+    let tries = 0;
+    const interval = setInterval(() => {
+      getProcessList(pid, (list: {name: string, pid: number}[]) => {
+        const success = list.length === size;
+        if (success) {
+          clearInterval(interval);
+          resolve(list);
+          return;
+        }
+        tries++;
+        if (tries * intervalMs >= timeoutMs) {
+          clearInterval(interval);
+          assert.fail(`Bad process state, expected: ${size}, actual: ${list.length}`);
+          resolve();
+        }
+      });
+    }, intervalMs);
+  });
+}
 
 if (process.platform === 'win32') {
   describe('WindowsTerminal', () => {
@@ -17,6 +91,31 @@ if (process.platform === 'win32') {
         // Add done call to deferred function queue to ensure the kill call has completed
         (<any>term)._defer(done);
       });
+      it('should kill the process tree', function (done: Mocha.Done): void {
+        this.timeout(5000);
+        const term = new WindowsTerminal('cmd.exe', [], {});
+        // Start sub-processes
+        term.write('powershell.exe\r');
+        term.write('notepad.exe\r');
+        term.write('node.exe\r');
+        pollForProcessTreeSize(term.pid, 4, 500, 5000).then(list => {
+          assert.equal(list[0].name, 'cmd.exe');
+          assert.equal(list[1].name, 'powershell.exe');
+          assert.equal(list[2].name, 'notepad.exe');
+          assert.equal(list[3].name, 'node.exe');
+          term.kill();
+          const desiredState: IProcessState = {};
+          desiredState[list[0].pid] = false;
+          desiredState[list[1].pid] = false;
+          desiredState[list[2].pid] = true;
+          desiredState[list[3].pid] = false;
+          pollForProcessState(desiredState).then(() => {
+            // Kill notepad before done
+            process.kill(list[2].pid);
+            done();
+          });
+        });
+      });
     });
 
     describe('resize', () => {

tsconfig.json

@@ -6,7 +6,7 @@
     "outDir": "lib",
     "sourceMap": true,
     "lib": [
-      "es5"
+      "es2015"
     ],
     "alwaysStrict": true,
     "noImplicitAny": true,

typings/node-pty.d.ts

@@ -28,7 +28,8 @@ declare module 'node-pty' {
     encoding?: string;
     /**
      * Whether to use the experimental ConPTY system on Windows. When this is not set, ConPTY will
-     * be used when the Windows build number is >= 17692.
+     * be used when the Windows build number is >= 18309 (it's available in 17134 and 17692 but is
+     * too unstable to enable by default).
      *
      * This setting does nothing on non-Windows.
      */