Terminal leaks processes

[dbaeumer]

• VSCode Version: 1.13 insider
• OS Version: Windows

Steps to Reproduce:

1. clone vscode-eslint repository
2. chdir to eslint/eslint-server
3. open terminal (I use powershell as the terminal, not sure if it matters)
4. type npm install
5. type npm run watch
6. Kill the terminal by pressing the garbage bin button

Open a process explorer

Observe: there are still two process orphaned conhost.exe and node.exe

[dbaeumer]

This also affects the task runner when using the terminal as the execution engine

[dbaeumer]

Executing this natively in a PowerShell command prompt and using Ctrl+C to interrupt the process works as expected.

[dbaeumer]

Using node version 6.5.0 but happens with 7.10.0 for me as well.

[dbaeumer]

I just noticed that this also happens in Hyper.js. Looks like something has changed how winpty-agent launches processes.

[Tyriar]

Related: #26897

[Tyriar]

I actually can't reproduce using your steps on:

• VSCode Version: Code - Insiders 1.13.0-insider (7403245, 2017-05-23T05:14:19.493Z)
• OS Version: Windows_NT ia32 10.0.14393

I tried 3 times and every time both node and conhost processes are terminated after around 5 seconds max which is consistent with the code https://github.com/Microsoft/vscode/blob/f95866f07eaf1cdea610479a11778ae82f69bf4b/src/vs/workbench/parts/terminal/node/terminalProcess.ts#L129

Any more info on a repro would be good. What version of Windows are you on?

[dbaeumer]

My Windows version:

[dbaeumer]

I will see if I can find out more. However I do see them leak and I was even able to reproduce the leaking in Hyper.js. How does winpty-agent start the process. Is it using detach ?

[dbaeumer]

And from using processes in tasks heavily I know that if a sub process inherits the input/output stream from a parent parent process then the child process might get orphaned if the streams are still open. So in the case of VS Code -> winpty -> node it might happend that node inherits stdio from VS Code and when winpty gets killed node is still sticking around since the stream owned be VS Code are still around.

[Tyriar]

I believe it works like this:

VS Code (node-pty) -> winpty-agent -> conhost -> node

Just speculating here, but maybe we need to tell ptyProcess to die here: https://github.com/Microsoft/vscode/blob/66bc0d13fafadbddb5b26a3bc114bdcc29cfd39c/src/vs/workbench/parts/terminal/node/terminalProcess.ts#L127

[Tyriar]

I can reproduce on my home machine using npm run dev on xterm.js, the server created on npm start doesn't die. Here's what's happening according to process explorer:

After hitting killing the terminal via the button the process is detached, all processes die except for the node server one:

[Tyriar]

Something similar happens when doing the same in a regular cmd.exe window, you need to hit the X window button about 4 times though as it only kills 2 or so of the processes at once. It eventually kills them all though if you're persistent.

[Tyriar]

taskkill /pid <pid> doesn't kill the entire tree either:

[Tyriar]

I can kill all the processes forcefully by running this:

taskkill /F /t /pid <pid>

Not sure that's better though.

[Tyriar]

@rprichard do you know why some processes running in the shell on Windows (servers in particular) don't get killed but all other processes in the tree are?

[rprichard]

@dbaeumer Thanks for the test case. I was able to immediately reproduce the issue.

Some background:

• winpty.dll spawns winpty-agent.exe using CreateProcessW and bInheritHandles==FALSE: [code]

• winpty-agent.exe spawns the child process (e.g. PowerShell.exe) using CreateProcessW and typically bInheritHandles==FALSE: [code]

• winpty-agent.exe registers a console ctrl handler so it can generate Ctrl-C without terminating itself: [code]

• When VSCode (or any winpty client) destroys the console, winpty notices that its control pipe has closed and ultimately calls agentShutdown, which sends a WM_CLOSE message to the console window. Closing a console window normally terminates all the processes attached to the console. [code]

When I run npm run watch in an ordinary Windows console, then click the console's 'X' button, it kills some of the processes running in the console, but it doesn't kill the second node.exe process.

I found the problem on GitHub by searching for SetConsoleCtrlHandler in the libuv repo (libuv is the runtime that node.js runs on):

    case CTRL_CLOSE_EVENT:
      if (uv__signal_dispatch(SIGHUP)) {
        /* Windows will terminate the process after the control handler */
        /* returns. After that it will just terminate our process. Therefore */
        /* block the signal handler so the main loop has some time to pick */
        /* up the signal and do something for a few seconds. */
        Sleep(INFINITE);
        return TRUE;
      }
      return FALSE;

[link]

When I click the 'X' button of a console with npm run watch running, libuv intercepts the CTRL_CLOSE_EVENT event on a new thread, which somehow dispatches a "SIGHUP"... somewhere... and then waits forever. I'm guessing that libuv is assuming that the SIGHUP handler will terminate the process, but it doesn't always. (Aside: I think the Unix emulation is breaking down here, though, because a Unix program can ignore multiple SIGHUP signals, right? A libuv Windows program can only ignore a single CTRL_CLOSE_EVENT/SIGHUP event.)

When I click the 'X' button a second time, I guess Windows knows that the console ctrl handler is already running and decides to just kill the thing.

Two ideas for a fix:

1. winpty could better expose the Windows behavior with a new API that "requests" the console be closed (e.g. sends a WM_CLOSE message), but the winpty instance remains functioning until it really has closed. A second close request would (probably?) kill it for sure. In the VSCode UI, you could click the Kill Terminal button, and nothing would happen because the console wasn't dead yet. After you click it a second time, the winpty instance dies, then VSCode detects this and hides the GUI. (For example, I tested ConEmu with npm run watch, and ConEmu behaved this way -- I had to close ConEmu twice to get it to really close.)

2. No API change. Instead, winpty mimics libuv's CTRL_CLOSE_EVENT handler a bit and prevents its termination until it sees that all attached processes are gone (using GetConsoleProcessList). If they aren't gone after, say, 30 seconds, then it posts another WM_CLOSE, which is very likely to terminate everything. (Note that winpty-agent.exe itself is attached to the console, so that second WM_CLOSE will presumably kill winpty-agent.exe.)

Right now, I lean towards fix 2 because it seems like less trouble.

I'm not sure about the VSCode "plan B" thing. As long as it doesn't terminate winpty-agent.exe, I guess it's OK.

(Note to self: I wonder what happens if I register a new console ctrl handler between the two WM_CLOSE messages. Does Windows still terminate the process?)

[rprichard]

30 seconds

I suppose leaving npm run watch around for 30 seconds would be painful. Maybe 5 seconds is better? It could be configurable.

Actually, this seems like an argument for implementing fix 1, if that's feasible.

[Tyriar]

Created rprichard/winpty#129 in winpty.

[Tyriar]

@dbaeumer unfortunately I'm busy through September with Electron tasks and travel so I doubt I'll be able to get to this. I'm committed to making it a priority and explicitly putting it on the October iteration plan though.

[Tyriar]

@the-ress did you end up making any progress on your branch of node-pty? If it's still a WIP maybe I can take what you have when I free up and build upon it?

[dbaeumer]

@Tyriar thanks for letting me know.

[the-ress]

@Tyriar no, it's still WIP/PoC. Yes, of course you can.

[the-ress]

Created rprichard/winpty#130 with winpty part of the changes.

[Tyriar]

@the-ress just tested your winpty patch and it correctly returns the process list without the notepad process launched via start notepad 👍

[Tyriar]

Thanks to @the-ress this is now fixed at the node-pty level. 🎉

@the-ress when the winpty PR is finalized I'll pull the latest code into the next version of node-pty.

[edongashi]

I am experiencing this issue on win 10 x64.

Version 1.17.1
Commit 1e9d36539b0ae51ac09b9d4673ebea4e447e5353
Date 2017-10-10T14:24:50.851Z
Shell 1.7.7
Renderer 58.0.3029.110
Node 7.9.0
Architecture x64

node.exe gets orphaned when using npm run *** and closing vs code.

Tested with git bash and ps as terminal.

[Tyriar]

@edongashi the fix for this will be out in v1.18, you can try it out now in the Insiders build https://code.visualstudio.com/insiders

[edongashi]

Tried it. It's working as expected. Thanks!

[Beretta1979]

For me this is still occurring on for version 1.18 on Windows 10 with node js. If I kill the terminal using the garbage bin button the node processes are killed correctly. If VS Code is closed the processes keep running.

[TomasHubelbauer]

@Beretta1979 I am on Windows 10 and VS Code 1.18.0 and the integrated terminal I am using is cmd.exe. I used an external PowerShell terminal and did tasklist -fi "imagename eq node.exe" after doing npm start and closing using the bin button and after doing npm start and closing VS Code as a whole and in my case it worked as it should have. What is in your terminal.integrated.shell.windows setting?

[Beretta1979]

@TomasHubelbauer I tested this both with terminal.integrated.shell.windows set to powershell (default) as with cmd.exe