Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to kill pty process on Windows #437

Open
t1m0thyj opened this issue Oct 29, 2020 · 12 comments
Open

Unable to kill pty process on Windows #437

t1m0thyj opened this issue Oct 29, 2020 · 12 comments
Labels
bug Issue identified by VS Code Team member as probable bug help wanted Issues identified as good community contribution opportunities windows

Comments

@t1m0thyj
Copy link

Environment details

  • OS version: Windows 10 2004
  • Node.js version: 14.15.0
  • node-pty version: 0.9.0

Issue description

Using the simple example script below, I am unable to kill the pty process on Windows 10. However, I can successfully kill the pty process on Ubuntu with the same script and identical versions of Node.js and node-pty.

Test Script

Based on https://github.com/microsoft/node-pty/blob/master/examples/killDeepTree/index.js

var os = require('os');
var pty = require('node-pty');

var shell = process.platform === 'win32' ? 'powershell.exe' : 'bash';

var ptyProcess = pty.spawn(shell, [], {
  name: 'xterm-color',
  cols: 80,
  rows: 30,
  cwd: os.homedir(),
  env: process.env
});

ptyProcess.onData((data) => process.stdout.write(data));

ptyProcess.write('ls\r');

setTimeout(() => {
  console.log('Killing pty');
  ptyProcess.kill();
}, 1000);

Output on Windows

The script hangs on the line "Killing pty" and never returns.

PS C:\Users\Timothy\Projects\test\node-pty-test> node index.js
Windows PowerShell
Copyright (C) Microsoft Corporation. All rights reserved.

Try the new cross-platform PowerShell https://aka.ms/pscore6

PS C:\Users\Timothy> ls
...

PS C:\Users\Timothy> Killing pty

Output on Ubuntu

The script finishes and returns to a prompt in the original directory.

timothy@timothy-Virtual-Machine:~/Desktop/node-pty-test$ node index.js
ls
timothy@timothy-Virtual-Machine:~$ ls
...
timothy@timothy-Virtual-Machine:~$ Killing pty
timothy@timothy-Virtual-Machine:~/Desktop/node-pty-test$
@Tyriar
Copy link
Member

Tyriar commented Oct 29, 2020

I can reproduce, not sure why this happens but you can always use process.exit() to workaround it.

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug help wanted Issues identified as good community contribution opportunities windows labels Oct 29, 2020
@t1m0thyj
Copy link
Author

In the example I could use process.exit() as a workaround, but unfortunately for my use case I can't. I need to use node-pty in a Jest test suite, where calling process.exit() would terminate Jest.

@Tyriar
Copy link
Member

Tyriar commented Oct 29, 2020

Ok, well this is up for grabs if someone wants to look into it. The likely culprit is either the process does not get killed correctly or something in windowsTerminal.ts or windowsPtyAgent.ts is not being released, preventing node from shutting down.

@jerch
Copy link
Collaborator

jerch commented Oct 29, 2020

@Tyriar Does node-pty populate the process id correctly under windows? If so, maybe its possible to force kill it by an external command called with childprocess.*. Not a nice solution but might help to work around blocking the mainthread forever.

@t1m0thyj
Copy link
Author

@jerch I tried to kill the pty process with process.kill(ptyProcess.pid) and it didn't help. Not sure if that's the same thing as your childprocess.* idea?

@Tyriar
Copy link
Member

Tyriar commented Oct 29, 2020

@jerch VS Code actually does that but only because I get reports about sometimes process trees not getting killed correctly, I'm pretty sure in the majority of cases pty.kill() works though.

@jerch
Copy link
Collaborator

jerch commented Oct 30, 2020

@t1m0thyj Cannot tell that for sure, imho process.kill(ptyProcess.pid) is similar to pty.kill() (would call the same C function from the same thread I guess), while a custom subprocess would run independently.

@Tyriar Ah ic, well if a kill subprocess gets the job done without segfaulting then it might be valid workaround for those corner cases. If really a dangling resource is the culprit (a forgotten handle in the C++ code), then it might provoke a segfault after an outer force-kill, just with the next access attempt by libuv (or any C++ code that still tries to access it).

@t1m0thyj
Copy link
Author

t1m0thyj commented Oct 30, 2020

@jerch Killing the process via an external command doesn't help:

require('child_process').exec(`taskkill /pid ${ptyProcess.pid} /T /F`);

I've also tried running taskkill in another PowerShell window to confirm that it runs successfully, yet my script still hangs.

@jerch
Copy link
Collaborator

jerch commented Nov 1, 2020

@t1m0thyj Then it is most likely some dangling resource in the C++ part, as @Tyriar already mentioned. You can try to call unref() on things like sockets used in the pty underneath. If the resource is not exposed to JS, well then you will have to hack on the C++ parts. Since I have no clue about that code, close guess what can keep the libuv event loop busy: thread primitves (should be joined when done), file and pipe handles (should be closed).

@meganrogge
Copy link
Contributor

happening on linux too microsoft/vscode#94877

@privatenumber
Copy link

I'm unable to set this project up locally to debug this (can there be a contribution guide for building on Windows?), but if it's helpful, I used why-is-node-running to determine what's preventing Node.js from exiting:

# PIPEWRAP
D:\a\node_modules\node-pty\lib\windowsPtyAgent.js:97 - this._inSocket = new net_1.Socket({
D:\a\node_modules\node-pty\lib\windowsTerminal.js:50 - _this._agent = new windowsPtyAgent_1.WindowsPtyAgent(file, args, parsedEnv, cwd, _this._cols, _this._rows, false, opt.useConpty, opt.conptyInheritCursor);
D:\a\node_modules\node-pty\lib\index.js:28           - return new terminalCtor(file, args, opt);

# PIPEWRAP
D:\a\node_modules\node-pty\lib\windowsPtyAgent.js:240 - this._outSocket.destroy();

# Timeout
D:\a\node_modules\node-pty\lib\eventEmitter2.js:40 - queue[i].call(undefined, data);
D:\a\node_modules\node-pty\lib\terminal.js:86      - this.on('exit', function (exitCode, signal) { return _this._onExit.fire({ exitCode: exitCode, signal: signal }); });

Looks like an open socket and a setTimeout?

@vidhimultiqos
Copy link

Any update for killing the node-pty process?

luciancooper added a commit to luciancooper/cli-screencast that referenced this issue Nov 23, 2022
The `useConpty` spawn option can be used to disable ConPTY usage until
microsoft/node-pty#437 is resolved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug help wanted Issues identified as good community contribution opportunities windows
Projects
None yet
Development

No branches or pull requests

6 participants