fix: ensure proper cleanup of console process on shutdown #755
Conversation
deepak1556
force-pushed
the
robo/fix_process_cleanup
branch
from
16ab761 to
483e7c7
Compare
Tyriar
left a comment
Tyriar
left a comment
There was a problem hiding this comment.
This also means we don't need the async PR either?
| }); | ||
| (this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll); | ||
| }); | ||
| (this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll); |
There was a problem hiding this comment.
I think this means we can remove conpty_console_list_agent.ts and win/conpty_console_list.cc
There was a problem hiding this comment.
Yeah left it as such incase you had use cases in future. Happy to remove them.
There was a problem hiding this comment.
I retain the old tree kill logic for useConpty mode while useConptyDll mode uses the new logic. This allows for a possible fallback incase things go horribly wrong. We can merge the two paths once we gain confidence.
Yeah it won't be needed with this change |
| }; | ||
|
|
||
| static std::vector<pty_baton*> ptyHandles; | ||
| static std::vector<std::unique_ptr<pty_baton>> ptyHandles; |
There was a problem hiding this comment.
Small cleanup to ensure proper lifetime management when the process exits.
| CloseHandle(baton->hIn); | ||
| CloseHandle(baton->hOut); | ||
| CloseHandle(baton->hShell); | ||
| assert(remove_pty_baton(baton->id)); |
There was a problem hiding this comment.
We were previously closing the client handle in pty.kill which would invalidate waiting on the exit callback. Moved it closer to when the client has signaled exit.
Fix for microsoft/vscode#225719
Fixes #733
Based on etl profiles, we see that the OpenConsole processes are leaked since they are waiting on I/O from the pseudo client processes that are not terminated even when
pty.killcommand was issued.The changes associates all the client process to a job handle with
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSEand the handle gets destroyed when the process hosting pty module gets torn down.