-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add a poll-based file watcher. #2325
Conversation
Yes please :) The current watcher is broken, although similar watchers like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested it locally, but the code looks good to me. Left a few minor comments here and there. Let's also add #2240 to the list of closes issues/PRs.
guide/src/cli/arg-watcher.md
Outdated
|
||
* `poll` (default) --- Checks for file modifications by scanning the filesystem every second. | ||
* `native` --- Uses the native operating system facilities to receive notifications when files change. | ||
This can have less constant overhead, but may not be as reliable as the `poll` based watcher. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to emphasize that the native notifier is a lot more unreliable than the poll-based watcher. it would also be great to list some of the issues that it causes, like issues with Docker, issues on Windows, issues on Mac etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I have added a little more context here.
.help("The filesystem watching technique"), | ||
); | ||
#[cfg(not(feature = "watch"))] | ||
return self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is a little weird to me. Wouldn't we want to feature gate the entire arg_watcher
function, instead of the body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require the callers to have to do some pretty awkward #[cfg]
statements to conditionally determine if the method is available (since #[cfg]
doesn't work well with builder patterns).
// Track average scan time, to help investigate if the poller is taking | ||
// undesirably long. This is not a rigorous benchmark, just a rough | ||
// estimate. | ||
const AVG_SIZE: usize = 60; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably have a visible warning emitted if the rebuild is taking more than 10s or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little concerned that would cause too many false positives. For example, just a temporary hiccup, or the user puts their machine to sleep, or whatever. I think we can add more here in the future if people report having issues.
This adds a poll-based file watcher which will poll the filesystem once every second for any file changes. We have had various issues with the operating-system native change notifications on various operating systems and filesystems. This poll watcher should be more reliable, and supports more features (such as detecting addition or removal of root paths, checking additional-css/js, etc.).
This includes a
--watcher
CLI option to give the user the option to go back to the native notifications if they are having issues with the overhead of the poll-based watcher.I'm not certain if we will continue to support both in the long term. I'm reluctant to keep the native facilities since it adds a fair bit of complexity and maintenance, and I suspect almost nobody will use it. However, I decided to keep the option in case someone needs it.
Closes #383
Closes #1441
Closes #1707
Closes #2035
Closes #2102