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.

Sign up for GitHub

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

Jump to bottom

Stream-based API is difficult to use due to buffer lifetime #120

Closed
hannobraun opened this issue Sep 7, 2018 · 4 comments
Closed

Stream-based API is difficult to use due to buffer lifetime #120

hannobraun opened this issue Sep 7, 2018 · 4 comments
Labels
help wanted

Comments

@hannobraun
Copy link
Owner

See #106 (comment).

#119 adds an example for working around this issue.
@hannobraun hannobraun mentioned this issue Sep 7, 2018
Open
@hannobraun
Copy link
Owner Author

I don't think I'll have time to work on this very soon. I'll take a look eventually, but in the meantime, any help is appreciated.

@hannobraun hannobraun mentioned this issue Sep 7, 2018
Merged
ceyusa added a commit to ceyusa/inotify that referenced this issue Sep 7, 2018
@ceyusa
test: add an example using tokio's event-loop
7976b80 
This example shows how to use inotify's stream as future polled by
tokio's event loop which is cheap in computational terms using Rust
idioms.

It also shows how to workaround issue hannobraun#120
hannobraun#120
@hannobraun hannobraun mentioned this issue Sep 25, 2018
Closed
@hawkw
Copy link
Contributor

hawkw commented Jan 24, 2019

I have a potential solution for this, but it'll require another API change. Happy to open a PR if you're interested.

@hannobraun
Copy link
Owner Author

@hawkw

I have a potential solution for this, but it'll require another API change. Happy to open a PR if you're interested.

Thank you, I'm always interested in fixing problems. Breaking compatibility is a downside, of course, but I think it's unreasonable to rule that out while the API is having problems like this.

hawkw added a commit to hawkw/inotify that referenced this issue Jan 25, 2019
@hawkw
fix: Allow more flexible EventStream buffers
4a06f53 
This commit changes the `EventStream` type to hopefully avoid some of the
previously observed difficulties (viz. hannobraun#120) around the lifetime
of the user-provided buffer.

Previously, calls to `EventStream::new` were required to provide an
`&'a mut [u8]`, where the `EventStream` is generic over the lifetime
`'a`. This is an issue in many cases, where the `EventStream` must
live for the `'static` lifetime, as it requires a `'static mut` buffer,
which is difficult to create safely.

This branch changes the `EventStream` type to be generic over a type `T`
which implements `AsRef<[u8]>` and `AsMut<[u8]>`. Since an `&mut [u8]`
implements these traits, means that the `EventStream` may now be
constructed with an owned buffer _or_ a borrowed buffer. Thus, users
can pass in a `&mut [u8]` to a short-lived `EventStream`, while a
`'static` one can be constructed by passing in a `[u8; N]`, a `Vec<u8>`,
_or_ a `&'static mut [u8]`.

Additionally, this means that buffer types from other crates (such as
`bytes`) could theoretically be used, without requiring `inotify` to
depend on those crates. I'm not sure if this would actually happen that
often in practice, but it's a nice side benefit.

Fixes hannobraun#120.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw mentioned this issue Jan 25, 2019
Merged
hawkw added a commit to hawkw/inotify that referenced this issue Jan 25, 2019
@hawkw
fix: Allow more flexible EventStream buffers
ea3e7a3 
This commit changes the `EventStream` type to hopefully avoid some of
the previously observed difficulties (viz. hannobraun#120) around the lifetime of
the user-provided buffer.

Previously, calls to `EventStream::new` were required to provide an
`&'a mut [u8]`, where the `EventStream` is generic over the lifetime
`'a`. This is an issue in many cases, where the `EventStream` must
live for the `'static` lifetime, as it requires a `'static mut` buffer,
which is difficult to create safely.

This branch changes the `EventStream` type to be generic over a type
`T` which implements `AsRef<[u8]>` and `AsMut<[u8]>`. Since an
`&mut [u8]` implements these traits, means that the `EventStream` may
now be constructed with an owned buffer _or_ a borrowed buffer. Thus,
users can pass in a `&mut [u8]` to a short-lived `EventStream`, while a
`'static` one can be constructed by passing in a `[u8; N]`, a
`Vec<u8>`, _or_ a `&'static mut [u8]`.

Additionally, this means that buffer types from other crates (such as
`bytes`) could theoretically be used, without requiring `inotify` to
depend on those crates. I'm not sure if this would actually happen that
often in practice, but it's a nice side benefit.

Fixes hannobraun#120.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Contributor

hawkw commented Jan 25, 2019

@hannobraun I've opened #124 to resolve this issue --- please let me know what you think! It works quite well in my use-case but do let me know if I've overlooked something.

@hannobraun hannobraun mentioned this issue Jan 28, 2019
Closed
Atul9 added a commit to Atul9/inotify that referenced this issue Jun 20, 2019
@Atul9
Remove example that works around issue hannobraun#120
1fc4f20
hannobraun added a commit that referenced this issue Jun 23, 2019
@hannobraun
Merge pull request #130 from Atul9/remove-example
75f5226 
Remove example that works around issue #120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@hannobraun @hawkw