From e8f62217c1a93b68f0b1eea632dd1ec3128697ae Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Wed, 9 Jun 2021 17:26:29 -0700 Subject: [PATCH 1/4] Make `Watcher` object safe This changes the `Watcher` trait to be object safe, which allows users to dynamically select which backend to use. This can be helpful on systems that support multiple file watching systems that have different tradeoffs. For example, Chromium's file watcher on OS X will use an fsevent backend when watching recursive directories, and a kqueues backend when not. In order to implement this, this makes a few changes to Watcher: * removes `new_immediate` constructor. * removes the `>` from the `watch` and `unwatch`. This then replaces (and renames) calls with `notify::recommended_watcher()` to get similar behavior as the old constructor. --- CHANGELOG.md | 1 + examples/async_monitor.rs | 4 +-- examples/hot_reload_tide/src/main.rs | 4 +-- examples/monitor_raw.rs | 4 +-- src/fsevent.rs | 48 ++++++++++++++-------------- src/inotify.rs | 23 ++++++++----- src/lib.rs | 48 ++++++++++++++++------------ src/null.rs | 10 ++---- src/poll.rs | 30 +++++++++-------- src/windows.rs | 16 +++++----- tests/race-with-remove-dir.rs | 6 ++-- 11 files changed, 104 insertions(+), 90 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f303456d..cf38cea6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ ## unreleased +- CHANGE: Make `Watcher` object safe - CHANGE: Change EventFn to take FnMut [#333] [#333]: https://github.com/notify-rs/notify/pull/333 diff --git a/examples/async_monitor.rs b/examples/async_monitor.rs index 5dccb68a..a0a9ab87 100644 --- a/examples/async_monitor.rs +++ b/examples/async_monitor.rs @@ -7,7 +7,7 @@ fn async_watcher() -> notify::Result<(RecommendedWatcher, Receiver>(path: P) -> notify::Result<()> { // Add a path to be watched. All files and directories at that path and // below will be monitored for changes. - watcher.watch(path, RecursiveMode::Recursive)?; + watcher.watch(path.as_ref(), RecursiveMode::Recursive)?; while let Some(res) = rx.next().await { match res { diff --git a/examples/hot_reload_tide/src/main.rs b/examples/hot_reload_tide/src/main.rs index 0f2899b6..7d1cf806 100644 --- a/examples/hot_reload_tide/src/main.rs +++ b/examples/hot_reload_tide/src/main.rs @@ -28,11 +28,11 @@ async fn main() -> tide::Result<()> { // We listen to file changes by giving Notify // a function that will get called when events happen - let mut watcher: RecommendedWatcher = + let mut watcher = // To make sure that the config lives as long as the function // we need to move the ownership of the config inside the function // To learn more about move please read [Using move Closures with Threads](https://doc.rust-lang.org/book/ch16-01-threads.html?highlight=move#using-move-closures-with-threads) - Watcher::new_immediate(move |result: Result| { + RecommendedWatcher::new(move |result: Result| { let event = result.unwrap(); if event.kind == EventKind::Modify(ModifyKind::Any) { diff --git a/examples/monitor_raw.rs b/examples/monitor_raw.rs index 0906eadd..39b35845 100644 --- a/examples/monitor_raw.rs +++ b/examples/monitor_raw.rs @@ -6,11 +6,11 @@ fn watch>(path: P) -> notify::Result<()> { // Automatically select the best implementation for your platform. // You can also access each implementation directly e.g. INotifyWatcher. - let mut watcher: RecommendedWatcher = Watcher::new_immediate(move |res| tx.send(res).unwrap())?; + let mut watcher = RecommendedWatcher::new(move |res| tx.send(res).unwrap())?; // Add a path to be watched. All files and directories at that path and // below will be monitored for changes. - watcher.watch(path, RecursiveMode::Recursive)?; + watcher.watch(path.as_ref(), RecursiveMode::Recursive)?; for res in rx { match res { diff --git a/src/fsevent.rs b/src/fsevent.rs index 72ac79a0..be3588de 100644 --- a/src/fsevent.rs +++ b/src/fsevent.rs @@ -20,7 +20,6 @@ use crossbeam_channel::{unbounded, Receiver, Sender}; use fsevent_sys as fs; use fsevent_sys::core_foundation as cf; use std::collections::HashMap; -use std::convert::AsRef; use std::ffi::CStr; use std::os::raw; use std::path::{Path, PathBuf}; @@ -250,6 +249,11 @@ extern "C" { } impl FsEventWatcher { + /// Create a new watcher. + pub fn new(event_fn: F) -> Result { + Self::from_event_fn(Arc::new(Mutex::new(event_fn))) + } + fn from_event_fn(event_fn: Arc>) -> Result { Ok(FsEventWatcher { paths: unsafe { @@ -309,14 +313,14 @@ impl FsEventWatcher { } } - fn remove_path>(&mut self, path: P) -> Result<()> { - let str_path = path.as_ref().to_str().unwrap(); + fn remove_path(&mut self, path: &Path) -> Result<()> { + let str_path = path.to_str().unwrap(); unsafe { let mut err: cf::CFErrorRef = ptr::null_mut(); let cf_path = cf::str_path_to_cfstring_ref(str_path, &mut err); if cf_path.is_null() { cf::CFRelease(err as cf::CFRef); - return Err(Error::watch_not_found().add_path(path.as_ref().into())); + return Err(Error::watch_not_found().add_path(path.into())); } let mut to_remove = Vec::new(); @@ -335,10 +339,10 @@ impl FsEventWatcher { cf::CFArrayRemoveValueAtIndex(self.paths, *idx); } } - let p = if let Ok(canonicalized_path) = path.as_ref().canonicalize() { + let p = if let Ok(canonicalized_path) = path.canonicalize() { canonicalized_path } else { - path.as_ref().to_owned() + path.to_owned() }; match self.recursive_info.remove(&p) { Some(_) => Ok(()), @@ -347,15 +351,15 @@ impl FsEventWatcher { } // https://github.com/thibaudgg/rb-fsevent/blob/master/ext/fsevent_watch/main.c - fn append_path>( + fn append_path( &mut self, - path: P, + path: &Path, recursive_mode: RecursiveMode, ) -> Result<()> { - if !path.as_ref().exists() { - return Err(Error::path_not_found().add_path(path.as_ref().into())); + if !path.exists() { + return Err(Error::path_not_found().add_path(path.into())); } - let str_path = path.as_ref().to_str().unwrap(); + let str_path = path.to_str().unwrap(); unsafe { let mut err: cf::CFErrorRef = ptr::null_mut(); let cf_path = cf::str_path_to_cfstring_ref(str_path, &mut err); @@ -363,13 +367,13 @@ impl FsEventWatcher { // Most likely the directory was deleted, or permissions changed, // while the above code was running. cf::CFRelease(err as cf::CFRef); - return Err(Error::path_not_found().add_path(path.as_ref().into())); + return Err(Error::path_not_found().add_path(path.into())); } cf::CFArrayAppendValue(self.paths, cf_path); cf::CFRelease(cf_path); } self.recursive_info.insert( - path.as_ref().to_path_buf().canonicalize().unwrap(), + path.to_path_buf().canonicalize().unwrap(), recursive_mode.is_recursive(), ); Ok(()) @@ -539,16 +543,12 @@ unsafe fn callback_impl( } impl Watcher for FsEventWatcher { - fn new_immediate(event_fn: F) -> Result { - FsEventWatcher::from_event_fn(Arc::new(Mutex::new(event_fn))) - } - - fn watch>(&mut self, path: P, recursive_mode: RecursiveMode) -> Result<()> { - self.watch_inner(path.as_ref(), recursive_mode) + fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()> { + self.watch_inner(path, recursive_mode) } - fn unwatch>(&mut self, path: P) -> Result<()> { - self.unwatch_inner(path.as_ref()) + fn unwatch(&mut self, path: &Path) -> Result<()> { + self.unwatch_inner(path) } fn configure(&mut self, config: Config) -> Result { @@ -578,7 +578,7 @@ fn test_fsevent_watcher_drop() { let event_fn = move |res| tx.send(res).unwrap(); { - let mut watcher: RecommendedWatcher = Watcher::new_immediate(event_fn).unwrap(); + let mut watcher = FsEventWatcher::new(event_fn).unwrap(); watcher.watch(dir.path(), RecursiveMode::Recursive).unwrap(); thread::sleep(Duration::from_millis(2000)); println!("is running -> {}", watcher.is_running()); @@ -599,7 +599,7 @@ fn test_fsevent_watcher_drop() { } #[test] -fn test_steam_context_info_send() { - fn check_send() {} +fn test_steam_context_info_send_and_sync() { + fn check_send() {} check_send::(); } diff --git a/src/inotify.rs b/src/inotify.rs index 01e36842..3483ffd6 100644 --- a/src/inotify.rs +++ b/src/inotify.rs @@ -554,6 +554,11 @@ fn filter_dir(e: walkdir::Result) -> Option(event_fn: F) -> Result { + Self::from_event_fn(Box::new(event_fn))) + } + fn from_event_fn(event_fn: Box) -> Result { let inotify = Inotify::init()?; let event_loop = EventLoop::new(inotify, event_fn)?; @@ -597,16 +602,12 @@ impl INotifyWatcher { } impl Watcher for INotifyWatcher { - fn new_immediate(event_fn: F) -> Result { - INotifyWatcher::from_event_fn(Box::new(event_fn)) - } - - fn watch>(&mut self, path: P, recursive_mode: RecursiveMode) -> Result<()> { - self.watch_inner(path.as_ref(), recursive_mode) + fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()> { + self.watch_inner(path, recursive_mode) } - fn unwatch>(&mut self, path: P) -> Result<()> { - self.unwatch_inner(path.as_ref()) + fn unwatch(&mut self, path: &Path) -> Result<()> { + self.unwatch_inner(path) } fn configure(&mut self, config: Config) -> Result { @@ -624,3 +625,9 @@ impl Drop for INotifyWatcher { self.waker.wake().unwrap(); } } + +#[test] +fn inotify_watcher_is_send_and_sync() { + fn check() {} + check::(); +} diff --git a/src/lib.rs b/src/lib.rs index 41ca40a2..1a700382 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -20,11 +20,12 @@ //! # Examples //! //! ``` +//! # use std::path::Path; //! use notify::{Watcher, RecommendedWatcher, RecursiveMode, Result}; //! //! fn main() -> Result<()> { //! // Automatically select the best implementation for your platform. -//! let mut watcher: RecommendedWatcher = Watcher::new_immediate(|res| { +//! let mut watcher = notify::recommended_watcher(|res| { //! match res { //! Ok(event) => println!("event: {:?}", event), //! Err(e) => println!("watch error: {:?}", e), @@ -33,7 +34,7 @@ //! //! // Add a path to be watched. All files and directories at that path and //! // below will be monitored for changes. -//! watcher.watch(".", RecursiveMode::Recursive)?; +//! watcher.watch(Path::new("."), RecursiveMode::Recursive)?; //! //! Ok(()) //! } @@ -48,10 +49,11 @@ //! //! ``` //! # use notify::{Watcher, RecommendedWatcher, RecursiveMode, Result}; +//! # use std::path::Path; //! # use std::time::Duration; //! # fn main() -> Result<()> { //! # // Automatically select the best implementation for your platform. -//! # let mut watcher: RecommendedWatcher = Watcher::new_immediate(|res| { +//! # let mut watcher = RecommendedWatcher::new(|res| { //! # match res { //! # Ok(event) => println!("event: {:?}", event), //! # Err(e) => println!("watch error: {:?}", e), @@ -60,7 +62,7 @@ //! //! # // Add a path to be watched. All files and directories at that path and //! # // below will be monitored for changes. -//! # watcher.watch(".", RecursiveMode::Recursive)?; +//! # watcher.watch(Path::new("."), RecursiveMode::Recursive)?; //! //! use notify::Config; //! watcher.configure(Config::PreciseEvents(true))?; @@ -77,6 +79,7 @@ //! //! ``` //! # use notify::{RecommendedWatcher, RecursiveMode, Result, Watcher}; +//! # use std::path::Path; //! # //! # fn main() -> Result<()> { //! fn event_fn(res: Result) { @@ -86,10 +89,10 @@ //! } //! } //! -//! let mut watcher1: RecommendedWatcher = Watcher::new_immediate(event_fn)?; -//! let mut watcher2: RecommendedWatcher = Watcher::new_immediate(event_fn)?; -//! # watcher1.watch(".", RecursiveMode::Recursive)?; -//! # watcher2.watch(".", RecursiveMode::Recursive)?; +//! let mut watcher1 = notify::recommended_watcher(event_fn)?; +//! let mut watcher2 = notify::recommended_watcher(event_fn)?; +//! # watcher1.watch(Path::new("."), RecursiveMode::Recursive)?; +//! # watcher2.watch(Path::new("."), RecursiveMode::Recursive)?; //! # //! # Ok(()) //! # } @@ -100,7 +103,6 @@ pub use config::{Config, RecursiveMode}; pub use error::{Error, ErrorKind, Result}; pub use event::{Event, EventKind}; -use std::convert::AsRef; use std::path::Path; #[cfg(target_os = "macos")] @@ -136,14 +138,7 @@ impl EventFn for F where F: 'static + FnMut(Result) + Send {} /// Watcher is implemented per platform using the best implementation available on that platform. /// In addition to such event driven implementations, a polling implementation is also provided /// that should work on any platform. -pub trait Watcher: Sized { - /// Create a new watcher in _immediate_ mode. - /// - /// Events will be sent using the provided `tx` immediately after they occur. - fn new_immediate(event_fn: F) -> Result - where - F: EventFn; - +pub trait Watcher { /// Begin watching a new path. /// /// If the `path` is a directory, `recursive_mode` will be evaluated. If `recursive_mode` is @@ -159,7 +154,7 @@ pub trait Watcher: Sized { /// /// [#165]: https://github.com/notify-rs/notify/issues/165 /// [#166]: https://github.com/notify-rs/notify/issues/166 - fn watch>(&mut self, path: P, recursive_mode: RecursiveMode) -> Result<()>; + fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()>; /// Stop watching a path. /// @@ -167,7 +162,7 @@ pub trait Watcher: Sized { /// /// Returns an error in the case that `path` has not been watched or if removing the watch /// fails. - fn unwatch>(&mut self, path: P) -> Result<()>; + fn unwatch(&mut self, path: &Path) -> Result<()>; /// Configure the watcher at runtime. /// @@ -200,9 +195,20 @@ pub type RecommendedWatcher = PollWatcher; /// _immediate_ mode. /// /// See [`Watcher::new_immediate`](trait.Watcher.html#tymethod.new_immediate). -pub fn immediate_watcher(event_fn: F) -> Result +pub fn recommended_watcher(event_fn: F) -> Result where F: EventFn, { - Watcher::new_immediate(event_fn) + // All recommended watchers currently implement `new`, so just call that. + RecommendedWatcher::new(event_fn) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_object_safe() { + let _watcher: &dyn Watcher = &NullWatcher; + } } diff --git a/src/null.rs b/src/null.rs index 5bfdf3bb..0911740c 100644 --- a/src/null.rs +++ b/src/null.rs @@ -2,7 +2,7 @@ #![allow(unused_variables)] -use super::{EventFn, RecursiveMode, Result, Watcher}; +use super::{RecursiveMode, Result, Watcher}; use std::path::Path; /// Stub `Watcher` implementation @@ -11,15 +11,11 @@ use std::path::Path; pub struct NullWatcher; impl Watcher for NullWatcher { - fn new_immediate(_event_fn: F) -> Result { - Ok(NullWatcher) - } - - fn watch>(&mut self, path: P, recursive_mode: RecursiveMode) -> Result<()> { + fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()> { Ok(()) } - fn unwatch>(&mut self, path: P) -> Result<()> { + fn unwatch(&mut self, path: &Path) -> Result<()> { Ok(()) } } diff --git a/src/poll.rs b/src/poll.rs index 750edb77..ab51f166 100644 --- a/src/poll.rs +++ b/src/poll.rs @@ -43,7 +43,14 @@ fn emit_event(event_fn: &Mutex, res: Result) { } impl PollWatcher { - /// Create a PollWatcher which polls every `delay` milliseconds + /// Create a new [PollWatcher]. + pub fn new(event_fn: F) -> Result { + let event_fn = Arc::new(Mutex::new(event_fn)); + let delay = Duration::from_secs(30); + Self::with_delay(event_fn, delay) + } + + /// Create a [PollWatcher] which polls every `delay` milliseconds pub fn with_delay(event_fn: Arc>, delay: Duration) -> Result { let mut p = PollWatcher { event_fn, @@ -272,18 +279,12 @@ impl PollWatcher { } impl Watcher for PollWatcher { - fn new_immediate(event_fn: F) -> Result { - let event_fn = Arc::new(Mutex::new(event_fn)); - let delay = Duration::from_secs(30); - PollWatcher::with_delay(event_fn, delay) - } - - fn watch>(&mut self, path: P, recursive_mode: RecursiveMode) -> Result<()> { - self.watch_inner(path.as_ref(), recursive_mode) + fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()> { + self.watch_inner(path, recursive_mode) } - fn unwatch>(&mut self, path: P) -> Result<()> { - self.unwatch_inner(path.as_ref()) + fn unwatch(&mut self, path: &Path) -> Result<()> { + self.unwatch_inner(path) } } @@ -293,5 +294,8 @@ impl Drop for PollWatcher { } } -// Because all public methods are `&mut self` it's also perfectly safe to share references. -unsafe impl Sync for PollWatcher {} +#[test] +fn poll_watcher_is_send_and_sync() { + fn check() {} + check::(); +} diff --git a/src/windows.rs b/src/windows.rs index ffe77045..17fab480 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -403,6 +403,14 @@ pub struct ReadDirectoryChangesWatcher { } impl ReadDirectoryChangesWatcher { + pub fn new(event_fn: F) -> Result { + // create dummy channel for meta event + // TODO: determine the original purpose of this - can we remove it? + let (meta_tx, _) = unbounded(); + let event_fn = Arc::new(Mutex::new(event_fn)); + Self::create(event_fn, meta_tx) + } + pub fn create( event_fn: Arc>, meta_tx: Sender, @@ -491,14 +499,6 @@ impl ReadDirectoryChangesWatcher { } impl Watcher for ReadDirectoryChangesWatcher { - fn new_immediate(event_fn: F) -> Result { - // create dummy channel for meta event - // TODO: determine the original purpose of this - can we remove it? - let (meta_tx, _) = unbounded(); - let event_fn = Arc::new(Mutex::new(event_fn)); - ReadDirectoryChangesWatcher::create(event_fn, meta_tx) - } - fn watch>(&mut self, path: P, recursive_mode: RecursiveMode) -> Result<()> { self.watch_inner(path.as_ref(), recursive_mode) } diff --git a/tests/race-with-remove-dir.rs b/tests/race-with-remove-dir.rs index fd753a5b..a09f53ff 100644 --- a/tests/race-with-remove-dir.rs +++ b/tests/race-with-remove-dir.rs @@ -1,6 +1,6 @@ use std::{fs, thread, time::Duration}; -use notify::{immediate_watcher, RecursiveMode, Watcher}; +use notify::{RecursiveMode, Watcher}; /// Test for . /// Note: This test will fail if your temp directory is not writable. @@ -11,12 +11,12 @@ fn test_race_with_remove_dir() { { let tmpdir = tmpdir.path().to_path_buf(); thread::spawn(move || { - let mut watcher = immediate_watcher(move |result| { + let mut watcher = notify::recommended_watcher(move |result| { eprintln!("received event: {:?}", result); }) .unwrap(); - watcher.watch(tmpdir, RecursiveMode::NonRecursive).unwrap(); + watcher.watch(&tmpdir, RecursiveMode::NonRecursive).unwrap(); }); } From c92b1a62755fd4422a8d93276f7fc6ad2c539eed Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Wed, 9 Jun 2021 18:02:00 -0700 Subject: [PATCH 2/4] Add changelog line --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf38cea6..3757c90a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,10 +19,11 @@ ## unreleased -- CHANGE: Make `Watcher` object safe +- CHANGE: Make `Watcher` object safe [#336] - CHANGE: Change EventFn to take FnMut [#333] [#333]: https://github.com/notify-rs/notify/pull/333 +[#336]: https://github.com/notify-rs/notify/pull/336 ## 5.0.0-pre.10 (2021-06-04) From e9e932086eacdc0a5a1221d7ac91f0062d1979ea Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Thu, 10 Jun 2021 10:50:20 -0700 Subject: [PATCH 3/4] Fix compiling on linux and windows --- src/inotify.rs | 2 +- src/windows.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/inotify.rs b/src/inotify.rs index 3483ffd6..8891d679 100644 --- a/src/inotify.rs +++ b/src/inotify.rs @@ -556,7 +556,7 @@ fn filter_dir(e: walkdir::Result) -> Option(event_fn: F) -> Result { - Self::from_event_fn(Box::new(event_fn))) + Self::from_event_fn(Box::new(event_fn)) } fn from_event_fn(event_fn: Box) -> Result { diff --git a/src/windows.rs b/src/windows.rs index 17fab480..c3f334ee 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -499,12 +499,12 @@ impl ReadDirectoryChangesWatcher { } impl Watcher for ReadDirectoryChangesWatcher { - fn watch>(&mut self, path: P, recursive_mode: RecursiveMode) -> Result<()> { - self.watch_inner(path.as_ref(), recursive_mode) + fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()> { + self.watch_inner(path, recursive_mode) } - fn unwatch>(&mut self, path: P) -> Result<()> { - self.unwatch_inner(path.as_ref()) + fn unwatch(&mut self, path: &Path) -> Result<()> { + self.unwatch_inner(path) } fn configure(&mut self, config: Config) -> Result { From 6edb86e4a038d5c6f767616054d7908247744574 Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Thu, 10 Jun 2021 10:57:32 -0700 Subject: [PATCH 4/4] Fix compiling tide example --- examples/hot_reload_tide/src/main.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/examples/hot_reload_tide/src/main.rs b/examples/hot_reload_tide/src/main.rs index 7d1cf806..71f9acfd 100644 --- a/examples/hot_reload_tide/src/main.rs +++ b/examples/hot_reload_tide/src/main.rs @@ -8,6 +8,7 @@ use notify::{ event::ModifyKind, Error, Event, EventKind, RecommendedWatcher, RecursiveMode, Watcher, }; +use std::path::Path; use std::sync::{Arc, Mutex}; use tide::{Body, Response}; @@ -43,7 +44,7 @@ async fn main() -> tide::Result<()> { } })?; - watcher.watch(CONFIG_PATH, RecursiveMode::Recursive)?; + watcher.watch(Path::new(CONFIG_PATH), RecursiveMode::Recursive)?; // We set up a web server using [Tide](https://github.com/http-rs/tide) let mut app = tide::with_state(config);