search

hannobraun / inotify-rs

Idiomatic inotify wrapper for the Rust programming language
ISC License
254 stars 64 forks source link

`Inotify::event_stream` is error-prone #176

Closed hannobraun closed 2 years ago

hannobraun commented 3 years ago

Inotify::event_stream enabled concurrent use of one Inotify instance and an arbitrary number of EventStream instances, which is error-prone. It should be replaced with an into_event_stream method that consumes the Inotify. Ideally, there should also be a matching method on EventStream, to convert back to Inotify.

See https://github.com/hannobraun/inotify-rs/issues/112#issuecomment-850352465 and https://github.com/hannobraun/inotify-rs/issues/112#issuecomment-850418244.

talklittle commented 2 years ago

The proposed solution makes sense to me. Between into_event_stream() versus passing a &mut Inotify to EventStream, the into option makes it clearer what the EventStream struct's API is, and relieves the caller of having to juggle two references (Inotify and EventStream). New APIs can be duplicated on both Inotify and EventStream if they both need access, like .watches().

Originally though, my thought was "why can't inotify-rs handle multiple EventStreams, like a PubSub pattern?" But I found there's not a drop-in solution for that. People have attempted such "forking" combinators for Rust futures, but there's not a stable, wide-consensus implementation yet (see Rust futures streams GitHub issues). The Tokio streams tutorial even delegates to Redis for PubSub functionality.

So in conclusion, Inotify.into_event_stream() seems the right solution and in line with what the Rust ecosystem currently provides. Downstream projects could implement their own multi-consumer solutions on top of the EventStream if needed.

hannobraun commented 2 years ago

Thank you for your assessment, @talklittle!