microsoft/node-pty

Properly thread the handles received from the ConPTY

Tyriar opened this issue · 11 comments

JustinGrote commented

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

jerch commented

@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 commented

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

JustinGrote commented

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.

  • 👍3
JustinGrote commented

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

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

Tyriar commented

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.

  • 👍1
DzmitryFil commented

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 commented

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

  • 👍1
Eugeny commented

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 commented

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

Tyriar commented

@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

  • 👍1