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

refactor: PollWatcher #409

Merged
merged 2 commits into from Jun 15, 2022
Merged

refactor: PollWatcher #409

merged 2 commits into from Jun 15, 2022

Conversation

visig9
Copy link
Contributor

@visig9 visig9 commented May 21, 2022

Refactor PollWatcher to make it clean for further develop.

This commit would not change any behaviors user observable (not include
Debug implementation) and public documentation.


I also add some QUESTION and FIXME in code comment to descript some weird (IMHO) behaviors. (Maybe we can fix it, or they have some pretty good reason so we should add description beside them)

If need any further change, please let me know, thanks.

Refactor PollWatcher to make it clean for further develop.

This commit would not change any behaviors user observable (not include
Debug implementation) and public documentation.
@visig9
Copy link
Contributor Author

visig9 commented May 22, 2022

Found a different behavior:

In compare_contents mode, original implementation use different RandomState (BuildHasher) instance in watch() and internal loop. So, event handler may immediate receive a Modify(Data(Any)) event after user assign a new watch point.

See:

  • notify/src/poll.rs

    Lines 158 to 162 in 0245625

    fn run(&mut self) {
    let watches = self.watches.clone();
    let open = self.open.clone();
    let delay = self.delay;
    let build_hasher = self.compare_contents.then(RandomState::default);
  • notify/src/poll.rs

    Lines 311 to 312 in 0245625

    fn watch_inner(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<()> {
    let build_hasher = self.compare_contents.then(RandomState::default);

After refactor this behavior was gone. I think it's a bug so not worth to add it back, only note it here.

Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks very good.

Sometimes I'm not quite sure when cargo fmt does accept something or doesn't..

fn watch_inner(&mut self, path: &Path, recursive_mode: RecursiveMode) {
// HINT: Make sure always lock in the same order to avoid deadlock.
//
// FIXME: inconsistent: some place mutex poison cause panic, some place just ignore.
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather we throw this back to the user or decide on a way to handle it.
Ignoring isn't something we want to do, except if we note why.

@0xpr03 0xpr03 merged commit fcaeb0e into notify-rs:main Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants