Skip to content

feat: rework PathsMut to be more consistent #705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: rework PathsMut to be more consistent #705

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
aaaac45
feat: rework PathsMut to be more consistent
riberk Jul 18, 2025
7530d19
fix: remove paths mut
riberk Jul 18, 2025
f3a13c2
fix: deprecate paths_mut, introduce WatchPathConfig and some renames
riberk Jul 18, 2025
79f2d0e
chore: update changelog for #705
riberk Jul 19, 2025
cc1bc73
feat: UpdatePathsError into Error conversion
riberk Jul 21, 2025
1a127c0
fix: typos
riberk Jul 21, 2025
5997e67
fix(test): macos fsevents may return non-canonicalized paths
riberk Jul 21, 2025
e1a23fd
Apply suggestions from code review
riberk Oct 19, 2025
a357ff3
Merge branch 'main' into paths-mut-rework
riberk Oct 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
## notify 8.2.0 (unreleased)
- FEATURE: notify a user if the `max_user_watches` has been reached implicitly (`INotifyWatcher`) [#698]
- FIX: `INotifyWatcher` ignores events with unknown watch descriptors (instead of `EventMask::Q_OVERFLOW`) [#700]
- FEATURE: deprecate `Watcher::paths_mut` and introduce `update_paths` [#705]

[#698]: https://github.com/notify-rs/notify/pull/698
[#700]: https://github.com/notify-rs/notify/pull/700
[#705]: https://github.com/notify-rs/notify/pull/705

## notify 8.1.0 (2025-07-03)
- FEATURE: added support for the [`flume`](https://docs.rs/flume) crate
Expand Down
61 changes: 60 additions & 1 deletion notify/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Configuration types

use std::time::Duration;
use std::{path::PathBuf, time::Duration};

/// Indicates whether only the provided directory or its sub-directories as well should be watched
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
Expand Down Expand Up @@ -123,3 +123,62 @@ impl Default for Config {
}
}
}

/// Single watch backend configuration
///
/// This contains some settings that may relate to only one specific backend,
/// such as to correctly configure each backend regardless of what is selected during runtime.
#[derive(Debug)]
pub struct WatchPathConfig {
recursive_mode: RecursiveMode,
}

impl WatchPathConfig {
/// Creates new instance with provided [`RecursiveMode`]
pub fn new(recursive_mode: RecursiveMode) -> Self {
Self { recursive_mode }
}

/// Set [`RecursiveMode`] for the watch
pub fn with_recursive_mode(mut self, recursive_mode: RecursiveMode) -> Self {
self.recursive_mode = recursive_mode;
self
}

/// Returns current setting
pub fn recursive_mode(&self) -> RecursiveMode {
self.recursive_mode
}
}

/// An operation to apply to a watcher
///
/// See [`Watcher::update_paths`] for more information
#[derive(Debug)]
pub enum PathOp {
/// Path should be watcher
Watch(PathBuf, WatchPathConfig),

/// Path should be unwatched
Unwatch(PathBuf),
}

impl PathOp {
/// Watch the path with [`RecursiveMode::Recursive`]
pub fn watch_recursive<P: Into<PathBuf>>(path: P) -> Self {
Self::Watch(path.into(), WatchPathConfig::new(RecursiveMode::Recursive))
}

/// Watch the path with [`RecursiveMode::NonRecursive`]
pub fn watch_non_recursive<P: Into<PathBuf>>(path: P) -> Self {
Self::Watch(
path.into(),
WatchPathConfig::new(RecursiveMode::NonRecursive),
)
}

/// Unwatch the path
pub fn unwatch<P: Into<PathBuf>>(path: P) -> Self {
Self::Unwatch(path.into())
}
}
66 changes: 57 additions & 9 deletions notify/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
//! Error types

use crate::Config;
use crate::{Config, PathOp};
use std::error::Error as StdError;
use std::fmt::Debug;
use std::path::PathBuf;
use std::result::Result as StdResult;
use std::{self, fmt, io};
Expand Down Expand Up @@ -158,14 +159,61 @@ impl<T> From<std::sync::PoisonError<T>> for Error {
}
}

#[test]
fn display_formatted_errors() {
let expected = "Some error";
/// The error provided by [`crate::Watcher::update_paths`] method
#[derive(Debug)]
pub struct UpdatePathsError {
/// The original error
pub source: Error,

/// The remaining operations that haven't been applied
pub remaining: Vec<PathOp>,
}

impl fmt::Display for UpdatePathsError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "unable to apply the batch operation: {}", self.source)
}
}

assert_eq!(expected, format!("{}", Error::generic(expected)));
impl StdError for UpdatePathsError {
fn source(&self) -> Option<&(dyn StdError + 'static)> {
Some(&self.source)
}
}

assert_eq!(
expected,
format!("{}", Error::io(io::Error::other(expected)))
);
impl From<UpdatePathsError> for Error {
fn from(value: UpdatePathsError) -> Self {
value.source
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn display_formatted_errors() {
let expected = "Some error";

assert_eq!(expected, format!("{}", Error::generic(expected)));

assert_eq!(
expected,
format!("{}", Error::io(io::Error::other(expected)))
);
}

#[test]
fn display_update_paths() {
let actual = UpdatePathsError {
source: Error::generic("Some error"),
remaining: Default::default(),
}
.to_string();

assert_eq!(
format!("unable to apply the batch operation: Some error"),
actual
);
}
}
54 changes: 23 additions & 31 deletions notify/src/fsevent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@

#![allow(non_upper_case_globals, dead_code)]

use crate::event::*;
use crate::{
unbounded, Config, Error, EventHandler, PathsMut, RecursiveMode, Result, Sender, Watcher,
};
use crate::{event::*, PathOp};
use crate::{unbounded, Config, Error, EventHandler, RecursiveMode, Result, Sender, Watcher};
use fsevent_sys as fs;
use fsevent_sys::core_foundation as cf;
use std::collections::HashMap;
Expand Down Expand Up @@ -267,29 +265,6 @@ extern "C" {
fn CFRunLoopIsWaiting(runloop: cf::CFRunLoopRef) -> cf::Boolean;
}

struct FsEventPathsMut<'a>(&'a mut FsEventWatcher);
impl<'a> FsEventPathsMut<'a> {
fn new(watcher: &'a mut FsEventWatcher) -> Self {
watcher.stop();
Self(watcher)
}
}
impl PathsMut for FsEventPathsMut<'_> {
fn add(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()> {
self.0.append_path(path, recursive_mode)
}

fn remove(&mut self, path: &Path) -> Result<()> {
self.0.remove_path(path)
}

fn commit(self: Box<Self>) -> Result<()> {
// ignore return error: may be empty path list
let _ = self.0.run();
Ok(())
}
Comment on lines -286 to -290
Copy link
Contributor

@branchseer branchseer Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added commit to give future implementations a chance to return errors at the time of commit. It was an intentional design choice with its own downside. Now on second thought, inconsistent states are indeed more harmful.

How about just replacing commit with drop?

impl Drop for FsEventPathsMut<'_>  {
  fn drop(&mut self) {
        // ignore return error: may be empty path list
        let _ = self.0.run();
  }
}

Would this change solve the inconsistent state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would, but it seems a bit confusing, isn't it?

I tried to rework it to be more clear and less verbose.

Compare

For known number of paths

let mut paths = watcher.paths_mut();
paths.add(path1, recursive)?;
paths.add(path2, recursive)?;
paths.add(path3, recursive)?;
patch.commit()?;

and

watcher.update_paths(vec![
    PathOp::watch_recursive(path1),
    PathOp::watch_recursive(path2),
    PathOp::watch_recursive(path3),
])?;

For unknown number of paths

let mut paths = watcher.paths_mut();
for path in my_paths {
    paths.add(path, recursive)?;
}
paths.commit()?;

and

watcher.update_paths(my_paths.iter().map(PathOp::watch_recursive).collect());

Copy link
Contributor

@branchseer branchseer Jul 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that commit is move to drop, there would be no commit call:

let mut paths = watcher.paths_mut();
for path in my_paths {
    paths.add(path, recursive)?;
}

It seems to me the commit-less version of paths_mut is now just a lower-level version of update_paths. It's now a problem of verbosity rather than inconsistency.

I appreciate your effort to make the API more friendly. But personally, I'd always prefer to expose a lower-level API than a friendly API in a rust library. You can always create a update_paths function externally based on paths_mut if you feel paths_mut is too verbose.

I know the performance is not really relevant here, but as a Rust developer, it just feels weird to see an API requiring a Vec, whereas paths_mut is a commonly known "builder" pattern (example: bindgen::Builder).

If you still strongly feel update_paths is essential in notify, please at least consider keeping paths_mut as well for people like me who prefer a builder-style API.

cc @dfaust

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder api is a good choice there and it can be made without lib-support, you can just build a Vec with some UpdatePathsBuilder you made. I think example proves it - they take any Into<String> into builder method header (for example) and make Box<str> that is added into the Vec<Box<str>>. Watcher just can't take generics because of dyn compability but the effect is the same as in your example - build some in-memory stuff to call the main method (build, update_paths, etc).

To be honest, PathsMut just pretends to be a builder, but it is not, because it doesn't build anything new, it mutates the watcher

You can always create a update_paths function externally based on paths_mut

Yes, but I just try to explain my opinion, that an API that allows user to have notify objects in a state that can't be come in any other ways is not a good choice. The verbosity reduction is just a side effect.

If we remove commit this state is still there - after paths_mut call. There are some implicit things that may be really confusing, for me it would be "Could I call long-time running code between add calls?". As a user I wouldn't expect, that I don't get any events after paths_mut call. This argument may work with my API choice either, but at least update_paths doesn't allow user to do anything while service is being stopped and forces user to wait the call finishes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@branchseer I think about it in a descriptive manner: how I explain that thing to a user.

PathsMut

  • Call paths_mut method and store the object you got. The method may fail in cases, that depend on the watcher you use (but the cases are not the same as in watch / unwatch calls)
  • Call add and / or remove methods on the given object. They may fail some cases, that similar to the watch unwatch but not the same
  • Drop the given object to apply the changes. It may be noop or do some things like running the macos fsevents service

update_paths

  • Make the changes you want to apply as a Vec of PathOp (PathOp::Watch or PathOp::Unwatch)
  • Call the update_paths method with the Vec. The method will fail in the exactly the same cases as watch and unwatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting approach - it does avoid the main issue of inconsistent watcher state externally.

That said, the resulting API feels a bit unusual.

And the allocation is still present - a Box is required either way.

Do you think avoiding allocations here justifies making the API substantially less clear? I was under the impression that, compared to filesystem calls, memory allocation was considered negligible.

Copy link
Contributor

@branchseer branchseer Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the callback style looks very natural to me. Here's a similar API from std: std::thread::scope.

Besides the builder parameter, I like every method returning the same type of plain error that can be easily handled using ?. To me UpdatePathsError is a high cognitive overhead.

This is just expressing my personal state, not to undermine your work :) Really appreciate you putting time and thoughts into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a similar API from std: std::thread::scope

Yes, but thread::scope takes a generic parameter and you don't have to box it

Besides the builder parameter, I like every method returning the same type of plain error that can be easily handled using ?.

Good point. I've added impl From<UpdatePathsError> for Error

To me UpdatePathsError is a high cognitive overhead

Yeh, it is, but we do need a way to handle paths, that weren’t successfully added. In some cases if you don't care about errors you can call update_paths in a loop

let mut paths_to_add = ...;
while !paths_to_add.is_empty() {
    if let Err(e) = watcher.update_paths(std::mem::take(&mut paths_to_add)) {
        paths_to_add = e.remaining
    }
}

with Box<dyn FnMut(PathsMut) -> Result<(), Error>> we have to do something like that:

let mut paths_to_add = ...;
loop {
    let _ = watcher.update_paths(Box::new(|mut paths_mut| {
        let mut iter = std::mem::take(paths_to_add).drain();
        while let Some(operation) = iter.next() {
            if paths_mut.add(operation).is_err() {
                paths_to_add.extend(iter);
                return Ok(())
            }
        }
        Ok(())
    }));
}

This is just expressing my personal state, not to undermine your work :) Really appreciate you putting time and thoughts into this.

No worries at all - I didn’t take it that way.

That said, I feel like in our efforts to improve the PathsMut API, we might be losing some of the broader context. Specifically, beyond just adjusting how we handle paths, we also need to account for other inputs — like supporting not just RecursiveMode, but also more general configuration via WatchConfig.

Changing just PathsMut would be a breaking change, so we’ll need to evolve the API structure anyway. As I mentioned earlier, I see a few possible directions to address this.

Another option would be:

  • Adding a method to the PathsMut trait, e.g. watch(&Path, WatchConfig). Since it's not really user-implementable, we can technically do this. But this would still suffer from the same core issue - inconsistency: either methods that never fail but return Result, or strange pre-commit state. In this case, update_paths becomes unnecessary, but to be honest, I'm strongly against this direction - it feels like an awkward and messy API.
  • Keeping PathsMut as an internal "mutation builder", and using something like Box<dyn FnMut(&mut PathsMut) -> Result<(), Error>>. Personally, I find this approach less clear overall. It also wouldn’t solve the inconsistency between calling paths_mut() and commit() means we’d still need to deprecate Watcher::paths_mut eventually.
  • Deprecating paths_mut and PathsMut entirely, update_paths takes a Vec<PathOp>. This is what the current PR does. I like this approach best - which is why I implemented it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the callback approach best. It fixes the inconsistency issue and it should work with an implementation of #632, right? It allows error propagation and requires only a single allocation, which is not an issue in our io heavy scenario. It should also allow to optimize the clones it mentioned away (but that's for another day).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my two cents: overall I agree with @riberk, the current impl does look like cleaner over the callback one. The error handling with the callback approach looks a deal breaker and wouldn't be user-friendly IMO. I'm also not sure allocation on Box vs. Vec would make a noticable difference here (in general).

}

impl FsEventWatcher {
fn from_event_handler(event_handler: Arc<Mutex<dyn EventHandler>>) -> Result<Self> {
Ok(FsEventWatcher {
Expand Down Expand Up @@ -321,6 +296,23 @@ impl FsEventWatcher {
result
}

fn update_paths_inner(
&mut self,
ops: Vec<crate::PathOp>,
) -> crate::StdResult<(), crate::UpdatePathsError> {
self.stop();

let result = crate::update_paths(ops, |op| match op {
crate::PathOp::Watch(path, config) => self.append_path(&path, config.recursive_mode()),
crate::PathOp::Unwatch(path) => self.remove_path(&path),
});

// ignore return error: may be empty path list
let _ = self.run();

result
}

#[inline]
fn is_running(&self) -> bool {
self.runloop.is_some()
Expand Down Expand Up @@ -586,14 +578,14 @@ impl Watcher for FsEventWatcher {
self.watch_inner(path, recursive_mode)
}

fn paths_mut<'me>(&'me mut self) -> Box<dyn PathsMut + 'me> {
Box::new(FsEventPathsMut::new(self))
}

fn unwatch(&mut self, path: &Path) -> Result<()> {
self.unwatch_inner(path)
}

fn update_paths(&mut self, ops: Vec<PathOp>) -> crate::StdResult<(), crate::UpdatePathsError> {
self.update_paths_inner(ops)
}

fn configure(&mut self, config: Config) -> Result<bool> {
let (tx, rx) = unbounded();
self.configure_raw_mode(config, tx);
Expand Down
Loading
Loading