winpty leaks processes

[Tyriar]

Forked from microsoft/vscode#26807, node servers in particular don't get killed when killing the winpty agent. Grabbing the process list via GetConsoleProcessList and killing them manually in winpty is probably the right solution.

[rprichard]

Thanks for filing the issue. I don't know when I'll get around to working on this issue, but it's probably something that should be fixed (or at least improved).

Comments:

• I occasionally think jobs are the fix for this issue, but I suspect they don't help because a GUI program spawned from a winpty shell would be part of the same job as the shell (probably?), and then closing the winpty terminal would terminate the GUI program.

• Ordinarily, closing a console sends the processes a CTRL_CLOSE_EVENT signal, giving them a chance to clean up (for ~5 seconds). It would be good if winpty maintained this behavior rather than simply terminate all processes on the console process list.

• If winpty were to terminate a PID from the console process list, then perhaps it should double-check that the PID is still on the console process list after opening the process handle. Otherwise, there is a race condition where a PID could be reused after calling GetConsoleProcessList but before calling OpenProcess.

• It looks like GenerateConsoleCtrlEvent is unable to synthesize CTRL_CLOSE_EVENT. I tested a GenerateConsoleCtrlEvent(CTRL_CLOSE_EVENT, 0) call on Win10_16257, and MSDN states that only CTRL_C_EVENT and CTRL_BREAK_EVENT are allowed.

• The GetConsoleProcessList process order is wrong in 15063's v2 console (Console process order changed in 15063's v2 console making npm harder to close microsoft/WSL#2170).

• The process order also determines the order that the console tries to kill processes. I believe (but haven't conclusively determined) that the process order is reverse of (or the same as) the order that processes attached to the console. (XXX: If a process detaches and reattaches, its PID should move to the front of (or the rear of) the process list. This should be tested.)

• I really need to test what happens if processes detach or attach from a console during the close operation. I already determined that if a process in the console list exits prematurely during the close operation, the close operation aborts. (See closetest.cc in Console process order changed in 15063's v2 console making npm harder to close microsoft/WSL#2170.)

Possible solution:

winpty can determine whether the console process order is in correct order or reversed order by looking for its own PID in the process list.

• If the order is correct, the agent process could close the console by repeatedly sending/posting WM_CLOSE until the agent process terminates. I'm guessing posting won't work because it won't block, and even a successful close operation operation can take arbitrarily long. (See closetest.cc in Console process order changed in 15063's v2 console making npm harder to close microsoft/WSL#2170 for an example.) Does SendMessage of WM_CLOSE block for the correct amount of time, and could it deadlock?

• If the order is wrong, the agent process could spawn a process whose job was to clean up. The cleanup task would repeatedly send/post WM_CLOSE, and it would be the last process to be killed, unless other/new processes attach to the console. Maybe it could have a CTRL_CLOSE_EVENT handler that respawns itself if it sees that it isn't the last process remaining in the console process list.

[Sader82]

Is there a solution already? I'm running the latest version of vscode and powershell module, and still have the leaking process issue

[Tyriar]

@Sader82 the problem is fixed in the latest insiders by making use of #130

I'm not sure what you're seeing but it's different to the leaking processes problem, where processes would be orphaned (no longer descendants of the Code.exe tree) and continue running.

[Sader82]

@Tyriar this happends when the integrated terminal for powershell is active.
killing the terminal stops cloning (leaking) the process.

Perhaps it is fixed in the new version. I do not run ths insider build..