From 8f809f71974bd895dbf6ed3f014c5320e6d33855 Mon Sep 17 00:00:00 2001 From: Daniel Faust Date: Sat, 14 Sep 2024 15:00:36 +0200 Subject: [PATCH 1/4] Fix ordering of debounced events Fixes #636. --- CHANGELOG.md | 6 +++ notify-debouncer-full/src/lib.rs | 49 ++++++++++++++----- .../sort_events_chronologically.hjson | 42 ++++++++++++++++ .../sort_events_with_reordering.hjson | 42 ++++++++++++++++ 4 files changed, 127 insertions(+), 12 deletions(-) create mode 100644 notify-debouncer-full/test_cases/sort_events_chronologically.hjson create mode 100644 notify-debouncer-full/test_cases/sort_events_with_reordering.hjson diff --git a/CHANGELOG.md b/CHANGELOG.md index 70962e50..fe14f503 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,12 @@ v5 maintenance branch is on `v5_maintenance` after `5.2.0` v4 commits split out to branch `v4_maintenance` starting with `4.0.16` +## debouncer-full 0.4.1 (unreleased) + +- FIX: ordering of debounced events could lead to a panic with Rust 1.81.0 and above [#636] + +[#636]: https://github.com/notify-rs/notify/issues/636 + ## 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 7d2595d5..ee6a57f5 100644 --- a/notify-debouncer-full/src/lib.rs +++ b/notify-debouncer-full/src/lib.rs @@ -249,17 +249,7 @@ impl DebounceDataInner { self.queues = queues_remaining; - // 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) - } - }); - - events_expired + sort_events(events_expired) } /// Returns all currently stored errors @@ -654,6 +644,39 @@ pub fn new_debouncer( ) } +fn sort_events(events: Vec) -> Vec { + let mut sorted = Vec::with_capacity(events.len()); + + // group events by path + let mut events_by_path: HashMap<_, VecDeque<_>> = + events.into_iter().fold(HashMap::new(), |mut acc, event| { + acc.entry(event.paths.last().cloned().unwrap_or_default()) + .or_default() + .push_back(event); + acc + }); + + // push events for different paths in chronological order and keep the order of events with the same path + while !events_by_path.is_empty() { + let min_time = events_by_path + .values() + .map(|events| events[0].time) + .min() + .unwrap(); + + for events in events_by_path.values_mut() { + while events.front().is_some_and(|event| event.time <= min_time) { + let event = events.pop_front().unwrap(); + sorted.push(event); + } + } + + events_by_path.retain(|_, events| !events.is_empty()); + } + + sorted +} + #[cfg(test)] mod tests { use std::{fs, path::Path}; @@ -702,7 +725,9 @@ mod tests { "emit_close_events_only_once", "emit_modify_event_after_close_event", "emit_needs_rescan_event", - "read_file_id_without_create_event" + "read_file_id_without_create_event", + "sort_events_chronologically", + "sort_events_with_reordering" )] file_name: &str, ) { diff --git a/notify-debouncer-full/test_cases/sort_events_chronologically.hjson b/notify-debouncer-full/test_cases/sort_events_chronologically.hjson new file mode 100644 index 00000000..363b22e3 --- /dev/null +++ b/notify-debouncer-full/test_cases/sort_events_chronologically.hjson @@ -0,0 +1,42 @@ +{ + state: { + queues: { + /watch/file-1: { + events: [ + { kind: "create-any", paths: ["*"], time: 2 } + { kind: "modify-any", paths: ["*"], time: 3 } + ] + } + /watch/file-2: { + events: [ + { kind: "create-any", paths: ["*"], time: 1 } + { kind: "modify-any", paths: ["*"], time: 4 } + ] + } + } + } + expected: { + queues: { + /watch/file-1: { + events: [ + { kind: "create-any", paths: ["*"], time: 2 } + { kind: "modify-any", paths: ["*"], time: 3 } + ] + } + /watch/file-2: { + events: [ + { kind: "create-any", paths: ["*"], time: 1 } + { kind: "modify-any", paths: ["*"], time: 4 } + ] + } + } + events: { + long: [ + { kind: "create-any", paths: ["/watch/file-2"], time: 1 } + { kind: "create-any", paths: ["/watch/file-1"], time: 2 } + { kind: "modify-any", paths: ["/watch/file-1"], time: 3 } + { kind: "modify-any", paths: ["/watch/file-2"], time: 4 } + ] + } + } +} diff --git a/notify-debouncer-full/test_cases/sort_events_with_reordering.hjson b/notify-debouncer-full/test_cases/sort_events_with_reordering.hjson new file mode 100644 index 00000000..030bab1b --- /dev/null +++ b/notify-debouncer-full/test_cases/sort_events_with_reordering.hjson @@ -0,0 +1,42 @@ +{ + state: { + queues: { + /watch/file-1: { + events: [ + { kind: "create-any", paths: ["*"], time: 2 } + { kind: "modify-any", paths: ["*"], time: 3 } + ] + } + /watch/file-2: { + events: [ + { kind: "rename-to", paths: ["*"], time: 4 } + { kind: "modify-any", paths: ["*"], time: 1 } + ] + } + } + } + expected: { + queues: { + /watch/file-1: { + events: [ + { kind: "create-any", paths: ["*"], time: 2 } + { kind: "modify-any", paths: ["*"], time: 3 } + ] + } + /watch/file-2: { + events: [ + { kind: "rename-to", paths: ["*"], time: 4 } + { kind: "modify-any", paths: ["*"], time: 1 } + ] + } + } + events: { + long: [ + { kind: "create-any", paths: ["/watch/file-1"], time: 2 } + { kind: "modify-any", paths: ["/watch/file-1"], time: 3 } + { kind: "rename-to", paths: ["/watch/file-2"], time: 4 } + { kind: "modify-any", paths: ["/watch/file-2"], time: 1 } + ] + } + } +} From f5c47023a6cd331715f8b43c904d1fb51ba94d80 Mon Sep 17 00:00:00 2001 From: Daniel Faust Date: Sat, 14 Sep 2024 21:23:47 +0200 Subject: [PATCH 2/4] Improve `sort_events` performance --- notify-debouncer-full/src/lib.rs | 43 ++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/notify-debouncer-full/src/lib.rs b/notify-debouncer-full/src/lib.rs index ee6a57f5..5ed27a64 100644 --- a/notify-debouncer-full/src/lib.rs +++ b/notify-debouncer-full/src/lib.rs @@ -57,9 +57,9 @@ //! - `crossbeam` enabled by default, adds [`DebounceEventHandler`](DebounceEventHandler) support for crossbeam channels. //! Also enables crossbeam-channel in the re-exported notify. You may want to disable this when using the tokio async runtime. //! - `serde` enables serde support for events. -//! +//! //! # Caveats -//! +//! //! As all file events are sourced from notify, the [known problems](https://docs.rs/notify/latest/notify/#known-problems) section applies here too. mod cache; @@ -69,7 +69,8 @@ mod debounced_event; mod testing; use std::{ - collections::{HashMap, VecDeque}, + cmp::Reverse, + collections::{BinaryHeap, HashMap, VecDeque}, path::PathBuf, sync::{ atomic::{AtomicBool, Ordering}, @@ -657,21 +658,31 @@ fn sort_events(events: Vec) -> Vec { }); // push events for different paths in chronological order and keep the order of events with the same path - while !events_by_path.is_empty() { - let min_time = events_by_path - .values() - .map(|events| events[0].time) - .min() - .unwrap(); - - for events in events_by_path.values_mut() { - while events.front().is_some_and(|event| event.time <= min_time) { - let event = events.pop_front().unwrap(); - sorted.push(event); - } + + let mut min_time_heap = events_by_path + .iter() + .map(|(path, events)| Reverse((events[0].time, path.clone()))) + .collect::>(); + + while let Some(Reverse((min_time, path))) = min_time_heap.pop() { + // unwrap is safe because only paths from `events_by_path` are added to `min_time_heap` + // and they are never removed from `events_by_path`. + let events = events_by_path.get_mut(&path).unwrap(); + + let mut push_next = false; + + while events.front().is_some_and(|event| event.time <= min_time) { + // unwrap is safe beause `pop_front` mus return some in order to enter the loop + let event = events.pop_front().unwrap(); + sorted.push(event); + push_next = true; } - events_by_path.retain(|_, events| !events.is_empty()); + if push_next { + if let Some(event) = events.front() { + min_time_heap.push(Reverse((event.time, path))); + } + } } sorted From 3606af8005f01da1bf018807a05dbbbec68d1823 Mon Sep 17 00:00:00 2001 From: Daniel Faust Date: Sun, 29 Sep 2024 20:19:45 +0200 Subject: [PATCH 3/4] Fix compatibility with MSRV 1.60 --- notify-debouncer-full/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notify-debouncer-full/src/lib.rs b/notify-debouncer-full/src/lib.rs index 5ed27a64..4bcc65b0 100644 --- a/notify-debouncer-full/src/lib.rs +++ b/notify-debouncer-full/src/lib.rs @@ -671,7 +671,7 @@ fn sort_events(events: Vec) -> Vec { let mut push_next = false; - while events.front().is_some_and(|event| event.time <= min_time) { + while events.front().map_or(false, |event| event.time <= min_time) { // unwrap is safe beause `pop_front` mus return some in order to enter the loop let event = events.pop_front().unwrap(); sorted.push(event); From ef8bc72b4bd0d8dd6f6b869f593277f9f9eac400 Mon Sep 17 00:00:00 2001 From: Daniel Faust Date: Sun, 29 Sep 2024 20:21:26 +0200 Subject: [PATCH 4/4] Fix debouncer-full version number and prepare release --- CHANGELOG.md | 2 +- notify-debouncer-full/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe14f503..87f9848f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ v5 maintenance branch is on `v5_maintenance` after `5.2.0` v4 commits split out to branch `v4_maintenance` starting with `4.0.16` -## debouncer-full 0.4.1 (unreleased) +## debouncer-full 0.3.2 (2024-09-29) - FIX: ordering of debounced events could lead to a panic with Rust 1.81.0 and above [#636] diff --git a/notify-debouncer-full/Cargo.toml b/notify-debouncer-full/Cargo.toml index 2fd08a09..61cf5c63 100644 --- a/notify-debouncer-full/Cargo.toml +++ b/notify-debouncer-full/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "notify-debouncer-full" -version = "0.3.1" +version = "0.3.2" edition = "2021" rust-version = "1.60" description = "notify event debouncer optimized for ease of use"