Properly thread the handles received from the ConPTY

[Tyriar]

ConPTY can hang because we're managing it in the main thread.

microsoft/terminal#1810 (comment)

https://github.com/microsoft/node-pty/blob/master/src/win/conpty.cc

[JustinGrote]

Remediation for this is noted in microsoft/terminal#1810 (comment)

[jerch]

@Tyriar Maybe it is enough to just move the closing of the handle in another thread and delete all references from the mainthread. This would avoid priority issues around the libuv IO handling, that might occur, if the whole conpty handling (spawning --> chunk reading -> closing) has to live in its own thread. Not to forget buffer backpressure/flow control/sync, that needs to be set up up to xterm.js, if the conpty consumer runs "unleashed".

[joshfria]

Is there any movement on this issue? It causes hangs for many vscode users.

[JustinGrote]

It's also the highest upvoted bug issue in this repository, to give an idea of the scope of users being affected by hard lock crashes.

[JustinGrote]

My VSCode still hard locks about 5 times a day due to this issue, to the point I had to make this function:

It is the most impacting issue to my VSCode quality of life for several months now.

[Tyriar]

Managed to put together a note-pty snippet which repros the crash reliably:

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();
    console.log(`  killing ${ptyProcess.pid} after`);
  }, 100);
}, 1200);

ie. close the pty when there is lots of writing happening.

[DzmitryFil]

can reliably hang vscode each time i use lldb debugger with rust and close terminal afterwards with trash icon. This wasn't an issue for me a month ago, now it's unusable

[Tyriar]

@DzmitryFil you can set "terminal.integrated.windowsEnableConpty": false to go back to the Windows <= 1809 system.

[Eugeny]

I've been preventing hangs while closing PTY handles with this patch in Terminus for a while now:

diff --git a/src/win/conpty.cc b/src/win/conpty.cc
index 4500f6e..2159d8b 100644
--- a/src/win/conpty.cc
+++ b/src/win/conpty.cc
@@ -411,6 +411,11 @@ static NAN_METHOD(PtyResize) {
   return info.GetReturnValue().SetUndefined();
 }
 
+DWORD WINAPI Closer(LPVOID lpParam) {
+    CloseHandle((HANDLE)lpParam);
+    return 0;
+}
+
 static NAN_METHOD(PtyKill) {
   Nan::HandleScope scope;
 
@@ -435,7 +440,7 @@ static NAN_METHOD(PtyKill) {
     }
   }
 
-  CloseHandle(handle->hShell);
+  CreateThread(NULL, 0, Closer, (LPVOID)handle->hShell, 0, NULL);
 
   return info.GetReturnValue().SetUndefined();
 }

Not sure if there any implications in these threads just hanging around if the close hangs forever.

[JustinGrote]

@Tyriar yay! Any idea when this will work its way upstream to code?

[Tyriar]

@Eugeny I'd expect conpty to clean up the threads, is there something to worry about here if so?

@JustinGrote will probably land in tomorrow's insiders, already tested and it seems to work well. PR: microsoft/vscode#116185