Skip to content
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.

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

Notify does not handle the new IsFile event on macOS. #465

Open
wdanilo opened this issue Jan 23, 2023 · 7 comments
Open

Notify does not handle the new IsFile event on macOS. #465

wdanilo opened this issue Jan 23, 2023 · 7 comments

Comments

@wdanilo
Copy link

wdanilo commented Jan 23, 2023

System details

What you did (as detailed as you can)

I'm using cargo-watch 8.1.1 (because 8.3.0 does not work on new macOS). Recently, our build script, which uses cargo-watch under the hood, started looping indefinitely. After debugging it turned out, that when using std::fs::copy("a.txt", "b.txt") on macOS, a file system event IsFile is sometimes emitted on the source file ("a.txt"). The term "sometimes" is better explained here. Our build script is copying some files to target directory, which causes the sources to emit this event, which causes cargo-watch to re-run the whole operation again. The cargo-watch issue, where the authors asked me to post issue here can be found here: watchexec/cargo-watch#242

@wdanilo wdanilo changed the title Notify does not handle the new IsFile event on macOs. Notify does not handle the new IsFile event on macOS. Jan 23, 2023
@0xpr03
Copy link
Member

0xpr03 commented Jan 23, 2023

So from what I've read across all 4 issues, the COW operation emits an Event for the source with a flag IsFile set. And we should ignore events with that flag ? Does the target also receive an event ? I don't own any mac hardware. I also guess you're using the fsevent backend, not kqueue.

@0xpr03 0xpr03 added A-bug os-mac Z-needs info Needs more information labels Jan 23, 2023
@wdanilo
Copy link
Author

wdanilo commented Jan 23, 2023

@0xpr03 thanks for the reply. Yes, the target also receives the event "sometimes". However, I have not debugged deeply enough to be able define what "sometimes" means here. I believe that you should ignore this event, yes. Regarding whether I'm using queue or fsevents, TBH, I have no idea. Googling told me only that these two systems are available on macOS, but I didn't find a way to check which one is being used under the hood. If you want me to provide any logs / run some command/scripts, I'd be happy to. Does it answer your question?

@0xpr03
Copy link
Member

0xpr03 commented Jan 23, 2023

It has to be kqueue, based on watchexec/cargo-watch.
Simply pushing out a version that ignores this, without confirming it doesn't break anything is something I wouldn't want to do, neither for v4 nor v5. I also need confirmation we will receive any event at all for such COW operations, if we ignore the source event. Otherwise some people might rather receive any event at all (*). I could create a branch with that fix, and you'd have to test out a custom build of notify (and possibly watchexec+cargo-watch).

(*) For example rebuilding everything once stuff changes with a debouncer, so they don't care about specific events, but at least one has to be emitted.

@wdanilo
Copy link
Author

wdanilo commented Jan 24, 2023

@0xpr03 instead of ignoring this event we can as well just handle it and libraries using notify can pattern match on it and ignore it by themselves. I believe this is a way more generic and safe approach. What do you think about it?

@wdanilo
Copy link
Author

wdanilo commented Feb 19, 2023

@0xpr03 Hi :) Do you think that something could be done regarding this issue? Because of it, cargo-watch is not usable on M1 and M2 Macs right now, which I believe renders it as a pretty important issue. What do you think? :)

@0xpr03
Copy link
Member

0xpr03 commented Feb 19, 2023

I'm open for PRs. I don't own any macos hardware, so I can't develop this.

@ehuss
Copy link

ehuss commented Jul 30, 2023

I've been trying to debug this and have not been able to make reasonable progress. From the best as I can tell, there are events that can happen to a cloned file (created with fclonefileat) that cannot be distinguished between "this is a real event" and "this is caused by creating the clone". For example:

  • IS_FILE | ITEM_CLONED — This is triggered by fclonefileat on the source. This is ok, since this already gets filtered out by notify.
  • ITEM_CREATED | INODE_META_MOD | ITEM_MODIFIED | IS_FILE | ITEM_CLONED — This is triggered on the source by fclonefileat, presumably just to let you know that the file was cloned.
  • INODE_META_MOD | ITEM_MODIFIED | IS_FILE | ITEM_CLONED — This is triggered when modifying the source of an already cloned file.
  • INODE_META_MOD | ITEM_MODIFIED | IS_FILE | ITEM_CLONED — This is triggered when running fclonefileat on an already cloned file.

I don't think it would be correct to ignore events with ITEM_CLONED, and thus I have no idea how to fix this (other than switching to PollWatcher or something else). I would almost venture to say that this is a bug in macOS.

Here is a demo program. It has an "input" directory, and it copies files from there to an "output" directory whenever the input changes. Running this essentially loops an infinite number of times, since calling fs::copy is being treated as a modification of the source file.

use std::error::Error;
use std::path::Path;
use std::sync::mpsc::channel;
use std::time::Duration;

fn main() -> Result<(), Box<dyn Error>> {
    if Path::new("output").exists() {
        std::fs::remove_dir_all("output")?;
    }
    if Path::new("input").exists() {
        std::fs::remove_dir_all("input")?;
    }
    std::fs::create_dir("output")?;
    std::fs::create_dir("input")?;
    std::fs::write("input/abc", "test")?;

    std::thread::spawn(|| watch(Path::new("input"), true).unwrap());
    // std::thread::spawn(|| watch(Path::new("output"), false).unwrap());
    std::thread::sleep(Duration::from_secs(1));
    eprintln!("modfying file");
    std::fs::write("input/abc", "test 2")?;
    std::thread::park();
    Ok(())
}

fn watch(path: &Path, do_copy: bool) -> Result<(), Box<dyn Error>> {
    use notify::RecursiveMode::*;

    let (tx, rx) = channel();
    let mut debouncer = notify_debouncer_mini::new_debouncer(Duration::from_secs(1), None, tx)?;
    let watcher = debouncer.watcher();
    watcher.watch(path, Recursive)?;
    println!("Listening for changes... {path:?}");
    loop {
        match rx.recv().unwrap() {
            Ok(event) => {
                eprintln!("got event {event:?}",);
                if do_copy {
                    // This simulates what happens when rebuilding the output.
                    // Remove these two lines to prevent an infinite loop. 
                    // However, it will still loop twice (once for the modification, and once for the copy).
                    std::fs::remove_dir_all("output")?;
                    std::fs::create_dir("output")?;
                    for entry in std::fs::read_dir(path)? {
                        let entry = entry?;
                        eprintln!("copy {:?}", entry.path());
                        std::fs::copy(
                            entry.path(),
                            Path::new("output").join(entry.path().file_name().unwrap()),
                        )?;
                    }
                }
            }
            Err(e) => {
                eprintln!("failed {e:?}");
            }
        }
    }
}

CyriacBr added a commit to CyriacBr/modules-watcher that referenced this issue Sep 11, 2023
BREAKING CHANGES

Relying on FS events was not reliable across all platforms, and some events are outright ignored based on how the file changes (e.g some IDE do not truly "write" to files directly).
See notify-rs/notify#465 and notify-rs/notify#468.

We now use a dumb poll system that computes changes every 250ms. The EntryChange struct was thus improved to provide trigger cause, and this is what the watch callback will now send.

Additional changes:
- Update rust napirs packages and set napi cli to 2.14.8 to avoid duplicate generated content
- Adapt tests to the new watching system
- Update README
CyriacBr added a commit to CyriacBr/modules-watcher that referenced this issue Sep 11, 2023
BREAKING CHANGES

Relying on FS events was not reliable across all platforms, and some events are outright ignored based on how the file changes (e.g some IDE do not truly "write" to files directly).
See notify-rs/notify#465 and notify-rs/notify#468.

We now use a dumb poll system that computes changes every 250ms. The EntryChange struct was thus improved to provide trigger cause, and this is what the watch callback will now send.

Additional changes:
- Update rust napirs packages and set napi cli to 2.14.8 to avoid duplicate generated content
- Adapt tests to the new watching system
- Update README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants