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

Watch triggers multiple times for non-markdown on APFS (macOS) #2133

Closed
bgotink opened this issue Jul 18, 2023 · 2 comments · Fixed by #2152 or #2157
Closed

Watch triggers multiple times for non-markdown on APFS (macOS) #2133

bgotink opened this issue Jul 18, 2023 · 2 comments · Fixed by #2152 or #2157
Labels
C-bug Category: A bug, incorrect or unintended behavior Command-watch Command: watch E-Help-wanted Experience: Help Needed

Comments

@bgotink
Copy link

bgotink commented Jul 18, 2023

Problem

Editing non-.md files in the book's source folder triggers multiple rebuilds when using mdbook serve. The amount of rebuilds is not constant, I've seen it vary from 2 to over 20 times. Editing markdown files triggers a single rebuild.

Steps

  1. cd $(mktemp -d)
  2. mdbook init </dev/null
  3. echo "export let lorem = 'ipsum';" > src/test.ts
  4. echo -e '```\n{{#include test.ts}}\n```' >> src/chapter_1.md
  5. mdbook serve
  6. in a second terminal: touch src/test.ts
terminal showing over a dozen rebuilds

Possible Solution(s)

Switching notify from macos_fsevent to macos_kqueue seems to fix the issue, triggering only a single rebuild whenever the src/test.ts file changes.

Notes

This happens on an APFS drive on macOS. It doesn't happen on ext4 in a linux VM.

I'm not sure why this happens. A simple test using the following code doesn't trigger the multitude of events, leading me to think the cause lies within mdbook and not in notify.

use std::{path::Path, thread::sleep, time::Duration};

use notify::RecursiveMode;

fn main() {
    let mut debouncer =
        notify_debouncer_mini::new_debouncer(Duration::from_secs(1), None, |event| {
            println!("event {:#?}", event);
        })
        .unwrap();

    let watcher = debouncer.watcher();

    watcher
        .watch(Path::new("."), RecursiveMode::Recursive)
        .unwrap();

    loop {
        sleep(Duration::from_millis(500));
    }
}

Version

mdbook v0.4.32 (also happens on v0.4.31, earlier versions not tested)
@bgotink bgotink added the C-bug Category: A bug, incorrect or unintended behavior label Jul 18, 2023
@ehuss ehuss added E-Help-wanted Experience: Help Needed Command-watch Command: watch labels Jul 18, 2023
@earv1
Copy link

earv1 commented Jul 19, 2023

Hey, I think I found something.
It seems like the copy_files_except_ext on line 631 is causing the file to be picked up as changed.

image

If you comment this line out it only gets picked up once.

@ehuss
Copy link
Contributor

ehuss commented Aug 2, 2023

I did a little bit of investigation which I posted at notify-rs/notify#465 (comment). This issue is roughly:

  1. mdBook detects an "extra" file is modified.
  2. mdBook deletes the output directory
  3. mdBook regenerates the book, and calls fs::copy on the "extra" file into the output directory.
  4. notify is triggering an event for the fs::copy with flags INODE_META_MOD | ITEM_MODIFIED | IS_FILE | ITEM_CLONED.
  5. mdBook thinks the file is modified, and loops back to step 1.

We can't ignore events with those flags, because that's the exact same set of flags that macOS generates if you modify the already cloned file.

Some rough ideas for different options:

  • On macOS, don't use fs::copy, but instead manually copy the contents of the file. This uses more disk space, but probably not harmful.
    • Hard links might also work, but copy is probably more reliable.
  • Have an option to use a different notify watcher, like PollWatcher. This could help with issues like mdbook serve doesn't work in Docker #2102, but there isn't a convenient place to put user-specific configurations, and the PollWatcher performance could be noticeably bad.
  • Switch notify to the kqueue backend.

I have been testing the last option, and it seems to work more reliably. It might be a risky change, but we'll see if anyone runs into any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: A bug, incorrect or unintended behavior Command-watch Command: watch E-Help-wanted Experience: Help Needed
Projects
None yet
3 participants