Watcher: use a standard native file watching library across all OS

[bpasero]

Growing tired of maintaining 3 different file watcher solutions and the one being known to explode on large folders, I would suggest to make a native file watching library our one and only watcher across all platforms and independent from multi root or not.

//cc @Tyriar @aeschli @deepak1556

[Tyriar]

I think #40898 is a blocker for this.

[Tyriar]

Your explode on large folders link is broken btw

[bpasero]

@Tyriar thanks, fixed the link. I am aware of #40898, but I am not sure why users cannot increase the handle count as suggested in docs as a workaround.

Btw, even with chokidar you can hit the same issue if our default rules don't capture large folders. So I would argue that users even with chokidar on Linux are used to getting this error notification.

[Tyriar]

Increasing the handle count would work, but it's an extra step as it's not the default in the OS and then it ends up using excessive resources. The real workaround should be files.watcherExclude imo, not modifying an OS-wide setting.

[bpasero]

Yeah, it is too bad that users cannot change this for VSCode alone. And it needs admin rights to make changes to that file, which is less than ideal.

On the other hand I never really liked how cumbersome it is to edit files.watcherExclude. It is easy to configure it wrong, so I wonder if there is an alternative where you could simply put the watcher in a "low resource" mode where only opened files and maybe visible folders in the explorer get watched up to the limit of the OS.

Some ideas of actions:

• enable nsfw by default in insiders (Sept, Oct)
• fork nsfw to incorporate exclude support such as the one suggested in linux: support simple ignore patterns Axosoft/nsfw#124
• look for more potential issues we need to fix to align with what we have today
• enable nsfw by default in stable in November with a setting to fallback to the others
• eventually remove that fallback at the end of the year

[Tyriar]

I think we have to fork to move ahead with this if it doesn't get merged, that plan sounds good.

[paulmillr]

@bpasero maintaining 3 watchers is not great.

I'm curious about the status of the claim that chokidar explodes though. The issue was created when chokidar 2 was out, 5 years ago.

Also, i've just did double-test and CPU usage is tiny with VSCode repo on Mac. watcherExclude is empty.

Seems like at least one user is complaining about NSFW CPU usage.

[bpasero]

@paulmillr here is a test comparing chokidar with nsfw on Windows (11, ARM) on a large project folder that has a few copies of vscode checked out:

Chokidar

NSFW

In both cases I used the code from the respective README.md to get going.

My understanding is that chokidar is recursively walking the folder tree in JavaScript code while nsfw is not. The high CPU rise for chokidar seems to originate from that fact, while nsfw is just calling ReadDirectoryChangesW on Windows which is a recursive watcher without overhead.

[paulmillr]

Windows has been a problematic spot, that's true. The backend aka ReadDirectoryChangesW there is not really used.

I'm talking about the issue # you've mentioned though, which says it happens on Linux and Darwin.

[bpasero]

@paulmillr thats true, but only because currently on Windows we use yet a third watcher: https://github.com/microsoft/vscode-filewatcher-windows written by us in C# leveraging ReadDirectoryChangesW.

So, going with chokidar for Windows is currently not an option and I also believe Windows is our largest user base.

[paulmillr]

Using one watcher instead of three projects is a good refactoring/optimization.

My question is this:

1. Are you confident that nsfw will have less issues similar to File watcher resulting in high CPU use on Mac/Linux for large folders #3998?
2. Am I correct in stating that there seem to be no performance problems on Linux and Macos?

[heejaechang]

we (Pylance) are having problem due to file watcher's behavior change. it looks like new file watcher doesn't provide a way to distinguish whether events we got was due to "modification" or merely a touch (such as read) on the file.

if it is going to raise event for file read, please provide a way to distinguish it with actual file modification.

[jakebailey]

In testing out things for a Pylance release today, @heejaechang hit a case on his insiders install where we're receiving what appears to be read accesses in the workspace (a virtualenv), so an event is sent to us, which then triggers analysis (as we're trying to watch for library installs), which then reanalyzes and causes another read access, and so on. This doesn't happen in stable, so by thumbing through commits, I found the changes for this issue, and I'm guessing it's the cause.

It seems like NSFW behaves a little bit differently than the watcher that used to be used in Windows, and may cause regressions in extensions like ours which aren't expecting to get read events. (Arguably, nobody should have to deal with read events.)

We currently do the same trick VS Code does to attempt to distinguish read accesses on Windows (the paths for reads come in relative rather than absolute, so are easier to filter), but potentially that was specific to the old set of watchers, as our extension as it exists now loops indefinitely.

---

Pylance also needs to watch files outside the workspace (something that VS Code and the LSP do not offer) to detect package installs, so we also have to replicate similar code on our end. The last time we tried enabling it (using chokidar), it similarly blew up on Windows and macOS, so we've had to revert it in hope we can come up with another solution. Not to throw another watcher into the mix (as if fs.watch, chokidar, nsfw, and the C# one weren't enough...), but in my testing watcher has also performed very well for our needs, we've just been hesitant to change that.

(@heejaechang beat me to commenting, ha)

[bpasero]

@jakebailey @heejaechang just to clarify, this is not a new file watcher, but a file watcher that was in the use when a user opens a .code-workspace file (= multi root workspace) ever since we had multi root workspaces, so this issue must be ~5y old for some users at least.

On top of that, both our file watcher on Windows/macOS that was used so far for 1-folder workspaces and nsfw use ReadDirectoryChangesW and fsevents as the underlying method to figure out change events, so I would expect a similar amount of events coming out of both. This is not a different way of file watching, it is actually the same way of watching. The only difference maybe into how these libraries are configured and maybe some events are being ignored in chokidar that nsfw let's through.

I just did some testing opening files on Windows and macOS observing file events and I do not see change events when just opening a file. So please come up with some steps how to get into this state.

[bpasero]

Comparing the ReaddirectoryChangesW flags used by nsfw vs us indicates that nsfw is probably catching more events than we do:

nsfw
https://github.com/Axosoft/nsfw/blob/e8a6e953b5ec4ed09b1e73cea944fcfc6149ea3e/src/win32/Watcher.cpp#L147-L154

vscode-windows-C# watcher
https://github.com/microsoft/vscode-filewatcher-windows/blob/ed602d617ea62a71a3f740b1137799570f67792a/FileWatcher/FileWatcher.cs#L42

[jakebailey]

just to clarify, this is not a new file watcher

I'm certainly aware of the fact that many watcher libraries use the same underlying watchers and post-process them, and that VS Code uses many in different scenarios; I'm not claiming that any of this is "new" to VS Code, just that we saw a change in behavior recently. We too have to do more post processing to attempt to deal with events that have been fired with the wrong event type (always change, never delete), and other workarounds to deal with missed events (like folder deletion).

I just did some testing opening files on Windows and macOS observing file events and I do not see change events when just opening a file. So please come up with some steps how to get into this state.

I've attempted to emulate one of our tests here (Python/Pylance have to be installed): https://github.com/jakebailey/bad-file-watcher

But, it's really inconsistent whether or not this manifests. I was able to replicate it only a few times, then further reopens didn't trigger it. I'm not sure if this is something like Windows Defender kicking in and scanning the files (then, deciding they're okay), or what. I will retry again in the morning.

Comparing the ReaddirectoryChangesW flags used by nsfw vs us indicates that nsfw is probably catching more events than we do:

Those additional options would explain it, I think; I had previously tested every file watcher for node I could get my hands on (for out-of-workspace watching), and dismissed nsfw and VS Code's fork of it for this reason. I could reliably make it send an event for simple accesses to files; perhaps that's the repro you want?

[bpasero]

@jakebailey

just that we saw a change in behavior recently.

That does not make sense to me though because nsfw is still at the same version. To verify this claim, just try your scenario in a multi-root workspace with VSCode stable (1.60).

We too have to do more post processing to attempt to deal with events that have been fired with the wrong event type (always change, never delete), and other workarounds to deal with missed events (like folder deletion).

That is with the custom watcher you use from your extension for watching folders that are outside of the workspace?

and dismissed nsfw and VS Code's fork of it for this reason

We can easily fork nsfw again (currently not using the fork) to match the file event handling on Windows as it is with our own Windows file watcher.

I could reliably make it send an event for simple accesses to files; perhaps that's the repro you want?

Yes please.

Btw is this Windows only or macOS as well?

[jakebailey]

I'll do more testing tomorrow, but I can say that we aren't using any external watcher right now. The post-processing I'm describing is performed on the events provided over LSP from the VS Code API itself. We have found them to be inaccurate.

I'm not saying that the nsfw version changed; it was my impression from the commits made here that it's now enabled by default for Windows in more scenarios, as opposed to the C# watcher.

I'm only focusing on Windows at the moment. I have not tested on macOS, where our issues using various watchers directly did not involve file access events; it's not related to what we saw earlier during our release testing.

(This is unrelated, but anything that depends on fsevents is very irritating to work with from the POV of publishing a cross platform VSIX, thanks to package managers not installing it outside of macOS; that's one reason why I'm considering using "watcher" for our external use among other reasons, in lieu of VS Code allowing access for watchers outside the workspace.)

[bpasero]

@jakebailey thanks. just to clarify, nsfw is used on Windows as well in multi-root workspaces. It was our original intent 5y ago to replace all other watching solutions with nsfw so we enabled it only for multi-root scenarios, which targets a lower user base (maybe 200.000 users) to get some testing and feedback. The next step in this journey is to enable it for all users and remove the others.

PS: the only platform where nsfw is currently not enabled by default (unless you are in multi-root workspace) is Linux because unfortunately nsfw does not support ignore patterns (our files.watcherExclude setting) and each folder in a hierarchy is counted as a opened file handle against the limited set of file handles in the OS, forcing users to increase that limit. Plan is to implement that support in October and then also get Linux on board.

[bpasero]

https://github.com/microsoft/vscode-nsfw/commit/97c11885bd03d36f5c6286b8bdbfbc1ec649f295 in our vscode-nsfw fork changes the event types to what we had used in our C# solution: FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_DIR_NAME | FILE_NOTIFY_CHANGE_LAST_WRITE

I will try to get a insiders build out tomorrow using that updated library for you to check.

[heejaechang]

event on file read has happened before as well, but there was a hack to distinguish whether event was due to file read or due to file change. now, it seems there is no way to make a distinction so our old hack (which we got from other team doing the same hack) no longer works.

just FYI, the hack is checking whether given file path on "change" event is relative path or absolute path. for some reasons, it raised "change' event with relative path for file read and "change" event with absolute path for file modification.

[bpasero]

@jakebailey @heejaechang can you retest with today's insider build to see if my fix works?

[heejaechang]

can you retest with today's insider build to see if my fix works?

@bpasero yep. it no longer repro! thank you!

[bpasero]

@jakebailey @heejaechang fyi starting tomorrow we will ship yet another new file watcher (https://github.com/parcel-bundler/watcher). I have checked their windows implementation and while they seem to be using a few more flags, I doubt it would result in file events on read like the previous one was:

https://github.com/parcel-bundler/watcher/blob/4b4389e750ff32c9c1f9feb149545d54cfd62557/src/windows/WindowsBackend.cc#L143-L144

But would be nice if you could verify once more.

[ghuser]

FYI: Tested parcel-watcher on linux. When I add node_modules/ in the 'ignore' array and check with inotify-consumers indeed it does not consume any inotify watches for that directory and thus also does not deliver any events.

I tested ignoring node_modules both with absolute and relative path and it works for both (inotify watchers are not added for that dir and subdirs).

glob-patterns are not supported but I think this is not really an issue since excluded paths (from glob-patterns) paths can be calculated at vscode-side.

Also, as desired, read events are not delivered for non-excluded directories.

.unsubscribe() removes the corresponding inotify watches. An additional call to subscribe() (after awaiting to subscription.unsubscribe() ) with different exclusion rules adds inotify watches based on the new rules only (as expected).

[bpasero]

@ghuser thanks a ton for this kind of thorough verification 👏

[heejaechang]

@bpasero I just tested on vscode v1.61.0 and vscode insider v 1.62.0-insider and both of them seems working as expected.

[bpasero]

Thanks!