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

Add file notifier integration into the #17

Merged
merged 31 commits into from
Sep 21, 2022
Merged

Add file notifier integration into the #17

merged 31 commits into from
Sep 21, 2022

Conversation

egregius313
Copy link
Collaborator

Uses the notify crate in order to use the file watching/notification functionality often provided by operating systems (eg inotify(7) on Linux.

Also wraps io::Error and notify::Error into a custom error type using thiserror.

Closes #16

In order to use file watching, it is necessary to be able to share data
between threads. Therefore, adding the `Send` requirement to the
`input_stream` and `output_stream` methods of `Opts` lets us share the
streams.
@egregius313 egregius313 added the enhancement New feature or request label Sep 16, 2022
Copy link
Owner

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

I love this!

Let's make a few tweaks, and get this into a new release!

src/errors.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Copy link

@jorendorff jorendorff left a comment

Choose a reason for hiding this comment

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

Are you on Linux? If so, please run this and the original under perf stat to see how much CPU each one uses. It's possible for this approach to be much worse, especially in the case where the file gets new input every few milliseconds.

Without any tests for this functionality, it's also possible it doesn't work. :-\

@CleanCut
Copy link
Owner

@jorendorff It works on my machine. 🤣

Seriously, though. I measured it on macOS and the CPU usage dropped to a stable 0.0%, compared to ~0.2% before this change. It appears to actually be interrupt-driven now. And my manual tests still showed normal function. So that's one data point.

@CleanCut
Copy link
Owner

I did a large refactor last night. I think it works well (I mean it seems great when I manually test it out), but I'm struggling to write an integration test. If anyone wants to help me figure out what's going on, that'd be great. Otherwise I'll return to the problem tonight or tomorrow and keep tinkering away.

@CleanCut
Copy link
Owner

Okay, so the root of the problem is that Rust doesn't block on filesystem operations. I needed to add some delays to let file writes, file removes, and headtail's reads to all happen sequentially instead of in parallel.

@CleanCut
Copy link
Owner

CleanCut commented Sep 21, 2022

CI does not appear to have inotify available on Linux. The fallback polling implementation doesn't seem to detect file deletion and recreation. So it doesn't appear we can test this feature on Linux CI. 😢 I'm going to try the other OS's...

Copy link
Owner

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

Phew! This turned out to be quite involved!

@CleanCut CleanCut merged commit 40db48d into main Sep 21, 2022
@CleanCut CleanCut deleted the file-notifier branch September 21, 2022 22:41
@CleanCut
Copy link
Owner

This PR is included in v0.3.1, just released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change follow to use a file watcher when possible
3 participants