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

Inotify::event_stream is error-prone #176

Closed
hannobraun opened this issue May 28, 2021 · 2 comments · Fixed by #199
Closed

Inotify::event_stream is error-prone #176

hannobraun opened this issue May 28, 2021 · 2 comments · Fixed by #199

Comments

@hannobraun
Copy link
Owner

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 #112 (comment) and #112 (comment).

@talklittle
Copy link
Contributor

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
Copy link
Owner Author

Thank you for your assessment, @talklittle!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants