search

microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.67k stars 28.68k forks source link

Remote development: Zombie file watcher processes not cleaned up on remote host #156690

Closed jvilk-stripe closed 2 years ago

jvilk-stripe commented 2 years ago

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

Steps to Reproduce:

  1. Open VS Code to a remote workspace over SSH with --disable-extensions.
  2. Reload the window.
  3. Observe that there are now two fileWatcher processes. You can run this program to see that they are both running and consuming inotify watchers.
  4. Close VS Code. Observe that the fileWatchers are still present.

Here's the proof of them still consuming watches via this script:

   INOTIFY
   WATCHES
    COUNT     PID USER     COMMAND
--------------------------------------
 [censored]    1344 jvilk    /pay/home/jvilk/.vscode-server/bin/3b889b090b5ad5793f524b5d1d39fda662b96a2a/node /pay/home/jvilk/.vscode-server/bin/3b889b090b5ad5793f524b5d1d39fda662b96a2a/out/bootstrap-fork --type=fileWatcher
  [censored]  130881 jvilk    /pay/home/jvilk/.vscode-server/bin/3b889b090b5ad5793f524b5d1d39fda662b96a2a/node /pay/home/jvilk/.vscode-server/bin/3b889b090b5ad5793f524b5d1d39fda662b96a2a/out/bootstrap-fork --type=fileWatcher

They both have the same parent process:

$ ps -o ppid= -p 1344
130615
$ ps -o ppid= -p 130881
130615

If you reload 5 times, you end up with 5 processes.

In 1.68, the fileWatcher process closes when the window is reloaded or closed. This appears to be a new bug.

This bug is causing Stripe engineers to consume all inotify watches on their development machines.

jvilk-stripe commented 2 years ago

Updated to indicate that I tested w/ extensions disabled.

jvilk-stripe commented 2 years ago

Validated that this does not happen when VS Code is open locally. Will update title/description to reflect that this is a remote development issue.

jvilk-stripe commented 2 years ago

The processes do seem to go away eventually, but it takes many minutes. Due to the size of our monorepo, having just a few of these around can cause all inotify watches to be exhausted.

bpasero commented 2 years ago

In 1.68, the fileWatcher process closes when the window is reloaded or closed. This appears to be a new bug. The processes do seem to go away eventually, but it takes many minutes.

@alexdima can you chime in, my understanding is that each client starts a management process and an extension host process which both should go away when the client disconnects (closes window, quits or reloads window). Did something change here?

@jvilk-stripe

Due to the size of our monorepo, having just a few of these around can cause all inotify watches to be exhausted.

It is possible to configure excludes in settings (via files.watcherExcludes) to reduce impact on inotify handles, did you try that?

jvilk-stripe commented 2 years ago

It is possible to configure excludes in settings (via files.watcherExcludes) to reduce impact on inotify handles, did you try that?

@bpasero Yup, I have already configured excludes to remove all third-party and codegenned artifacts in the repo. There's no more clear fat to trim there. VS Code watches far fewer files than Watchman or Bazel (which also run in the monorepo), but due to the size of our monorepo it is still a fair number of files.

As an aside, I wish we could just have VS Code use watchman for notifications to reduce the file watching duplication here.

EDIT: I know I say "just", but I understand that this would be a real feature undertaking -- not something that would take 5 minutes of time. I should be more careful with my wording!

bpasero commented 2 years ago

Its funny you mention that because under the hood we do use https://github.com/parcel-bundler/watcher and that has support for Watchman as a backend, we just never allowed to enable this because we have literally 0 testing on this backend since nobody is using Watchman here and could say if it actually works or not...

jvilk-stripe commented 2 years ago

@bpasero Oh, you are? For some reason, I thought I saw some commits by you that replaced this with some other watcher by default?

Well, if there is a way to somehow experiment with using watchman as the backend, please let me know!

alexdima commented 2 years ago

Bisect points to https://github.com/microsoft/vscode/compare/e71b6105ebb4fbcfbf38a1b62bcbafdb048d3468...e7751367f79f327b8103bee33a07c16817a2c7cd , with the most likely culprit being https://github.com/microsoft/vscode/commit/9396199ba5dbec22a298e0a4933da0d300405247 which fixes leaking extension host connections (and most likely introduces leaking management connections).

jvilk-stripe commented 2 years ago

Thank you all for your quick response to this issue! I look forward to the next stable release with this fix. :)

bpasero commented 2 years ago

@alexdima can you also put this to the release/1.70 branch? Just checking in if the change will be different from the one that landed on main.

alexdima commented 2 years ago

Done! --> https://github.com/microsoft/vscode/pull/157557

alexdima commented 2 years ago

Steps to verify:

weinand commented 2 years ago

I could reproduce the leaked watcher processes in VS Code 1.70 and I've verified that no watcher processes are leaked with the fix.