diff --git a/CHANGELOG.md b/CHANGELOG.md index e879dba6..7b785823 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,8 @@ New crate containing public type definitions for the notify and debouncer crates - CHANGE: add `RecommendedCache`, which automatically enables the file ID cache on Windows and MacOS and disables it on Linux, where it is not needed +- FIX: ordering of debounced events when multiple files are modified and renamed (eg. during a safe save performed by Blender) + ## debouncer-full 0.3.1 (2023-08-21) - CHANGE: remove serde binary experiment opt-out after it got removed [#530] diff --git a/notify-debouncer-full/src/lib.rs b/notify-debouncer-full/src/lib.rs index 0996fd66..f13480f1 100644 --- a/notify-debouncer-full/src/lib.rs +++ b/notify-debouncer-full/src/lib.rs @@ -153,7 +153,7 @@ struct Queue { impl Queue { fn was_created(&self) -> bool { - self.events.front().map_or(false, |event| { + self.events.front().is_some_and(|event| { matches!( event.kind, EventKind::Create(_) | EventKind::Modify(ModifyKind::Name(RenameMode::To)) @@ -162,7 +162,7 @@ impl Queue { } fn was_removed(&self) -> bool { - self.events.front().map_or(false, |event| { + self.events.front().is_some_and(|event| { matches!( event.kind, EventKind::Remove(_) | EventKind::Modify(ModifyKind::Name(RenameMode::From)) @@ -171,9 +171,48 @@ impl Queue { } } +#[derive(Debug)] +pub struct BlockEntry { + pub blocker_path: PathBuf, + pub blocker_time: Instant, + pub blockee_path: PathBuf, +} + +#[derive(Debug, Default)] +pub struct BlockManager { + entries: Vec, +} + +impl BlockManager { + pub fn new() -> BlockManager { + BlockManager { + entries: Vec::new(), + } + } + + pub fn add_blocker(&mut self, entry: BlockEntry) { + self.entries.push(entry); + } + + pub fn remove_blocker(&mut self, path: &PathBuf, time: Instant) { + self.entries + .retain(|entry| entry.blocker_path != *path || entry.blocker_time != time); + } + + pub fn is_blocked_by(&self, path: &PathBuf) -> Option<(&PathBuf, Instant)> { + for entry in &self.entries { + if entry.blockee_path == *path { + return Some((&entry.blocker_path, entry.blocker_time)); + } + } + None + } +} + #[derive(Debug)] pub(crate) struct DebounceDataInner { queues: HashMap, + blocking: BlockManager, roots: Vec<(PathBuf, RecursiveMode)>, cache: T, rename_event: Option<(DebouncedEvent, Option)>, @@ -186,6 +225,7 @@ impl DebounceDataInner { pub(crate) fn new(cache: T, timeout: Duration) -> Self { Self { queues: HashMap::new(), + blocking: BlockManager::new(), roots: Vec::new(), cache, rename_event: None, @@ -195,11 +235,17 @@ impl DebounceDataInner { } } + fn contains_event(&self, path: &PathBuf, time: Instant) -> bool { + self.queues + .get(path) + .is_some_and(|queue| queue.events.iter().any(|event| event.time == time)) + } + /// Retrieve a vec of debounced events, removing them if not continuous pub fn debounced_events(&mut self) -> Vec { let now = Instant::now(); - let mut events_expired = Vec::with_capacity(self.queues.len()); - let mut queues_remaining = HashMap::with_capacity(self.queues.len()); + let events_count = self.queues.values().map(|queue| queue.events.len()).sum(); + let mut events_expired = Vec::with_capacity(events_count); if let Some(event) = self.rescan_event.take() { if now.saturating_duration_since(event.time) >= self.timeout { @@ -210,48 +256,62 @@ impl DebounceDataInner { } } - // TODO: perfect fit for drain_filter https://github.com/rust-lang/rust/issues/59618 - for (path, mut queue) in self.queues.drain() { - let mut kind_index = HashMap::new(); - - while let Some(event) = queue.events.pop_front() { - if now.saturating_duration_since(event.time) >= self.timeout { - // remove previous event of the same kind - if let Some(idx) = kind_index.get(&event.kind).copied() { - events_expired.remove(idx); - - kind_index.values_mut().for_each(|i| { - if *i > idx { - *i -= 1 - } - }) - } + let mut kind_index: HashMap> = HashMap::new(); - kind_index.insert(event.kind, events_expired.len()); + while let Some(path) = self + .queues + // iterate over all queues + .iter() + // get the first event of every queue + .filter_map(|(path, queue)| queue.events.front().map(|event| (path, event.time))) + // filter out all blocked events + .filter(|(path, _)| { + self.blocking + .is_blocked_by(path) + .map_or(true, |(path, time)| !self.contains_event(path, time)) + }) + // get the event with the earliest timestamp + .min_by_key(|(_, time)| *time) + // get the path of the event + .map(|(path, _)| path.clone()) + { + let event = self + .queues + .get_mut(&path) + .unwrap() + .events + .pop_front() + .unwrap(); - events_expired.push(event); - } else { - queue.events.push_front(event); - break; + if now.saturating_duration_since(event.time) >= self.timeout { + // remove previous event of the same kind + let kind_index = kind_index.entry(path.clone()).or_default(); + if let Some(idx) = kind_index.get(&event.kind).copied() { + events_expired.remove(idx); + + kind_index.values_mut().for_each(|i| { + if *i > idx { + *i -= 1 + } + }) } - } + kind_index.insert(event.kind, events_expired.len()); - if !queue.events.is_empty() { - queues_remaining.insert(path, queue); + self.blocking.remove_blocker(&path, event.time); + + events_expired.push(event); + } else { + self.queues.get_mut(&path).unwrap().events.push_front(event); + + break; } } - self.queues = queues_remaining; + self.queues.retain(|_, queue| !queue.events.is_empty()); - // order events for different files chronologically, but keep the order of events for the same file - events_expired.sort_by(|event_a, event_b| { - // use the last path because rename events are emitted for the target path - if event_a.paths.last() == event_b.paths.last() { - std::cmp::Ordering::Equal - } else { - event_a.time.cmp(&event_b.time) - } - }); + if self.queues.is_empty() { + self.blocking.entries.clear(); + } events_expired } @@ -426,18 +486,6 @@ impl DebounceDataInner { source_queue.events.remove(remove_index); } - // split off remove or move out event and add it back to the events map - if source_queue.was_removed() { - let event = source_queue.events.pop_front().unwrap(); - - self.queues.insert( - event.paths[0].clone(), - Queue { - events: [event].into(), - }, - ); - } - // update paths for e in &mut source_queue.events { e.paths = vec![event.paths[0].clone()]; @@ -456,7 +504,12 @@ impl DebounceDataInner { } if let Some(target_queue) = self.queues.get_mut(&event.paths[0]) { - if !target_queue.was_created() { + if target_queue.was_removed() { + let event = target_queue.events.pop_front().unwrap(); + source_queue.events.push_front(event); + } + + if !target_queue.was_created() && !source_queue.was_removed() { let mut remove_event = DebouncedEvent { event: Event { kind: EventKind::Remove(RemoveKind::Any), @@ -474,6 +527,8 @@ impl DebounceDataInner { } else { self.queues.insert(event.paths[0].clone(), source_queue); } + + self.find_blocked_events(&event.paths[0]); } fn push_remove_event(&mut self, event: Event, time: Instant) { @@ -519,6 +574,25 @@ impl DebounceDataInner { ); } } + + fn find_blocked_events(&mut self, path: &Path) { + for queue in self.queues.values_mut() { + for event in &mut queue.events { + if matches!( + event.event.kind, + EventKind::Modify(ModifyKind::Name(RenameMode::Both)) + ) && event.event.paths[0] == path + { + self.blocking.add_blocker(BlockEntry { + blocker_path: event.event.paths[1].clone(), + blocker_time: event.time, + blockee_path: path.to_path_buf(), + }); + break; + } + } + } + } } /// Debouncer guard, stops the debouncer on drop. @@ -756,6 +830,11 @@ mod tests { "add_remove_parent_event_after_remove_child_event", "add_errors", "emit_continuous_modify_content_events", + "emit_create_event_after_safe_save_and_backup_override", + "emit_create_event_after_safe_save_and_backup_rotation_twice", + "emit_create_event_after_safe_save_and_backup_rotation", + "emit_create_event_after_safe_save_and_double_move", + "emit_create_event_after_safe_save_and_double_move_and_recreate", "emit_events_in_chronological_order", "emit_events_with_a_prepended_rename_event", "emit_close_events_only_once", diff --git a/notify-debouncer-full/src/testing.rs b/notify-debouncer-full/src/testing.rs index 55ae706e..1888822f 100644 --- a/notify-debouncer-full/src/testing.rs +++ b/notify-debouncer-full/src/testing.rs @@ -14,7 +14,7 @@ use notify::{ Error, ErrorKind, Event, EventKind, RecursiveMode, }; -use crate::{DebounceDataInner, DebouncedEvent, FileIdCache, Queue}; +use crate::{BlockManager, DebounceDataInner, DebouncedEvent, FileIdCache, Queue}; pub(crate) use schema::TestCase; @@ -268,6 +268,7 @@ impl schema::State { DebounceDataInner { queues, + blocking: BlockManager::new(), roots: Vec::new(), cache, rename_event, diff --git a/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_override.hjson b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_override.hjson new file mode 100644 index 00000000..9ca2e563 --- /dev/null +++ b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_override.hjson @@ -0,0 +1,33 @@ +// Based on the Blender safe save scenario. +// +// In this scenario the backup file is not removed first. +{ + state: {} + events: [ + { kind: "create-any", paths: ["/watch/file@"], time: 1 } + { kind: "rename-from", paths: ["/watch/file"], tracker: 1, time: 3 } + { kind: "rename-to", paths: ["/watch/file1"], tracker: 1, time: 4 } + { kind: "rename-from", paths: ["/watch/file@"], tracker: 2, time: 5 } + { kind: "rename-to", paths: ["/watch/file"], tracker: 2, time: 6 } + ] + expected: { + queues: { + /watch/file: { + events: [ + { kind: "create-any", paths: ["*"], time: 1 } + ] + } + /watch/file1: { + events: [ + { kind: "rename-both", paths: ["/watch/file", "/watch/file1"], tracker: 1, time: 3 } + ] + } + } + events: { + long: [ + { kind: "rename-both", paths: ["/watch/file", "/watch/file1"], tracker: 1, time: 3 } + { kind: "create-any", paths: ["/watch/file"], time: 1 } + ] + } + } +} diff --git a/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_rotation.hjson b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_rotation.hjson new file mode 100644 index 00000000..0591fd71 --- /dev/null +++ b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_rotation.hjson @@ -0,0 +1,41 @@ +// https://github.com/notify-rs/notify/issues/587 +// +// Blender causes the following events when saving a file: +// +// create test.blend@ (new content) +// delete test.blend1 (delete backup) +// rename test.blend -> test.blend1 (move current to backup) +// rename test.blend@ -> test.blend (move new to current) +{ + state: {} + events: [ + { kind: "create-any", paths: ["/watch/file@"], time: 1 } + { kind: "remove-any", paths: ["/watch/file1"], time: 2 } + { kind: "rename-from", paths: ["/watch/file"], tracker: 1, time: 3 } + { kind: "rename-to", paths: ["/watch/file1"], tracker: 1, time: 4 } + { kind: "rename-from", paths: ["/watch/file@"], tracker: 2, time: 5 } + { kind: "rename-to", paths: ["/watch/file"], tracker: 2, time: 6 } + ] + expected: { + queues: { + /watch/file: { + events: [ + { kind: "create-any", paths: ["*"], time: 1 } + ] + } + /watch/file1: { + events: [ + { kind: "remove-any", paths: ["*"], time: 2 } + { kind: "rename-both", paths: ["/watch/file", "/watch/file1"], tracker: 1, time: 3 } + ] + } + } + events: { + long: [ + { kind: "remove-any", paths: ["/watch/file1"], time: 2 } + { kind: "rename-both", paths: ["/watch/file", "/watch/file1"], tracker: 1, time: 3 } + { kind: "create-any", paths: ["/watch/file"], time: 1 } + ] + } + } +} diff --git a/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_rotation_twice.hjson b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_rotation_twice.hjson new file mode 100644 index 00000000..515f85a4 --- /dev/null +++ b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_backup_rotation_twice.hjson @@ -0,0 +1,42 @@ +// Based on the Blender safe save scenario. +// +// In this scenario a file is saved twice in very short succession. +{ + state: {} + events: [ + { kind: "create-any", paths: ["/watch/file@"], time: 1 } + { kind: "remove-any", paths: ["/watch/file1"], time: 2 } + { kind: "rename-from", paths: ["/watch/file"], tracker: 1, time: 3 } + { kind: "rename-to", paths: ["/watch/file1"], tracker: 1, time: 4 } + { kind: "rename-from", paths: ["/watch/file@"], tracker: 2, time: 5 } + { kind: "rename-to", paths: ["/watch/file"], tracker: 2, time: 6 } + { kind: "create-any", paths: ["/watch/file@"], time: 7 } + { kind: "remove-any", paths: ["/watch/file1"], time: 8 } + { kind: "rename-from", paths: ["/watch/file"], tracker: 3, time: 9 } + { kind: "rename-to", paths: ["/watch/file1"], tracker: 3, time: 10 } + { kind: "rename-from", paths: ["/watch/file@"], tracker: 4, time: 11 } + { kind: "rename-to", paths: ["/watch/file"], tracker: 4, time: 12 } + ] + expected: { + queues: { + /watch/file: { + events: [ + { kind: "create-any", paths: ["*"], time: 7 } + ] + } + /watch/file1: { + events: [ + { kind: "remove-any", paths: ["*"], time: 8 } + { kind: "create-any", paths: ["*"], time: 1 } + ] + } + } + events: { + long: [ + { kind: "create-any", paths: ["/watch/file"], time: 7 } + { kind: "remove-any", paths: ["/watch/file1"], time: 8 } + { kind: "create-any", paths: ["/watch/file1"], time: 1 } + ] + } + } +} diff --git a/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_double_move.hjson b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_double_move.hjson new file mode 100644 index 00000000..13b2db5c --- /dev/null +++ b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_double_move.hjson @@ -0,0 +1,36 @@ +// Based on the Blender safe save scenario. +// +// In this scenario the backup file is renamed twice in very short succession. +{ + state: {} + events: [ + { kind: "create-any", paths: ["/watch/file@"], time: 1 } + { kind: "rename-from", paths: ["/watch/file"], tracker: 1, time: 3 } + { kind: "rename-to", paths: ["/watch/file1"], tracker: 1, time: 4 } + { kind: "create-any", paths: ["/watch/file1"], time: 5 } + { kind: "rename-from", paths: ["/watch/file1"], tracker: 2, time: 6 } + { kind: "rename-to", paths: ["/watch/file2"], tracker: 2, time: 7 } + { kind: "rename-from", paths: ["/watch/file@"], tracker: 2, time: 8 } + { kind: "rename-to", paths: ["/watch/file"], tracker: 2, time: 9 } + ] + expected: { + queues: { + /watch/file: { + events: [ + { kind: "create-any", paths: ["*"], time: 1 } + ] + } + /watch/file2: { + events: [ + { kind: "create-any", paths: ["*"], time: 5 } + ] + } + } + events: { + long: [ + { kind: "create-any", paths: ["/watch/file"], time: 1 } + { kind: "create-any", paths: ["/watch/file2"], time: 5 } + ] + } + } +} diff --git a/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_double_move_and_recreate.hjson b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_double_move_and_recreate.hjson new file mode 100644 index 00000000..4115f1b3 --- /dev/null +++ b/notify-debouncer-full/test_cases/emit_create_event_after_safe_save_and_double_move_and_recreate.hjson @@ -0,0 +1,40 @@ +// Based on the Blender safe save scenario. +// +// In this scenario the backup file is renamed, re-created and renamed again in very short succession. +// +// The create event for `/watch/file` is blocked by the rename of `/watch/file` to `/watch/file1`. +// Then the renames from `/watch/file` to `/watch/file1` and from `/watch/file1` to `/watch/file2` +// are merged and the first rename is removed. Therefore the block must be ignored. +{ + state: {} + events: [ + { kind: "create-any", paths: ["/watch/file@"], time: 1 } + { kind: "rename-from", paths: ["/watch/file"], tracker: 1, time: 3 } + { kind: "rename-to", paths: ["/watch/file1"], tracker: 1, time: 4 } + { kind: "create-any", paths: ["/watch/file1"], time: 5 } + { kind: "rename-from", paths: ["/watch/file@"], tracker: 2, time: 6 } + { kind: "rename-to", paths: ["/watch/file"], tracker: 2, time: 7 } + { kind: "rename-from", paths: ["/watch/file1"], tracker: 2, time: 8 } + { kind: "rename-to", paths: ["/watch/file2"], tracker: 2, time: 9 } + ] + expected: { + queues: { + /watch/file: { + events: [ + { kind: "create-any", paths: ["*"], time: 1 } + ] + } + /watch/file2: { + events: [ + { kind: "create-any", paths: ["*"], time: 5 } + ] + } + } + events: { + long: [ + { kind: "create-any", paths: ["/watch/file"], time: 1 } + { kind: "create-any", paths: ["/watch/file2"], time: 5 } + ] + } + } +} diff --git a/notify-debouncer-full/test_cases/emit_needs_rescan_event.hjson b/notify-debouncer-full/test_cases/emit_needs_rescan_event.hjson index 81c6bd34..ddaae5c4 100644 --- a/notify-debouncer-full/test_cases/emit_needs_rescan_event.hjson +++ b/notify-debouncer-full/test_cases/emit_needs_rescan_event.hjson @@ -47,9 +47,9 @@ events: { short: [] long: [ + { kind: "other", flags: ["rescan"], time: 3 } { kind: "create-any", paths: ["/watch/file-a"], time: 1 } { kind: "create-any", paths: ["/watch/file-b"], time: 2 } - { kind: "other", flags: ["rescan"], time: 3 } ] } }