fix(filesystem): apply files.watcherExclude to all watchers#17630
fix(filesystem): apply files.watcherExclude to all watchers#17630safisa wants to merge 4 commits into
Conversation
`FileService.watch()` only appended the temporary-upload exclude and left `files.watcherExclude` to individual callers, so watchers requesting `excludes: []` - internal recursive watchers and plugin/language-server watchers created via `vscode.workspace.createFileSystemWatcher` - placed unbounded OS watches, and overlapping watchers whose exclude lists differed were never subsumed and produced duplicate change events. Resolve `files.watcherExclude` generically in `FileService.watch()` so every watcher shares the same base excludes: the native parcel `ignore` prunes excluded directories for all watchers, and the existing watcher subsumption collapses overlapping watchers into a single OS watch. Closes eclipse-theia#17247, closes eclipse-theia#10794
|
For reviewers on Linux: here is a self-contained diagnostic to verify the effect of this change on real inotify usage. It reads Requires Linux
|
| /** | ||
| * Resolve the effective exclude globs for a watcher: the caller-supplied `excludes`, the | ||
| * always-on temporary-upload exclude, and the user's `files.watcherExclude` preference. | ||
| * | ||
| * Applying `files.watcherExclude` here, for every watcher, rather than relying on individual | ||
| * callers, keeps the number of OS file watches (e.g. inotify watches on Linux) bounded even for | ||
| * watchers that request `excludes: []` - internal recursive watchers as well as plugin and | ||
| * language-server watchers created via `vscode.workspace.createFileSystemWatcher`. It also gives | ||
| * overlapping watchers a consistent set of excludes, so the watcher subsumption in `doWatch` can | ||
| * collapse them into a single OS watch instead of leaving duplicates that emit duplicate events. | ||
| */ | ||
| protected resolveWatcherExcludes(resource: URI, excludes: string[]): string[] { | ||
| const resolved = new Set(excludes); | ||
| // always ignore temporary upload files | ||
| resolved.add('**/theia_upload_*'); | ||
| const configured = this.preferences.get('files.watcherExclude', undefined, resource.toString()); | ||
| if (configured) { | ||
| for (const pattern of Object.keys(configured)) { | ||
| if (configured[pattern]) { | ||
| resolved.add(pattern); | ||
| } | ||
| } | ||
| } | ||
| return Array.from(resolved); | ||
| } |
There was a problem hiding this comment.
This looks like it closely mirrors MainFileSystemEventService.getExcludes. Do we want to keep both, or does this implementation render that one obsolete, since this is closer to the actual call to create the watcher?
Separately, (how) do we handle preference changes in this area? Now all watches are stamped with the excludes set at the time they're created. If the user changes their exclude preferences, who's responsible for making sure that watches are updated to reflect the change?
There was a problem hiding this comment.
Good catch on both.
Done the first — now that FileService.watch applies the excludes for every watcher, getExcludes here was redundant, so I dropped it and $watch just delegates. The merge behaviour is still covered by file-service-watcher.spec.ts.
On preference changes: fair point, and it's a real gap — watches are stamped at creation and nothing re-issues them when files.watcherExclude changes. That's pre-existing though (we've never re-watched on a change to that setting); this PR only widens where the excludes get applied. I'd rather not grow this PR for it and handle live re-watching as a follow-up, which pairs nicely with the ancestor-watch follow-up already noted in the description.
…exclude-all-watchers # Conflicts: # CHANGELOG.md
… in FileService.watch
What it does
Fixes #17247
Fixes #10794
FileService.watch()previously appended only the temporary-upload exclude and relied on individual callers to applyfiles.watcherExclude. As a result:excludes: []— internal recursive watchers, and plugin/language-server watchers created throughvscode.workspace.createFileSystemWatcher— placed OS file watches thatfiles.watcherExcludenever bounded, so large workspaces could still exhaust the inotify budget (ENOSPC/ "Unable to watch for file changes in this large workspace") even with excludes configured (file.watcherExclude preference doesn't prevent watcher from being created #10794).This resolves
files.watcherExcludegenerically insideFileService.watch(), so every watcher shares the same base excludes. The native@parcel/watcherignoreoption then prunes the excluded directories for all watchers, and the existing watcher subsumption collapses overlapping watchers into a single OS watch.Builds on #17598, which made the backend honor excludes via parcel's native
ignore.How to test
files.watcherExclude(e.g."**/node_modules/**": true) in a workspace containing a large excluded subtree.createFileSystemWatcher).Unit tests in
packages/filesystem/src/browser/file-service-watcher.spec.tscover the merge (enabled vs.false-valued patterns, caller-supplied excludes preserved, temporary-upload exclude retained) and that a child watcher requesting empty excludes is now subsumed by the root watcher.Follow-ups
A separate driver remains and is intentionally out of scope here: a watcher rooted at an ancestor of a workspace folder (e.g. a language server watching the parent directory to detect deletion of the workspace folder itself) still triggers a recursive crawl of sibling trees, because the backend always watches recursively.
files.watcherExcludecannot bound it since the watch root is outside the workspace. This warrants a dedicated issue/PR.Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers