feat: update to tokio 0.2 and futures 0.3 #134

blckngm · 2019-09-18T06:17:28Z

(Stability note: this feature depends on nightly rust and alpha releases of tokio 0.2 and futures 0.3. Maybe you want to hold off the merge until they are stable. In which case I will try to make sure that this PR is up to date.)

This is a minimal and straightforward update to make inotify up to date with std::task, async-await syntax and next versions of tokio and futures.

API changes

EventStream is now a futures 0.3 Stream

A futures 0.3 Stream can be used with async-await directly (cc Async/Await syntax support #121). See the stream example.
The buffer passed to the event_stream method is now required to be Unpin

Other changes

Mandatory dependencies is minimized: only tokio-net, tokio-io and futures-core are mandatory dependencies. And for tokio-net unused features are disabled. The full tokio crate and futures-util are dev-dependencies.
The crate is updated to 2018 edition, so that related tests and
examples can be written in async/await.
The CI config is modified to:
- Run on nightly
- Run with --no-default-features as well

hannobraun

Thank you for your pull requests, @sopium! This looks really good. I've left a few comments with questions and suggestions.

I'd like to hold off on merging this until everything has become stable.

@hawkw This concerns stuff you've worked on before, and you probably know a lot more about futures/tokio/async than I do. Want to take a look?

hannobraun · 2019-09-18T15:03:55Z

Cargo.toml

+libc                 = "0.2"
+mio                  = { version = "0.6", optional = true }
+tokio-io             = { version = "=0.2.0-alpha.4", optional = true }
+tokio-net            = { version = "=0.2.0-alpha.4", optional = true, default-features = false }


Some of the versions are pinned to an exact alpha version. Is there a specific reason for that?

I think requiring an exact version should usually be avoided, as that can cause problems with other crates that might have slightly different version requirements.

These crates might have breaking changes between different alpha versions, so they are pinned to a specific version. It won't be necessary once they are stable.

Makes sense.

hannobraun · 2019-09-18T15:04:14Z

Cargo.toml

-tempdir = "0.3"
+tempdir              = "0.3"
+futures-util-preview = "=0.3.0-alpha.18"
+tokio                = "=0.2.0-alpha.4"


Same as above. Versions are pinned.

hannobraun · 2019-09-18T15:08:54Z

src/inotify.rs

@@ -405,7 +405,7 @@ impl Inotify {
    pub fn event_stream<T>(&mut self, buffer: T)
        -> EventStream<T>
    where
-        T: AsMut<[u8]> + AsRef<[u8]>,
+        T: AsMut<[u8]> + AsRef<[u8]> + Unpin,


I'm a bit concerned about the Unpin, as it makes the API less flexible. I think right now one can pass an array, but with the Unpin, you'd either need a slice (possibly causing lifetime issues) or a heap allocation.

I'm not sure if it can be avoided and if that's a big deal in practice. I just figured I'd point out my concerns, in case you have something informative to say :-)

Arrays are Unpin. But your general point is valid.

Interesting, I didn't think they were!

hannobraun · 2019-09-18T15:09:18Z

src/inotify.rs

@@ -422,7 +422,7 @@ impl Inotify {
    pub fn event_stream_with_handle<T>(&mut self, handle: &Handle, buffer: T)
        -> io::Result<EventStream<T>>
    where
-        T: AsMut<[u8]> + AsRef<[u8]>,
+        T: AsMut<[u8]> + AsRef<[u8]> + Unpin,


See comment above about Unpin.

hannobraun · 2019-09-18T15:11:14Z

src/stream.rs

@@ -47,7 +39,7 @@ pub struct EventStream<T> {

 impl<T> EventStream<T>
 where
-    T: AsMut<[u8]> + AsRef<[u8]>,
+    T: AsMut<[u8]> + AsRef<[u8]> + Unpin,


See comment above about Unpin.

hannobraun · 2019-09-18T15:11:19Z

src/stream.rs

@@ -78,36 +70,37 @@ where

 impl<T> Stream for EventStream<T>
 where
-    T: AsMut<[u8]> + AsRef<[u8]>,
+    T: AsMut<[u8]> + AsRef<[u8]> + Unpin,


See comment above about Unpin.

hannobraun · 2019-09-18T15:19:42Z

src/stream.rs

    {
-        if self.unused_bytes == 0 {
+        let self_ = self.get_mut();


Can we use get_unchecked_mut here, to forgo the requirement for Unpin? I don't think we're moving out of self_, so it should be fine, I think.

hannobraun · 2019-09-18T15:30:26Z

Two more things:

Note to my future self: Travis didn't run for some reason. I'll have to figure out why, if it doesn't fix itself at some point.
@sopium: For future reference, I'd prefer to have each self-contained change (e.g. the update to Rust 2018) in its own commit. You don't need to change this pull request (it's not a big change anyway), but I'd appreciate it if future pull requests had smaller commits, as those are easier to review.

hannobraun · 2019-09-19T05:29:54Z

Thanks for the change, @sopium! Let's hope things become stable soon, so we can merge this. Until then, more reviews/comments are always welcome.

peku33 · 2019-09-30T22:29:46Z

Any updates on this? Waiting for merge! Maybe this could become available as pre-release?
https://stackoverflow.com/questions/46373028/how-to-release-a-beta-version-of-a-crate-for-limited-public-testing
I pretty much need this now and I'm ready to test it

hannobraun · 2019-10-01T06:42:00Z

@peku33 My understanding is, that this still requires nightly. I don't want to merge it as long as this is the case, as I want the master branch to always be in a releasable state (if someone submits a bugfix, for example).

Would it be practical for you to use a Git dependency in Cargo, to depend on @sopium's branch directly?

peku33 · 2019-10-01T14:12:03Z

@hannobraun How about doing the same as tokio itself did?
At the moment we have tokio-0.1.22 which is considered stable. We also have tokio-0.2.0-alpha.6 (as of today) which is a pre-release, IIRC requires rust nightly, futures-preview (also alpha). AFAIK, when you use cargo add tokio it will add 0.1.22. But running cargo add tokio --allow-prerelease will install 0.2.0-alpha.6 version. This allows users to choose stable/nightly version pretty easily.
I think keeping current version as stable is reasonable, but why not publish a pre-release (eg 0.8.0-alpha.1 or 1.0.0-alpha.1)?
Current codebase looks pretty stable, since inotify itself won'y probably evolve in the nearest future, there wouldn't probably be more development against current branch.

hannobraun · 2019-10-02T14:09:48Z

@peku33

I think keeping current version as stable is reasonable, but why not publish a pre-release

Because that would take time to do and I don't have much of that. From my perspective, it's a much more economical decision to just wait until everything is stable, then merge and release.

peku33 · 2019-10-04T17:45:20Z

Hmm, pretty sad, especially taking into account fact, that lots of popular crates uses such approach (tokio itself, mysql_async etc).

@hannobraun Are you sure accepting @sopium PR with version set to 0.8.0.alpha-1 is too much? This would not automatically update all clients to this unstable version. Even the main version on crates.io etc will still be the current stable version (take a look at https://crates.io/crates/mysql_async ) ;)

peku33 · 2019-10-04T20:23:53Z

@sopium Any way to use event_stream with buffer larger than 32 bytes? For now it looks like AsMut<[u8]> accepts arrays of up to 32 bytes...

blckngm · 2019-10-05T03:22:29Z

@sopium Any way to use event_stream with buffer larger than 32 bytes? For now it looks like AsMut<[u8]> accepts arrays of up to 32 bytes...

You can use Vec<u8>, &mut buf[..], or define a newtype wrapper:

struct LargeU8Array([u8; 512]);

impl AsRef<[u8]> for LargeU8Array {...}

impl AsMut<[u8]> for LargeU8Array {...}

hannobraun · 2019-10-07T06:04:05Z

@peku33

Hmm, pretty sad, especially taking into account fact, that lots of popular crates uses such approach (tokio itself, mysql_async etc).

I guess those crates have less busy maintainers then, because yes, I am sure that adding yet another thing to my plate would be too much.

You haven't commented on my suggestion to add a Git dependency on @sopium's branch in Cargo. Doing so would require no additional work from anyone.

If that's not good enough for some reason (the only one I can think of would be that you want to publish a crate with that dependency to crates.io), you can publish your own version of inotify. Call it peku33-inotify or inotify-async-nightly or whatever. Then anyone else who can't wait for the next version of inotify can use that in the meantime.

blckngm · 2019-11-01T07:26:17Z

The latest commit is in sync with tokio master.

In tokio master, PollEvented::new now returns io::Result<_>, and PollEvented::new_with_handle is removed. As a result, the event_stream API has changed accordingly.

If you are still using tokio 0.2.0-alpha.6, stick to the previous commit.

emarcotte · 2019-11-04T19:51:13Z

I'm not exactly fluent in all the ins/outs of the new async stuff. Is it reasonable to implement EventStream as a FusedStream to be more compatible with select!()? E.g. https://rust-lang.github.io/async-book/06_multiple_futures/03_select.html
~~I'm not sure if I should be wrapping it externally or if the stream itself should track a terminated flag... ~~
EDIT: I just saw StreamExt has fuse(). Woopsie!. Can't wait for the docs to catch up a bit :)

blckngm · 2019-11-27T06:53:36Z

Updated to tokio v0.2.1.

The event_stream_with_handle method is removed becuase PollEvented no longer has new_from_handle.

hannobraun · 2019-11-28T05:53:39Z

Thank you @sopium for your continued work on this! The changes look good to me, but for some reason, the CI build doesn't run. I've set up GitHub Actions instead. Can you please rebase on master, so it runs for this pull request?

`EventStream` is now a futures 0.3 stream (backed by tokio 0.2) and can be used with async/await. The stream example is updated accordingly. The crate is updated to the 2018 edition, so that related tests and examples can be written in async/await. The CI config is modified to: * Run on nightly * Run with `--no-default-features` as well

tokio 0.2.0-alpha.6 futures 0.3.0-alpha.19

hannobraun · 2019-11-29T05:31:02Z

Thank you, @sopium! I'll try to release a new version soon.

blckngm mentioned this pull request Sep 18, 2019

Async await support #135

Closed

hannobraun reviewed Sep 18, 2019

View reviewed changes

hannobraun added the status: blocked The issue is currently blocked by something outside this project. label Sep 18, 2019

hannobraun removed the status: blocked The issue is currently blocked by something outside this project. label Nov 28, 2019

blckngm added 11 commits November 28, 2019 21:25

Don't require that the buffer type is Unpin

d946056

Update tokio to 0.2.0-alpha.5

9c83429

Update tokio and futures

736a3cf

tokio 0.2.0-alpha.6 futures 0.3.0-alpha.19

Use the beta toolchain

e99d8a4

Update to use latest tokio

7b15937

Use futures 0.3

de63805

Update to tokio v0.2.1

01caa4c

Support stable rust again

ba74ec4

Remove reference to event_stream_with_handle in doc

96f6320

Build and test with --no-default-features in CI

75c4bd1

blckngm force-pushed the master branch from 0a61ce0 to 75c4bd1 Compare November 28, 2019 20:30

blckngm force-pushed the master branch 2 times, most recently from 3027d92 to 60dcd82 Compare November 28, 2019 20:38

Use rust 1.39 in CI

111c2df

blckngm force-pushed the master branch from 60dcd82 to 111c2df Compare November 28, 2019 20:39

hannobraun merged commit abd7936 into hannobraun:master Nov 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update to tokio 0.2 and futures 0.3 #134

feat: update to tokio 0.2 and futures 0.3 #134

blckngm commented Sep 18, 2019

hannobraun left a comment

hannobraun Sep 18, 2019

blckngm Sep 18, 2019

hannobraun Sep 19, 2019

hannobraun Sep 18, 2019

hannobraun Sep 18, 2019

blckngm Sep 18, 2019

hannobraun Sep 19, 2019

hannobraun Sep 18, 2019

hannobraun Sep 18, 2019

hannobraun Sep 18, 2019

hannobraun Sep 18, 2019

hannobraun commented Sep 18, 2019

hannobraun commented Sep 19, 2019

peku33 commented Sep 30, 2019 •

edited

Loading

hannobraun commented Oct 1, 2019

peku33 commented Oct 1, 2019

hannobraun commented Oct 2, 2019

peku33 commented Oct 4, 2019

peku33 commented Oct 4, 2019

blckngm commented Oct 5, 2019

hannobraun commented Oct 7, 2019

blckngm commented Nov 1, 2019

emarcotte commented Nov 4, 2019 •

edited

Loading

blckngm commented Nov 27, 2019

hannobraun commented Nov 28, 2019

hannobraun commented Nov 29, 2019

feat: update to tokio 0.2 and futures 0.3 #134

feat: update to tokio 0.2 and futures 0.3 #134

Conversation

blckngm commented Sep 18, 2019

API changes

Other changes

hannobraun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannobraun commented Sep 18, 2019

hannobraun commented Sep 19, 2019

peku33 commented Sep 30, 2019 • edited Loading

hannobraun commented Oct 1, 2019

peku33 commented Oct 1, 2019

hannobraun commented Oct 2, 2019

peku33 commented Oct 4, 2019

peku33 commented Oct 4, 2019

blckngm commented Oct 5, 2019

hannobraun commented Oct 7, 2019

blckngm commented Nov 1, 2019

emarcotte commented Nov 4, 2019 • edited Loading

blckngm commented Nov 27, 2019

hannobraun commented Nov 28, 2019

hannobraun commented Nov 29, 2019

peku33 commented Sep 30, 2019 •

edited

Loading

emarcotte commented Nov 4, 2019 •

edited

Loading