-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
Watcher: use a standard native file watching library across all OS #132483
Comments
I think #40898 is a blocker for this. |
Your explode on large folders link is broken btw |
@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 |
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 |
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 Some ideas of actions:
|
I think we have to fork to move ahead with this if it doesn't get merged, that plan sounds good. |
@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. Seems like at least one user is complaining about NSFW CPU usage. |
@paulmillr here is a test comparing In both cases I used the code from the respective My understanding is that |
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. |
@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 So, going with |
Using one watcher instead of three projects is a good refactoring/optimization. My question is this:
|
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. |
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 (@heejaechang beat me to commenting, ha) |
@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 On top of that, both our file watcher on Windows/macOS that was used so far for 1-folder workspaces and 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. |
Comparing the
|
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
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.
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? |
That does not make sense to me though because
That is with the custom watcher you use from your extension for watching folders that are outside of the workspace?
We can easily fork
Yes please. Btw is this Windows only or macOS as well? |
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.) |
@jakebailey thanks. just to clarify, PS: the only platform where |
https://github.com/microsoft/vscode-nsfw/commit/97c11885bd03d36f5c6286b8bdbfbc1ec649f295 in our I will try to get a insiders build out tomorrow using that updated library for you to check. |
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. |
@jakebailey @heejaechang can you retest with today's insider build to see if my fix works? |
@bpasero yep. it no longer repro! thank you! |
nsfw
as main watcher across all OS
@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: But would be nice if you could verify once more. |
FYI: Tested parcel-watcher on linux. When I add I tested ignoring 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,
|
@ghuser thanks a ton for this kind of thorough verification 👏 |
@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. |
Thanks! |
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
The text was updated successfully, but these errors were encountered: