From d7152e2bcc7cd49868fc6d0ed8336cee56bfdadb Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Thu, 14 May 2020 14:12:47 -0700 Subject: [PATCH] Move away from the debounced notify watcher (#9754) ### Problem We're on a slightly older version of the `notify` crate because we wanted to use the "debounced" API, which has a thread that attempts to batch events and dedupe them where possible. But the implementation of debouncing has issues like https://github.com/notify-rs/notify/issues/205. ### Solution Move away from the debounced watcher towards the trustier direct delivery of events: post #9636 we do our own debouncing via the Graph not double-cleaning `Nodes`, and via the `--loop` delay. Additionally, fix two cases where code and tests used to race pants' invalidation and assume (intentionally or otherwise) that pants would not notice created files. ### Result Reduced risk of lost `notify` events. [ci skip-jvm-tests] --- .../pants/contrib/go/tasks/go_buildgen.py | 29 ++++++++++------- .../pants/engine/internals/mapper_test.py | 5 --- src/rust/engine/Cargo.lock | 31 +++++++++---------- src/rust/engine/watch/Cargo.toml | 6 +--- src/rust/engine/watch/src/lib.rs | 10 ++++-- 5 files changed, 41 insertions(+), 40 deletions(-) diff --git a/contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py b/contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py index e3d7c8af045..eb3881ecdda 100644 --- a/contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py +++ b/contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py @@ -330,14 +330,21 @@ def _materialize(self, generation_result): remote = self.get_options().remote existing_go_buildfiles = set() - def gather_go_buildfiles(rel_path): - address_mapper = self.context.address_mapper - for build_file in address_mapper.scan_build_files(base_path=rel_path): - existing_go_buildfiles.add(build_file) + # We scan for existing BUILD files containing Go targets before we begin to materialize + # things, because once we have created files (possibly next to existing files), collisions + # between the existing definitions and the new definitions are possible. + def gather_go_buildfiles(rel_path, is_relevant_target): + for build_file in self.context.address_mapper.scan_build_files(rel_path): + spec_path = os.path.dirname(build_file) + for address in self.context.address_mapper.addresses_in_spec_path(spec_path): + if is_relevant_target(self.context.build_graph.resolve_address(address)): + existing_go_buildfiles.add(address.rel_path) - gather_go_buildfiles(generation_result.local_root) + gather_go_buildfiles(generation_result.local_root, lambda t: isinstance(t, GoLocalSource)) if remote and generation_result.remote_root != generation_result.local_root: - gather_go_buildfiles(generation_result.remote_root) + gather_go_buildfiles( + generation_result.remote_root, lambda t: isinstance(t, GoRemoteLibrary) + ) targets = set(self.context.build_graph.targets(self.is_go)) if remote and generation_result.remote_root: @@ -347,6 +354,7 @@ def gather_go_buildfiles(rel_path): remote_root = os.path.join(get_buildroot(), generation_result.remote_root) targets.update(self.context.scan(remote_root).targets(self.is_remote_lib)) + # Generate targets, and discard any BUILD files that were overwritten. failed_results = [] for result in self.generate_build_files(targets): existing_go_buildfiles.discard(result.build_file_path) @@ -354,15 +362,12 @@ def gather_go_buildfiles(rel_path): if result.failed: failed_results.append(result) + # Finally, unlink any BUILD files that were invalidated but not otherwise overwritten. if existing_go_buildfiles: deleted = [] for existing_go_buildfile in existing_go_buildfiles: - spec_path = os.path.dirname(existing_go_buildfile) - for address in self.context.address_mapper.addresses_in_spec_path(spec_path): - target = self.context.build_graph.resolve_address(address) - if isinstance(target, GoLocalSource): - os.unlink(os.path.join(get_buildroot(), existing_go_buildfile)) - deleted.append(existing_go_buildfile) + os.unlink(os.path.join(get_buildroot(), existing_go_buildfile)) + deleted.append(existing_go_buildfile) if deleted: self.context.log.info( "Deleted the following obsolete BUILD files:\n\t{}".format( diff --git a/src/python/pants/engine/internals/mapper_test.py b/src/python/pants/engine/internals/mapper_test.py index 8a714b01ee3..f25a0341cc5 100644 --- a/src/python/pants/engine/internals/mapper_test.py +++ b/src/python/pants/engine/internals/mapper_test.py @@ -204,11 +204,6 @@ def test_no_address_no_family(self) -> None: build_file = os.path.join(self.build_root, "a/c", "c.BUILD.json") with safe_open(build_file, "w") as fp: fp.write('{"type_alias": "struct", "name": "c"}') - - # Exists on disk, but not yet in memory. - with self.assertRaises(Exception): - self.resolve(spec) - self.scheduler.invalidate_files(["a/c"]) # Success. diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index 06236faaf0d..d3d2622d005 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -1046,16 +1046,16 @@ dependencies = [ [[package]] name = "fsevent" -version = "0.4.0" +version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "bitflags 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", - "fsevent-sys 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)", + "fsevent-sys 3.0.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] name = "fsevent-sys" -version = "2.0.1" +version = "3.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "libc 0.2.69 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1539,7 +1539,7 @@ dependencies = [ [[package]] name = "inotify" -version = "0.7.0" +version = "0.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "bitflags 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1961,18 +1961,17 @@ dependencies = [ [[package]] name = "notify" -version = "5.0.0-pre.1" -source = "git+https://github.com/notify-rs/notify?rev=fba00891d9105e2f581c69fbe415a58cb7966fdd#fba00891d9105e2f581c69fbe415a58cb7966fdd" +version = "5.0.0-pre.2" +source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "anymap 0.12.1 (registry+https://github.com/rust-lang/crates.io-index)", "bitflags 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "chashmap 2.2.2 (registry+https://github.com/rust-lang/crates.io-index)", - "crossbeam-channel 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)", + "crossbeam-channel 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)", "filetime 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)", - "fsevent 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", - "fsevent-sys 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)", - "inotify 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", - "kernel32-sys 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", + "fsevent 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)", + "fsevent-sys 3.0.0 (registry+https://github.com/rust-lang/crates.io-index)", + "inotify 0.8.2 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.69 (registry+https://github.com/rust-lang/crates.io-index)", "mio 0.6.22 (registry+https://github.com/rust-lang/crates.io-index)", "mio-extras 2.0.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3784,7 +3783,7 @@ dependencies = [ "hashing 0.0.1", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "logging 0.0.1", - "notify 5.0.0-pre.1 (git+https://github.com/notify-rs/notify?rev=fba00891d9105e2f581c69fbe415a58cb7966fdd)", + "notify 5.0.0-pre.2 (registry+https://github.com/rust-lang/crates.io-index)", "parking_lot 0.6.4 (registry+https://github.com/rust-lang/crates.io-index)", "task_executor 0.0.1", "tempfile 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3993,8 +3992,8 @@ dependencies = [ "checksum foreign-types 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)" = "f6f339eb8adc052cd2ca78910fda869aefa38d22d5cb648e6485e4d3fc06f3b1" "checksum foreign-types-shared 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "00b0228411908ca8685dba7fc2cdd70ec9990a6e753e89b6ac91a84c40fbaf4b" "checksum fs2 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)" = "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213" -"checksum fsevent 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "5ab7d1bd1bd33cc98b0889831b72da23c0aa4df9cec7e0702f46ecea04b35db6" -"checksum fsevent-sys 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "f41b048a94555da0f42f1d632e2e19510084fb8e303b0daa2816e733fb3644a0" +"checksum fsevent 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "1616e68919f49d311720c3cf316e0a3522d8f2bd08f8da35f6b8a0fa12f9234b" +"checksum fsevent-sys 3.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "a41f1722e9bf862f62429d192f37d0c82c589aa18783aa06f0c4e5c3c90649fb" "checksum fuchsia-cprng 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "a06f77d526c1a601b7c4cdd98f54b5eaabffc14d5f2f0296febdc7f357c6d3ba" "checksum fuchsia-zircon 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "2e9763c69ebaae630ba35f74888db465e49e259ba1bc0eda7d06f4a067615d82" "checksum fuchsia-zircon-sys 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "3dcaa9ae7725d12cdb85b3ad99a434db70b468c09ded17e012d86b5c1010f7a7" @@ -4040,7 +4039,7 @@ dependencies = [ "checksum im-rc 12.3.4 (registry+https://github.com/rust-lang/crates.io-index)" = "e882e6e7cd335baacae574b56aa3ce74844ec82fc6777def7c0ac368837dc3d5" "checksum indexmap 1.3.2 (registry+https://github.com/rust-lang/crates.io-index)" = "076f042c5b7b98f31d205f1249267e12a6518c1481e9dae9764af19b707d2292" "checksum indicatif 0.14.0 (registry+https://github.com/rust-lang/crates.io-index)" = "49a68371cf417889c9d7f98235b7102ea7c54fc59bcbd22f3dea785be9d27e40" -"checksum inotify 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)" = "24e40d6fd5d64e2082e0c796495c8ef5ad667a96d03e5aaa0becfd9d47bcbfb8" +"checksum inotify 0.8.2 (registry+https://github.com/rust-lang/crates.io-index)" = "bc39ee997811267bf8aa0b10e1674c5bea6caacc1957eede5ea45251fe33c6d5" "checksum inotify-sys 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)" = "e74a1aa87c59aeff6ef2cc2fa62d41bc43f54952f55652656b18a02fd5e356c0" "checksum iovec 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)" = "b2b3ea6ff95e175473f8ffe6a7eb7c00d054240321b84c57051175fe3c1e075e" "checksum itertools 0.7.11 (registry+https://github.com/rust-lang/crates.io-index)" = "0d47946d458e94a1b7bcabbf6521ea7c037062c81f534615abcad76e84d4970d" @@ -4082,7 +4081,7 @@ dependencies = [ "checksum nails 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "e878cb7ecafadf84e7f41c9b58b104d522bb3c861340c80364fb56e289f99683" "checksum net2 0.2.34 (registry+https://github.com/rust-lang/crates.io-index)" = "2ba7c918ac76704fb42afcbbb43891e72731f3dcca3bef2a19786297baf14af7" "checksum nom 5.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "0b471253da97532da4b61552249c521e01e736071f71c1a4f7ebbfbf0a06aad6" -"checksum notify 5.0.0-pre.1 (git+https://github.com/notify-rs/notify?rev=fba00891d9105e2f581c69fbe415a58cb7966fdd)" = "" +"checksum notify 5.0.0-pre.2 (registry+https://github.com/rust-lang/crates.io-index)" = "7b00c0b65188bffb5598c302e19b062feb94adef02c31f15622a163c95d673c3" "checksum num 0.1.42 (registry+https://github.com/rust-lang/crates.io-index)" = "4703ad64153382334aa8db57c637364c322d3372e097840c72000dabdcf6156e" "checksum num-bigint 0.1.44 (registry+https://github.com/rust-lang/crates.io-index)" = "e63899ad0da84ce718c14936262a41cee2c79c981fc0a0e7c7beb47d5a07e8c1" "checksum num-complex 0.1.43 (registry+https://github.com/rust-lang/crates.io-index)" = "b288631d7878aaf59442cffd36910ea604ecd7745c36054328595114001c9656" diff --git a/src/rust/engine/watch/Cargo.toml b/src/rust/engine/watch/Cargo.toml index c64dccefc3e..3d7fe8ff5b5 100644 --- a/src/rust/engine/watch/Cargo.toml +++ b/src/rust/engine/watch/Cargo.toml @@ -13,11 +13,7 @@ graph = { path = "../graph" } hashing = { path = "../hashing" } log = "0.4" logging = { path = "../logging" } -# notify is currently an experimental API, we are pinning to https://docs.rs/notify/5.0.0-pre.1/notify/ -# because the latest prerelease at time of writing has removed the debounced watcher which we would like to use. -# The author suggests they will add the debounced watcher back into the stable 5.0.0 release. When that happens -# we can move to it. -notify = { git = "https://github.com/notify-rs/notify", rev = "fba00891d9105e2f581c69fbe415a58cb7966fdd" } +notify = "5.0.0-pre.2" parking_lot = "0.6" task_executor = { path = "../task_executor" } diff --git a/src/rust/engine/watch/src/lib.rs b/src/rust/engine/watch/src/lib.rs index 1ac53d59b01..e4ae9b4141a 100644 --- a/src/rust/engine/watch/src/lib.rs +++ b/src/rust/engine/watch/src/lib.rs @@ -81,8 +81,14 @@ impl InvalidationWatcher { let canonical_build_root = std::fs::canonicalize(build_root.as_path()).map_err(|e| format!("{:?}", e))?; let (watch_sender, watch_receiver) = crossbeam_channel::unbounded(); - let mut watcher: RecommendedWatcher = Watcher::new(watch_sender, Duration::from_millis(50)) - .map_err(|e| format!("Failed to begin watching the filesystem: {}", e))?; + let mut watcher: RecommendedWatcher = Watcher::new_immediate(move |ev| { + if watch_sender.send(ev).is_err() { + // The watch thread shutting down first is ok, because it can exit when the Invalidatable + // is dropped. + debug!("Watch thread has shutdown, but Watcher is still running."); + } + }) + .map_err(|e| format!("Failed to begin watching the filesystem: {}", e))?; let (liveness_sender, liveness_receiver) = crossbeam_channel::unbounded();