inotify-rs
inotify-rs copied to clipboard
Consider unifying traditional and future-based APIs
Currently there are two parallel APIs: The original one, and the new future-based one. I think it would be ideal if we could unify those.
One problem I see is efficiency. The stream-based API requires one additional heap allocation for every event that has a name
. The reason for this are lifetime issues that probably can't be resolved, at least not in safe Rust.
I think it's best for now to keep things as they are, while keeping a look at how futures develop. Feedback is very welcome!
One thought on this would be that the current stream API takes &mut on the Inotify and does not provide a way to add additional watches. I'm working on something where I want to recursively watch a directory, and thus need to add new watches on any newly created directories. One solution would be to allow mutably borrowing an inotify from the stream, or adding functions to add/remove watches directly.
Personally I would favor adding a method to clone an Inotify. It should be safe (in a Rust sense) and the underlying representation is already friendly to this. I understand this would permit certain undesirable uses (e.g., two streams on the same inotify would each get a subset of events).
I've tried a few ways of writing this involving multiple threads, etc, which have all been clumsy. Next I want to try using select
along with read_events
which I think should be simpler.
Thank you for your feedback, @calmofthestorm. I'm very open to improving the situation and make your use case possible, but I think allowing users to clone Inotify
would be taking it too far. It would be very error-prone, for the reasons you mention. Adding more methods to EventStream
would certainly be acceptable. Maybe there could be a separate API, Watches
, with add
and remove
methods that can be accessed both through Inotify
and EventStream
(e.g. inotify.watches().add(...)
). Maybe it could even be possible to clone a Watches
and use it anywhere.
But does creating an EventStream
even prevent you from using the original Inotify
instance? Inotify::event_stream
takes &mut self
, but the resulting EventStream
doesn't keep that reference. That means it should already be possible to use an Inotify
and an arbitrary number of EventStream
s in parallel, which seems wrong to me.
Maybe it would be better to replace Inotify::event_stream
with an into_event_stream
method that consumes the Inotify
, and make sure EventStream
has all the methods that users need.
Thanks, I missed that the mut reference is not saved by EventStream
(but I agree with your concern that this is wrong).
I like into_event_stream
(and ideally a way to go back as well if it's easy to implement), along with having .watches()
on both Inotify
and EventStream
. I could even see an argument to make Watches
cloneable and of separated ownership (i.e., contains its own reference to the Arc with the fd).
Then you have two separate concerns -- configuring the inotify, and consuming its events. The latter can be made unique to solve the two streams on the same fd problem.
Fully agreed!
I've opened https://github.com/hannobraun/inotify-rs/issues/176 and https://github.com/hannobraun/inotify-rs/issues/177 to track the improvements we talked about here. Any help in implementing them is very welcome. I don't have much time to work on this crate these days, but I'm standing by to review and merge any incoming pull requests.