From e45a449e26096521122110dfa6da9bbcb68aaf4b Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 9 Nov 2021 20:59:40 -0800 Subject: [PATCH 1/2] Extract event handling to its own function. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/rust/engine/watch/src/lib.rs | 114 +++++++++++++++++-------------- 1 file changed, 62 insertions(+), 52 deletions(-) diff --git a/src/rust/engine/watch/src/lib.rs b/src/rust/engine/watch/src/lib.rs index a425faa804e..bf765a81781 100644 --- a/src/rust/engine/watch/src/lib.rs +++ b/src/rust/engine/watch/src/lib.rs @@ -38,7 +38,7 @@ use crossbeam_channel::{self, Receiver, RecvTimeoutError, TryRecvError}; use fs::GitignoreStyleExcludes; use log::{debug, trace, warn}; use notify::event::Flag; -use notify::{RecommendedWatcher, RecursiveMode, Watcher}; +use notify::{Event, RecommendedWatcher, RecursiveMode, Watcher}; use parking_lot::Mutex; use task_executor::Executor; @@ -63,7 +63,7 @@ type WatcherTaskInputs = ( Arc, PathBuf, crossbeam_channel::Sender, - Receiver>, + Receiver>, ); pub struct InvalidationWatcher(Mutex); @@ -145,7 +145,7 @@ impl InvalidationWatcher { ignorer: Arc, canonical_build_root: PathBuf, liveness_sender: crossbeam_channel::Sender, - watch_receiver: Receiver>, + watch_receiver: Receiver>, ) -> thread::JoinHandle<()> { thread::spawn(move || { let exit_msg = loop { @@ -157,55 +157,7 @@ impl InvalidationWatcher { break "The watcher was shut down.".to_string(); }; match event_res { - Ok(Ok(ev)) => { - let flag = ev.flag(); - let paths: HashSet<_> = ev - .paths - .into_iter() - .filter_map(|path| { - // relativize paths to build root. - let path_relative_to_build_root = if path.starts_with(&canonical_build_root) { - // Unwrapping is fine because we check that the path starts with - // the build root above. - path.strip_prefix(&canonical_build_root).unwrap().into() - } else { - path - }; - // To avoid having to stat paths for events we will eventually ignore we "lie" to the ignorer - // to say that no path is a directory, they could be if someone chmod's or creates a dir. - // This maintains correctness by ensuring that at worst we have false negative events, where a directory - // only glob (one that ends in `/` ) was supposed to ignore a directory path, but didn't because we claimed it was a file. That - // directory path will be used to invalidate nodes, but won't invalidate anything because its path is somewhere - // out of our purview. - if ignorer.is_ignored_or_child_of_ignored_path( - &path_relative_to_build_root, - /* is_dir */ false, - ) { - trace!("notify ignoring {:?}", path_relative_to_build_root); - None - } else { - Some(path_relative_to_build_root) - } - }) - .flat_map(|path_relative_to_build_root| { - let mut paths_to_invalidate: Vec = vec![]; - if let Some(parent_dir) = path_relative_to_build_root.parent() { - paths_to_invalidate.push(parent_dir.to_path_buf()); - } - paths_to_invalidate.push(path_relative_to_build_root); - paths_to_invalidate - }) - .collect(); - - // Only invalidate stuff if we have paths that weren't filtered out by gitignore. - if flag == Some(Flag::Rescan) { - debug!("notify queue overflowed: invalidating all paths"); - invalidatable.invalidate_all("notify"); - } else if !paths.is_empty() { - debug!("notify invalidating {:?} because of {:?}", paths, ev.kind); - invalidatable.invalidate(&paths, "notify"); - } - } + Ok(Ok(ev)) => Self::handle_event(&*invalidatable, &ignorer, &canonical_build_root, ev), Ok(Err(err)) => { if let notify::ErrorKind::PathNotFound = err.kind { warn!("Path(s) did not exist: {:?}", err.paths); @@ -227,6 +179,64 @@ impl InvalidationWatcher { }) } + fn handle_event( + invalidatable: &I, + ignorer: &GitignoreStyleExcludes, + canonical_build_root: &Path, + ev: Event, + ) { + let flag = ev.flag(); + let paths: HashSet<_> = ev + .paths + .into_iter() + .filter_map(|path| { + // relativize paths to build root. + let path_relative_to_build_root = if path.starts_with(canonical_build_root) { + // Unwrapping is fine because we check that the path starts with + // the build root above. + path.strip_prefix(canonical_build_root).unwrap().into() + } else { + path + }; + // To avoid having to stat paths for events we will eventually ignore we "lie" to + // the ignorer to say that no path is a directory (although they could be if someone + // chmod's or creates a dir). + // + // This maintains correctness by ensuring that at worst we have false negative + // events, where a directory-only glob (one that ends in `/` ) was supposed to + // ignore a directory path, but didn't because we claimed it was a file. That + // directory path will be used to invalidate nodes, but won't invalidate anything + // because its path is somewhere out of our purview. + if ignorer.is_ignored_or_child_of_ignored_path( + &path_relative_to_build_root, + /* is_dir */ false, + ) { + trace!("notify ignoring {:?}", path_relative_to_build_root); + None + } else { + Some(path_relative_to_build_root) + } + }) + .flat_map(|path_relative_to_build_root| { + let mut paths_to_invalidate: Vec = vec![]; + if let Some(parent_dir) = path_relative_to_build_root.parent() { + paths_to_invalidate.push(parent_dir.to_path_buf()); + } + paths_to_invalidate.push(path_relative_to_build_root); + paths_to_invalidate + }) + .collect(); + + // Only invalidate stuff if we have paths that weren't filtered out by gitignore. + if flag == Some(Flag::Rescan) { + debug!("notify queue overflowed: invalidating all paths"); + invalidatable.invalidate_all("notify"); + } else if !paths.is_empty() { + debug!("notify invalidating {:?} because of {:?}", paths, ev.kind); + invalidatable.invalidate(&paths, "notify"); + } + } + /// /// An InvalidationWatcher will never restart on its own: a consumer should re-initialize if this /// method returns an error. From 271ed3de6282080170d874878ad74ea4766eb2bd Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 9 Nov 2021 21:34:41 -0800 Subject: [PATCH 2/2] Do not invalidate parent paths for file-data-only events. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/engine/internals/scheduler.py | 5 +---- src/rust/engine/watch/src/lib.rs | 17 +++++++++++------ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/python/pants/engine/internals/scheduler.py b/src/python/pants/engine/internals/scheduler.py index 0f4ec38b61d..320d9a04cc7 100644 --- a/src/python/pants/engine/internals/scheduler.py +++ b/src/python/pants/engine/internals/scheduler.py @@ -277,10 +277,7 @@ def rule_graph_consumed_types( self.py_scheduler, root_subject_types, product_type ) - def invalidate_files(self, direct_filenames: Iterable[str]) -> int: - filenames = set(direct_filenames) - # TODO(#11707): Evaluate removing the invalidation of parent directories. - filenames.update(os.path.dirname(f) for f in direct_filenames) + def invalidate_files(self, filenames: Iterable[str]) -> int: return native_engine.graph_invalidate_paths(self.py_scheduler, tuple(filenames)) def invalidate_all_files(self) -> int: diff --git a/src/rust/engine/watch/src/lib.rs b/src/rust/engine/watch/src/lib.rs index bf765a81781..5ea63b46af5 100644 --- a/src/rust/engine/watch/src/lib.rs +++ b/src/rust/engine/watch/src/lib.rs @@ -37,8 +37,8 @@ use std::time::Duration; use crossbeam_channel::{self, Receiver, RecvTimeoutError, TryRecvError}; use fs::GitignoreStyleExcludes; use log::{debug, trace, warn}; -use notify::event::Flag; -use notify::{Event, RecommendedWatcher, RecursiveMode, Watcher}; +use notify::event::{Flag, ModifyKind}; +use notify::{Event, EventKind, RecommendedWatcher, RecursiveMode, Watcher}; use parking_lot::Mutex; use task_executor::Executor; @@ -185,7 +185,9 @@ impl InvalidationWatcher { canonical_build_root: &Path, ev: Event, ) { + let is_data_only_event = matches!(ev.kind, EventKind::Modify(ModifyKind::Data(_))); let flag = ev.flag(); + let paths: HashSet<_> = ev .paths .into_iter() @@ -218,16 +220,19 @@ impl InvalidationWatcher { } }) .flat_map(|path_relative_to_build_root| { - let mut paths_to_invalidate: Vec = vec![]; - if let Some(parent_dir) = path_relative_to_build_root.parent() { - paths_to_invalidate.push(parent_dir.to_path_buf()); + let mut paths_to_invalidate: Vec = Vec::with_capacity(2); + if !is_data_only_event { + // If the event is anything other than a data change event (a change to the content of + // a file), then we additionally invalidate the parent of the path. + if let Some(parent_dir) = path_relative_to_build_root.parent() { + paths_to_invalidate.push(parent_dir.to_path_buf()); + } } paths_to_invalidate.push(path_relative_to_build_root); paths_to_invalidate }) .collect(); - // Only invalidate stuff if we have paths that weren't filtered out by gitignore. if flag == Some(Flag::Rescan) { debug!("notify queue overflowed: invalidating all paths"); invalidatable.invalidate_all("notify");