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

Process handles leaked by pty host on windows #134939

Closed
deepak1556 opened this issue Oct 12, 2021 · 2 comments
Closed

Process handles leaked by pty host on windows #134939

deepak1556 opened this issue Oct 12, 2021 · 2 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug candidate Issue identified as probable candidate for fixing in the next release confirmed Issue has been confirmed by VS Code Team member freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues insiders-released Patch has been released in VS Code Insiders terminal General terminal issues that don't fall under another label verified Verification succeeded windows VS Code on Windows issues

Comments

@deepak1556
Copy link
Collaborator

Does this issue occur when all extensions are disabled?: Yes/No

  • VS Code Version: 1.62.0-insider (system setup)
  • OS Version: Windows_NT arm64 10.0.22000

Steps to Reproduce:

  1. Open terminal, run handle tool on pty host to get base output
Handle type summary:
  <Unknown type>  : 66
  ALPC Port       : 3
  Desktop         : 1
  Directory       : 2
  Event           : 33
  File            : 23
  IoCompletion    : 8
  IRTimer         : 6
  Key             : 12
  Process         : 65
  Section         : 3
  Semaphore       : 13
  Thread          : 19
  TpWorkerFactory : 3
  WaitCompletionPacket: 9
  WindowStation   : 2
Total handles: 268
  1. Run some sequence of commands (for ex: yarn in vscode repo) and close the terminal
  2. Check handle output
Handle type summary:
  <Unknown type>  : 66
  ALPC Port       : 3
  Desktop         : 1
  Directory       : 2
  Event           : 34
  File            : 17
  IoCompletion    : 8
  IRTimer         : 8
  Job             : 1
  Key             : 12
  Process         : 5418
  Section         : 3
  Semaphore       : 9
  Thread          : 18
  TpWorkerFactory : 4
  WaitCompletionPacket: 10
  WindowStation   : 2
Total handles: 5616

Originally reported in https://twitter.com/BruceDawson0xB/status/1447668569626476548

@deepak1556 deepak1556 self-assigned this Oct 12, 2021
@deepak1556 deepak1556 added this to the October 2021 milestone Oct 12, 2021
@deepak1556 deepak1556 added bug Issue identified by VS Code Team member as probable bug confirmed Issue has been confirmed by VS Code Team member freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues terminal General terminal issues that don't fall under another label labels Oct 12, 2021
@deepak1556 deepak1556 added the windows VS Code on Windows issues label Oct 12, 2021
@deepak1556
Copy link
Collaborator Author

Update:

Started with an event trace based on the nice guide at https://randomascii.wordpress.com/2021/07/25/finding-windows-handle-leaks-in-chromium-and-others/
Screen Shot 2021-10-14 at 12 38 13 AM

Narrowing on the pty host process, the leaked handles are coming from vscode-windows-process-tree module. Specifically some call to openProcess does not close the related handle. After this it was just a matter of hunting the problematic code and a quick scan pointed to https://github.com/microsoft/vscode-windows-process-tree/blob/bc0ee891ca3df19dad46b023e3bb1266dfd1a205/src/process.cc#L44-L58, voila! Since this code path gets executed pretty much after every command execution in the terminal, it matches the observed leak.

With the following patch applied the handles from the pty host are not leaked anymore,

diff --git a/src/process.cc b/src/process.cc
index 1563537..239e266 100644
--- a/src/process.cc
+++ b/src/process.cc
@@ -55,6 +55,8 @@ void GetProcessMemoryUsage(ProcessInfo process_info[1024], uint32_t* process_cou
   if (GetProcessMemoryInfo(hProcess, &pmc, sizeof(pmc))) {
     process_info[*process_count].memory = (DWORD)pmc.WorkingSetSize;
   }
+
+  CloseHandle(hProcess);
 }

@Tyriar I think this worth a candidate fix.

@deepak1556 deepak1556 added the candidate Issue identified as probable candidate for fixing in the next release label Oct 13, 2021
deepak1556 added a commit to microsoft/vscode-windows-process-tree that referenced this issue Oct 13, 2021
@Tyriar Tyriar closed this as completed in 728f813 Oct 13, 2021
Tyriar added a commit that referenced this issue Oct 13, 2021
@Tyriar Tyriar reopened this Oct 13, 2021
@deepak1556
Copy link
Collaborator Author

\closedWith 728f813

@rzhao271 rzhao271 added the verified Verification succeeded label Oct 14, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2021
lemanschik pushed a commit to code-oss-dev/code that referenced this issue Nov 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug candidate Issue identified as probable candidate for fixing in the next release confirmed Issue has been confirmed by VS Code Team member freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues insiders-released Patch has been released in VS Code Insiders terminal General terminal issues that don't fall under another label verified Verification succeeded windows VS Code on Windows issues
Projects
None yet
Development

No branches or pull requests

4 participants
@deepak1556 @Tyriar @rzhao271 and others