fix: Allow more flexible EventStream buffers #124
Conversation
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
force-pushed
the
eliza/more-flexible-event-stream
branch
from
4a06f53
to
ea3e7a3
Compare
There was a problem hiding this comment.
I love it!
I haven't tested this thoroughly, to make sure it's strictly better than the previous API, but:
- It does fix Stream-based API is difficult to use due to buffer lifetime #120.
- All tests and examples are still working.
- I really can't think of any new problems this would introduce.
So I'm confident merging this. Thank you @hawkw!
| @@ -26,7 +22,7 @@ fn main() -> Result<(), io::Error> { | |||
| }); | |||
|
|
|||
| let future = inotify | |||
| .event_stream(unsafe { &mut BUFFER }) | |||
There was a problem hiding this comment.
I think this example should be removed and this technique here should be part of the API documentation (as a small example). Not necessary to do it in this PR, but we shouldn't forget to open an issue, at least.
| @@ -61,7 +61,7 @@ fn it_should_watch_a_file_async() { | |||
|
|
|||
| use futures::Stream; | |||
| let events = inotify | |||
| .event_stream(&mut buffer) | |||
| .event_stream(&mut buffer[..]) | |||
There was a problem hiding this comment.
I assume this change was required to get it to compile? If so, this is a breaking change. Not a problem, just want to confirm.
There was a problem hiding this comment.
Yeah, that's correct --- it's only necessary when the array is longer than [T; 32], as only arrays of length 0-32 have AsRef/AsMut impls.
|
@hawkw I'd prefer to let this sit for a while, due to laziness/busyness, to let it mature for a bit, and potentially to group it with other breaking changes that might be coming. But I'm happy to cut a release right now, if that helps you out. |
|
@hannobraun if you're anticipating more breaking changes in the near future, I think we're fine with a Git dependency temporarily. Alternatively, I think it may be possible to make the change more backwards-compatible, by adding a new constructor that takes a |
|
@hawkw I'm not anticipating more breaking changes, I just figured we might decrease some churn in case any more are coming. Sounds like a new release is the way to go though. I'll try to get it done soon. I agree that attempts to alleviate the breakage are probably counter-productive, for the reasons you mentioned. |
Since my PR against inotify (hannobraun/inotify-rs#124) has merged, we can go back to depending on the upstream version. Also, this updates the SHA we depend on to one that actually exists. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Since my PR against `inotify` (hannobraun/inotify-rs#124) has merged, we can go back to depending on the upstream version. Also, this updates the SHA we depend on to one that actually exists, which should fix the build. I've also removed the `inotify` dependency from the main proxy crate's `Cargo.toml.` This dependency is not actually used, as the `fs-watch` lib crate fully encapsulates the dependency. I think that this was overlooked when factoring out `fs-watch` into its own crate. It seems better to not specify the same dependency in multiple places, especially since we're not using one of them. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Since my PR against `inotify` (hannobraun/inotify-rs#124) has merged, we can go back to depending on the upstream version. Also, this updates the SHA we depend on to one that actually exists, which should fix the build. I've also removed the `inotify` dependency from the main proxy crate's `Cargo.toml.` This dependency is not actually used, as the `fs-watch` lib crate fully encapsulates the dependency. I think that this was overlooked when factoring out `fs-watch` into its own crate. It seems better to not specify the same dependency in multiple places, especially since we're not using one of them. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This commit changes the
EventStreamtype to hopefully avoid some ofthe previously observed difficulties (viz. #120) around the lifetime of
the user-provided buffer.
Previously, calls to
EventStream::newwere required to provide an&'a mut [u8], where theEventStreamis generic over the lifetime'a. This is an issue in many cases, where theEventStreammustlive for the
'staticlifetime, as it requires a'static mutbuffer,which is difficult to create safely.
This branch changes the
EventStreamtype to be generic over a typeTwhich implementsAsRef<[u8]>andAsMut<[u8]>. Since an&mut [u8]implements these traits, means that theEventStreammaynow be constructed with an owned buffer or a borrowed buffer. Thus,
users can pass in a
&mut [u8]to a short-livedEventStream, while a'staticone can be constructed by passing in a[u8; N], aVec<u8>, or a&'static mut [u8].Additionally, this means that buffer types from other crates (such as
bytes) could theoretically be used, without requiringinotifytodepend on those crates. I'm not sure if this would actually happen that
often in practice, but it's a nice side benefit.
Fixes #120.
Signed-off-by: Eliza Weisman eliza@buoyant.io