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

Use PollWatcher to always get filesystem updates #2240

Closed
wants to merge 2 commits into from

Conversation

KFearsoff
Copy link
Contributor

See #2102 (comment) for explanation on this

Closes #2102, #2035, #383 and #1441

@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Nov 18, 2023
@ehuss
Copy link
Contributor

ehuss commented Nov 24, 2023

I vaguely recall trying the PollWatcher and having issues with it, but I can't remember what they were. I think it was mostly performance problems.

One issue is that with this change, mdBook always prints an error when it starts (assuming you don't have a theme directory):

2023-11-24 12:58:56 [WARN] (mdbook::cmd::watch): error while watching for changes: No such file or directory (os error 2) about ["/Users/eric/Temp/z30/theme"]

IIUC, the way this works is that every second it scans all the files, and checks if any mtime changes are there. Is that correct? I'm not sure I understand why this would need a debouncer if it scans every second, and the debouncer is removing duplicates once a second. How do the two of those interact?

For larger books, what is the expected performance concerns? What if the source is over a network drive?

cc @0xpr03, do you perhaps have some guidance on our usage of notify and using a PollWatcher?

@KFearsoff
Copy link
Contributor Author

One issue is that with this change, mdBook always prints an error when it starts (assuming you don't have a theme directory):

Makes sense. Shouldn't be hard to fix.

IIUC, the way this works is that every second it scans all the files, and checks if any mtime changes are there. Is that correct?

Yes! That's completely right.

I'm not sure I understand why this would need a debouncer if it scans every second, and the debouncer is removing duplicates once a second. How do the two of those interact?

Truth be told, I can't tell you this, because I simply don't know. Upstream provides an example of using debouncer with PollWatcher. I'm not sure if that makes sense, but my handwavy assumption is that debouncer may provide us with some safeguards against really weird edge cases like filesystem being extremely unresponsive for a second and then submitting two events simultaneously.

For larger books, what is the expected performance concerns?

Hard to say without benchmarking it. Alacritty seems to use 1 second, cargo-watch uses 5 seconds. Unfortunately, there doesn't seem to be much information about the performance hit.

What if the source is over a network drive?

Actually, it seems like inotify doesn't support NFS anyway: notify-rs/notify#64 (comment)

NFS isn't the only network filesystem, of course, but I would cautiously assume it to be the most popular one that can be used with mdbook.

Regarding large books and network drives in particular: I think it's a good idea to evaluate Kubernetes services for how they do live reloading. Overall, I've seen two approaches:

  1. Don't do any kind of filesystem watching at all, and just reload on SIGHUP or HTTP POST request on /-/reload endpoint
  2. Give up on live reloading completely and just re-create the container (often the case for static content, which is what we have)

Overall, I think supporting live reload on demand is a good idea. I'm not sure how high of a priority it has, considering that we haven't mapped out the user story for live reloading (as in, we don't know who actually needs this feature, what workarounds they use right now, and we don't even have a container image which is a prerequisite to talking with people who care about live reload). On a completely unrelated note: even if we don't support SIGHUP right now, there's a good reason to consider it, because we don't support SIGTERM either, which means containers end up getting SIGKILL'ed anyway because we don't have a graceful shutdown.

@0xpr03
Copy link
Contributor

0xpr03 commented Nov 25, 2023

There is no reason to use the debouncer with pollwatcher, unless you have timings that could make this work. Example would be to poll every 1 second and debounce every 2. So you can detect re-created files (depends on the editor of your choice), instead of just rendering nothing because in that second it was still gone.

It may also be useful in case you swap out debouncer backends depending on what your underlying filesystem is.

For bigger books it could be very resource intense. Though I do expect the OS to already have all of the relevant files cached from previous re-renders. On the flip side inotify can use up too many file handles in big workspaces, as it requires one file handle per watched node (folder,file). Which is not the case on windows, so those users won't see that problem.

As you said: It really depends on what your users actually expect from the live-reload. For instant and low-resource, low-idle live reloading, the pollwatcher might be a bad idea. But the moment someone comes around with networked files on linux (WSL -> Windows access), you've lost.

This ultimately just shows the need to get some kind of self-driven selection system into the full-debouncer, which somehow detects the underlying filesystem. Eventually, when I have time for that..

The easiest choice would be to allow both: Some flag so users can switch between native backends and the pollwatcher. Whether that still requires the debouncer I can't say. I definitely would use it for the native backends, too much chatter otherwise.

I hope I didn't confuse you even more.

@KFearsoff
Copy link
Contributor Author

That's a great write-up!

I looked into what the world has to offer a little bit more, apparently tsx also has this issue, and it currently doesn't expose the command-line flag: privatenumber/tsx#266

From my experience, there are two kinds of live-reloading: on-demand (for services e.g. in the cloud) and watching the filesystem directly (for dev environments). Generally, those are exposed by different commands: for on-demand, you have build, release and stuff, and for watching you use dev, serve. mdbook is a little bit weird in that it doesn't emphasize/provide a correct way to use the software in production, so people end up using mdbook serve for Docker containers (which doesn't really work without PollWatcher) and for local development.

There are a lot of things that can be done here, and I guess it ultimately comes down to user experience. I think I'll go with providing a flag for now.

@KFearsoff
Copy link
Contributor Author

I tried implementing a flag today, and failed miserably. I haven't figured out a way do provide new_debouncer_opt with a notifier that can be of either Watcher type, I'll have a look at it later too, but so far it seems impossible with current Rust features. I may be very wrong though!

@KFearsoff
Copy link
Contributor Author

Note: #2030 is ready to merge, and its live-reload functionality depends on this PR

@ehuss
Copy link
Contributor

ehuss commented Feb 24, 2024

Since I've been having a lot of problems lately with the watcher, I have opened #2325 which includes an option to set the backend, and uses a simple scanner which can handle pruning via gitignore and handles watching more things (like additional-css/js, adding/removing theme, etc.).

@KFearsoff Do you think you could check out that PR and try it out and see if that helps for any scenarios you were running into?

@ehuss
Copy link
Contributor

ehuss commented May 13, 2024

I'm going to close in favor of #2325. Thanks @KFearsoff!

@ehuss ehuss closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mdbook serve doesn't work in Docker
4 participants