Update to conpty v1.22 (#759)

* Update to conpty v1.22

Fixes #490

* Add logs to show the problem happening

* spec: increase exit delay

* refactor: input and output handling with conpty

* Close the input read and output write handles after creating
  the client process
* Call ReleasePseudoConsole after creating the client process
  which will cause the output read handle to close when there
  is no more data from the session
* For manual termination via Kill, we close the input write handle
  and call into ClosePseudoConsole, we should then drain the output
  handle

NB: ideally draining the output handle should have been enough
to cause the client process to close but it doesn't work, we call
TerminateProcess to fix this case.

* chore: restore legacy conpty path

---------

Co-authored-by: deepak1556 <hop2deep@gmail.com>

Author: Tyriar

publish.yml

@@ -61,6 +61,6 @@ extends:
           tag: beta
           publishRequiresApproval: false
 
-        apiScanExcludes: 'package/third_party/conpty/1.20.240626001/win10-arm64/*.*'
+        apiScanExcludes: 'package/third_party/conpty/1.22.250204002/win10-arm64/*.*'
         apiScanSoftwareName: 'vscode-node-pty'
         apiScanSoftwareVersion: '1'

src/win/conpty.cc

@@ -34,24 +34,17 @@ typedef HRESULT (__stdcall *PFNCREATEPSEUDOCONSOLE)(COORD c, HANDLE hIn, HANDLE
 typedef HRESULT (__stdcall *PFNRESIZEPSEUDOCONSOLE)(HPCON hpc, COORD newSize);
 typedef HRESULT (__stdcall *PFNCLEARPSEUDOCONSOLE)(HPCON hpc);
 typedef void (__stdcall *PFNCLOSEPSEUDOCONSOLE)(HPCON hpc);
+typedef void (__stdcall *PFNRELEASEPSEUDOCONSOLE)(HPCON hpc);
 
 #endif
 
-typedef struct _PseudoConsole
-{
-    HANDLE hSignal;
-    HANDLE hPtyReference;
-    HANDLE hConPtyProcess;
-} PseudoConsole;
-
 struct pty_baton {
   int id;
   HANDLE hIn;
   HANDLE hOut;
   HPCON hpc;
 
   HANDLE hShell;
-  HANDLE pty_job_handle;
 
   pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {};
 };
@@ -80,27 +73,6 @@ static bool remove_pty_baton(int id) {
   return false;
 }
 
-static BOOL InitPtyJobHandle(HANDLE* pty_job_handle) {
-  SECURITY_ATTRIBUTES attr;
-  memset(&attr, 0, sizeof(attr));
-  attr.bInheritHandle = FALSE;
-  *pty_job_handle = CreateJobObjectW(&attr, nullptr);
-  if (*pty_job_handle == NULL) {
-    return FALSE;
-  }
-  JOBOBJECT_EXTENDED_LIMIT_INFORMATION limits = {};
-  limits.BasicLimitInformation.LimitFlags =
-      JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION |
-      JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
-  if (!SetInformationJobObject(*pty_job_handle,
-                               JobObjectExtendedLimitInformation,
-                               &limits,
-                               sizeof(limits))) {
-    return FALSE;
-  }
-  return TRUE;
-}
-
 struct ExitEvent {
   int exit_code = 0;
 };
@@ -130,11 +102,6 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) {
     // Get process exit code.
     GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code));
     // Clean up handles
-    // Calling DisconnectNamedPipes here or in PtyKill results in a crash,
-    // ref https://github.com/microsoft/node-pty/issues/512,
-    // so we only call CloseHandle for now.
-    CloseHandle(baton->hIn);
-    CloseHandle(baton->hOut);
     CloseHandle(baton->hShell);
     assert(remove_pty_baton(baton->id));
 
@@ -459,14 +426,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
     throw errorWithCode(info, "Cannot create process");
   }
 
-  if (useConptyDll) {
-    if (!InitPtyJobHandle(&handle->pty_job_handle)) {
-      throw errorWithCode(info, "Initialize pty job handle failed");
-    }
-
-    PseudoConsole* server = reinterpret_cast<PseudoConsole*>(handle->hpc);
-    if (!AssignProcessToJobObject(handle->pty_job_handle, server->hConPtyProcess)) {
-      throw errorWithCode(info, "AssignProcessToJobObject for server failed");
+  HANDLE hLibrary = LoadConptyDll(info, useConptyDll);
+  bool fLoadedDll = hLibrary != nullptr;
+  if (fLoadedDll)
+  {
+    PFNRELEASEPSEUDOCONSOLE const pfnReleasePseudoConsole = (PFNRELEASEPSEUDOCONSOLE)GetProcAddress(
+      (HMODULE)hLibrary, "ConptyReleasePseudoConsole");
+    if (pfnReleasePseudoConsole)
+    {
+      pfnReleasePseudoConsole(handle->hpc);
     }
   }
 
@@ -475,6 +443,9 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) {
 
   // Close the thread handle to avoid resource leak
   CloseHandle(piClient.hThread);
+  // Close the input read and output write handle of the pseudoconsole
+  CloseHandle(handle->hIn);
+  CloseHandle(handle->hOut);
 
   SetupExitCallback(env, exitCallback, handle);
 
@@ -588,11 +559,7 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) {
         pfnClosePseudoConsole(handle->hpc);
       }
     }
-
     TerminateProcess(handle->hShell, 1);
-    if (useConptyDll) {
-      CloseHandle(handle->pty_job_handle);
-    }
   }
 
   return env.Undefined();

src/windowsConoutConnection.ts

@@ -54,10 +54,6 @@ export class ConoutConnection implements IDisposable {
   }
 
   dispose(): void {
-    if (this._isDisposed) {
-      return;
-    }
-    this._isDisposed = true;
     // Drain all data from the socket before closing
     this._drainDataAndClose();
   }

src/windowsPtyAgent.ts

@@ -158,11 +158,11 @@ export class WindowsPtyAgent {
   }
 
   public kill(): void {
-    this._inSocket.readable = false;
-    this._outSocket.readable = false;
     // Tell the agent to kill the pty, this releases handles to the process
     if (this._useConpty) {
       if (!this._useConptyDll) {
+        this._inSocket.readable = false;
+        this._outSocket.readable = false;
         this._getConsoleProcessList().then(consoleProcessList => {
           consoleProcessList.forEach((pid: number) => {
             try {
@@ -172,8 +172,16 @@ export class WindowsPtyAgent {
             }
           });
         });
+        (this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll);
+        this._conoutSocketWorker.dispose();
+      } else {
+        // Close the input write handle to signal the end of session.
+        this._inSocket.destroy();
+        (this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll);
+        this._outSocket.on('data', () => {
+          this._conoutSocketWorker.dispose();
+        });
       }
-      (this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll);
     } else {
       // Because pty.kill closes the handle, it will kill most processes by itself.
       // Process IDs can be reused as soon as all handles to them are
@@ -191,7 +199,6 @@ export class WindowsPtyAgent {
         }
       });
     }
-    this._conoutSocketWorker.dispose();
   }
 
   private _getConsoleProcessList(): Promise<number[]> {
@@ -235,18 +242,26 @@ export class WindowsPtyAgent {
    */
   private _$onProcessExit(exitCode: number): void {
     this._exitCode = exitCode;
-    this._flushDataAndCleanUp();
-    this._outSocket.on('data', () => this._flushDataAndCleanUp());
+    if (!this._useConptyDll) {
+      this._flushDataAndCleanUp();
+      this._outSocket.on('data', () => this._flushDataAndCleanUp());
+    }
   }
 
   private _flushDataAndCleanUp(): void {
+    if (this._useConptyDll) {
+      return;
+    }
     if (this._closeTimeout) {
       clearTimeout(this._closeTimeout);
     }
     this._closeTimeout = setTimeout(() => this._cleanUpProcess(), FLUSH_DATA_INTERVAL);
   }
 
   private _cleanUpProcess(): void {
+    if (this._useConptyDll) {
+      return;
+    }
     this._inSocket.readable = false;
     this._outSocket.readable = false;
     this._outSocket.destroy();

src/windowsTerminal.test.ts

@@ -26,13 +26,16 @@ function pollForProcessState(desiredState: IProcessState, intervalMs: number = 1
       psList({ all: true }).then(ps => {
         let success = true;
         const pids = Object.keys(desiredState).map(k => parseInt(k, 10));
+        console.log('expected pids', JSON.stringify(pids));
         pids.forEach(pid => {
           if (desiredState[pid]) {
             if (!ps.some(p => p.pid === pid)) {
+              console.log(`pid ${pid} does not exist`);
               success = false;
             }
           } else {
             if (ps.some(p => p.pid === pid)) {
+              console.log(`pid ${pid} still exists`);
               success = false;
             }
           }
@@ -71,6 +74,7 @@ function pollForProcessTreeSize(pid: number, size: number, intervalMs: number =
           }).forEach(p => openList.push(p));
           list.push(current);
         }
+        console.log('list', JSON.stringify(list));
         const success = list.length === size;
         if (success) {
           clearInterval(interval);
@@ -88,37 +92,33 @@ function pollForProcessTreeSize(pid: number, size: number, intervalMs: number =
 }
 
 if (process.platform === 'win32') {
-  [[true, false], [true, true], [false, false]].forEach(([useConpty, useConptyDll]) => {
+  [[false, false], [true, false], [true, true]].forEach(([useConpty, useConptyDll]) => {
     describe(`WindowsTerminal (useConpty = ${useConpty}, useConptyDll = ${useConptyDll})`, () => {
       describe('kill', () => {
-        it('should not crash parent process', (done) => {
+        it('should not crash parent process', function (done) {
+          this.timeout(20000);
           const term = new WindowsTerminal('cmd.exe', [], { useConpty, useConptyDll });
+          term.on('exit', () => done());
           term.kill();
-          // 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(10000);
+          this.timeout(20000);
           const term = new WindowsTerminal('cmd.exe', [], { useConpty, useConptyDll });
           // 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 => {
+          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(), 'notepad.exe');
-            assert.strictEqual(list[3].name.toLowerCase(), 'node.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] = true;
-            desiredState[list[3].pid] = false;
+            desiredState[list[2].pid] = false;
             term.on('exit', () => {
-              pollForProcessState(desiredState).then(() => {
-                // Kill notepad before done
-                process.kill(list[2].pid);
+              pollForProcessState(desiredState, 1000, 5000).then(() => {
                 done();
               });
             });
@@ -127,11 +127,15 @@ if (process.platform === 'win32') {
       });
 
       describe('resize', () => {
-        it('should throw a non-native exception when resizing an invalid value', () => {
+        it('should throw a non-native exception when resizing an invalid value', (done) => {
           const term = new WindowsTerminal('cmd.exe', [], { useConpty, useConptyDll });
           assert.throws(() => term.resize(-1, -1));
           assert.throws(() => term.resize(0, 0));
           assert.doesNotThrow(() => term.resize(1, 1));
+          term.on('exit', () => {
+            done();
+          });
+          term.kill();
         });
         it('should throw a non-native exception when resizing a killed terminal', (done) => {
           const term = new WindowsTerminal('cmd.exe', [], { useConpty, useConptyDll });
@@ -146,7 +150,8 @@ if (process.platform === 'win32') {
       });
 
       describe('Args as CommandLine', () => {
-        it('should not fail running a file containing a space in the path', (done) => {
+        it('should not fail running a file containing a space in the path', function (done) {
+          this.timeout(10000);
           const spaceFolder = path.resolve(__dirname, '..', 'fixtures', 'space folder');
           if (!fs.existsSync(spaceFolder)) {
             fs.mkdirSync(spaceFolder);
@@ -173,8 +178,9 @@ if (process.platform === 'win32') {
       });
 
       describe('env', () => {
-        it('should set environment variables of the shell', (done) => {
-          const term = new WindowsTerminal('cmd.exe', '/C echo %FOO%', { env: { FOO: 'BAR' }});
+        it('should set environment variables of the shell', function (done) {
+          this.timeout(10000);
+          const term = new WindowsTerminal('cmd.exe', '/C echo %FOO%', { useConpty, useConptyDll, env: { FOO: 'BAR' }});
           let result = '';
           term.on('data', (data) => {
             result += data;
@@ -187,15 +193,17 @@ if (process.platform === 'win32') {
       });
 
       describe('On close', () => {
-        it('should return process zero exit codes', (done) => {
+        it('should return process zero exit codes', function (done) {
+          this.timeout(10000);
           const term = new WindowsTerminal('cmd.exe', '/C exit', { useConpty, useConptyDll });
           term.on('exit', (code) => {
             assert.strictEqual(code, 0);
             done();
           });
         });
 
-        it('should return process non-zero exit codes', (done) => {
+        it('should return process non-zero exit codes', function (done) {
+          this.timeout(10000);
           const term = new WindowsTerminal('cmd.exe', '/C exit 2', { useConpty, useConptyDll });
           term.on('exit', (code) => {
             assert.strictEqual(code, 2);
@@ -205,7 +213,8 @@ if (process.platform === 'win32') {
       });
 
       describe('Write', () => {
-        it('should accept input', (done) => {
+        it('should accept input', function (done) {
+          this.timeout(10000);
           const term = new WindowsTerminal('cmd.exe', '', { useConpty, useConptyDll });
           term.write('exit\r');
           term.on('exit', () => {

src/worker/conoutSocketWorker.ts

@@ -4,19 +4,22 @@
 
 import { parentPort, workerData } from 'worker_threads';
 import { Socket, createServer } from 'net';
+import * as fs from 'fs';
 import { ConoutWorkerMessage, IWorkerData, getWorkerPipeName } from '../shared/conout';
 
 const conoutPipeName = (workerData as IWorkerData).conoutPipeName;
-const conoutSocket = new Socket();
+const outSocketFD = fs.openSync(conoutPipeName, 'r');
+const conoutSocket = new Socket({
+  fd: outSocketFD,
+  readable: true
+});
 conoutSocket.setEncoding('utf8');
-conoutSocket.connect(conoutPipeName, () => {
-  const server = createServer(workerSocket => {
-    conoutSocket.pipe(workerSocket);
-  });
-  server.listen(getWorkerPipeName(conoutPipeName));
-
-  if (!parentPort) {
-    throw new Error('worker_threads parentPort is null');
-  }
-  parentPort.postMessage(ConoutWorkerMessage.READY);
+const server = createServer(workerSocket => {
+  conoutSocket.pipe(workerSocket);
 });
+server.listen(getWorkerPipeName(conoutPipeName));
+
+if (!parentPort) {
+  throw new Error('worker_threads parentPort is null');
+}
+parentPort.postMessage(ConoutWorkerMessage.READY);