feat: Implement futures-0.2 support by Marwes · Pull Request #29 · tokio-rs/tokio-uds · GitHub

This repository was archived by the owner on May 15, 2018. It is now read-only.

Contributor

Marwes commented Feb 13, 2018 •

edited

Loading

tokio-rs/tokio-rfcs#2

Re-implement vectored read and write

BREAKING CHANGE

The needs_* methods has been removed as PollEvented do not have them anymore
The crate now uses the tokio-reactor crate instead of tokio_core
Implementation of the (depreceated) tokio_core::io::Io trait has been removed.

Marwes mentioned this pull request

Add an async interface using futures-rs redis-rs/redis-rs#141

Merged

Member

carllerche commented Feb 13, 2018

Thanks for doing this.

I would like to hold off on this for a little bit. Ideally, these libs would not depend on tokio directly. I plan on splitting out the reactor into a separate crate, and then tokio-uds can be updated to depend on that crate.

I should be able to get that work shipped pretty quickly (ideally this week).

Contributor Author

Marwes commented Feb 13, 2018

No, rush at all. Just one of these changes that are just as easy to send a PR for as they are to open an issue.

ufobat commented Mar 10, 2018

Since the last update on tokio, it would be awesome if this would work with the current tokio version.

Marwes force-pushed the upgrade_to_tokio branch from 00278c8 to 9e4d99a Compare

March 12, 2018 16:01

Contributor Author

Marwes commented Mar 12, 2018

@ufobat Partially moved the code over to use tokio and futures 0.2-alpha. There are parts of tokio that is not yet updated to work with futures 0.2 yet however so can't complete it yet.

ufobat commented Mar 13, 2018

thank you!

Member

carllerche commented Mar 23, 2018

Thanks for updating the PR! Let me take a look at this now.

Member

carllerche commented Mar 23, 2018

@Marwes Would it be possible to add an unstable-futures feature flag here as well?

Basically, follow the pattern that tokio-tcp takes? https://github.com/tokio-rs/tokio/tree/master/tokio-tcp

carllerche mentioned this pull request

Compatibility with tokio crate (not tokio-core) #31

Closed

Contributor Author

Marwes commented Mar 23, 2018

@Marwes Would it be possible to add an unstable-futures feature flag here as well?

Sure. I will look into it tomorrow.

Marwes force-pushed the upgrade_to_tokio branch 2 times, most recently from fbd7bfa to 131c301 Compare

March 24, 2018 19:09

Contributor Author

Marwes commented Mar 24, 2018

@carllerche It should be good now I think but I may have accidentally messed up something as I were restoring the old impls.

Contributor Author

Marwes commented Mar 28, 2018

ping @carllerche

Member

carllerche commented Apr 1, 2018

@Marwes Thanks for doing this. Sorry for the delay. I did a quick pass but I will have to focus on it more tomorrow. I will try to leave some quick inline notes.

The first thing that stands out is that there are a number of unrelated changes that make the PR harder to review. I will try to point them out inline. Would it be possible to revert those for this PR?

carllerche suggested changes

View reviewed changes

Member

carllerche left a comment

Thanks for doing this!

This isn't a comprehensive review as it is quite big, but it looks like it is going in the right direction.

It looks like rustfmt or something was run and it makes the diff a bit hard to review.

Would it be possible to revert formatting changes? Those can be addressed in a separate PR.

Cargo.toml Outdated

-              tokio-io = "0.1"
+              [package]
+              name = "tokio-uds"
+              version = "0.1.7"

Member

carllerche Apr 1, 2018

This will be a breaking change, so this should be bumped to v0.2.0.

src/frame.rs

               use std::path::PathBuf;
-              use futures::{Async, Poll, Stream, Sink, StartSend, AsyncSink};
+              use futures::{Async, AsyncSink, Poll, Sink, StartSend, Stream};

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

src/frame.rs

                   /// should be directed, which will be returned as a `SocketAddr`.
-                  fn encode(&mut self, msg: Self::Out, buf: &mut Vec<u8>)
-                            -> io::Result<PathBuf>;
+                  fn encode(&mut self, msg: Self::Out, buf: &mut Vec<u8>) -> io::Result<PathBuf>;

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

src/frame.rs

                       if self.wr.is_empty() {
-                          return Ok(Async::Ready(()))
+                          return Ok(Async::Ready(()));

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

src/frame.rs

                       } else {
-                          Err(io::Error::new(io::ErrorKind::Other,
-                                             "failed to write entire datagram to socket"))
+                          Err(io::Error::new(

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

src/lib.rs

                   /// more easily.
                   pub fn framed<C>(self, codec: C) -> UnixDatagramFramed<C>
-                      where C: UnixDatagramCodec,
+                  where

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

src/lib.rs

               impl<T, P> Future for SendDgram<T, P>
-                  where T: AsRef<[u8]>,
-                        P: AsRef<Path>
+              where

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

src/ucred.rs

		@@ -1,4 +1,4 @@
		use libc::{uid_t, gid_t};
		use libc::{gid_t, uid_t};

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

src/ucred.rs

               #[cfg(any(target_os = "linux", target_os = "android"))]
               pub mod impl_linux {
-                  use libc::{getsockopt, SOL_SOCKET, SO_PEERCRED, c_void, socklen_t};
+                  use libc::{c_void, getsockopt, socklen_t, SOL_SOCKET, SO_PEERCRED};

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

src/ucred.rs

                           let raw_fd = sock.as_raw_fd();
-                          let mut ucred = ucred { pid: 0, uid: 0, gid: 0 };
+                          let mut ucred = ucred {

Member

carllerche Apr 1, 2018

This does not seem related to the PR?

Marwes force-pushed the upgrade_to_tokio branch 3 times, most recently from b148842 to 14e9c08 Compare

April 2, 2018 13:53

Contributor Author

Marwes commented Apr 2, 2018

@carllerche Sorry about the rustfmting, I broke it out to a separate PR #32 . Diff without that commit at https://github.com/tokio-rs/tokio-uds/pull/29/files/c6826ec86c5b6f6f831516ba044521ac52190900..14e9c084691cd6ece741ecc8d0adc676a23b3c3e .

arcnmx commented Apr 13, 2018 •

edited

Loading

The tokio-reactor = { features=["unstable-futures"] } bit seems wrong, it should be set conditionally by the feature unstable-futures = ["futures-core", "futures-io", "futures-sink", "tokio-reactor/unstable-futures"] I believe?

Also the futures-* deps can probably be updated to final 0.2.0 versions, which are now released. EDIT: erm maybe not since futures2 still depends on the beta? seems like the futures 0.2 stuff is still in flux. It fails to compile currently for that reason, unless the futures-* deps are changed to version requirement "=0.2.0-beta".

EDIT: This commit was necessary to get it compiling.

arcnmx mentioned this pull request

Async UnixStream::connect #35

Closed

Marwes and others added 3 commits

April 20, 2018 16:34


          Run rustfmt

9a0d8fa


          Upgrade tokio_core dependency to use tokio 0.1

f2ad0e0

tokio-rs/tokio-rfcs#2


          feat: Implement futures-0.2 support

3c86d22

tokio-rs/tokio-rfcs#2

Marwes mentioned this pull request

Upgrade tokio_core dependency to use tokio 0.1 #36

Merged

Marwes force-pushed the upgrade_to_tokio branch from 978731b to 3c86d22 Compare

April 20, 2018 14:51

Contributor Author

Marwes commented Apr 20, 2018

Broke out the tokio 0.1 support into #36.

Marwes changed the title ~~Upgrade tokio_core dependency to use tokio 0.1~~ feat: Implement futures-0.2 support

Member

carllerche commented May 11, 2018

I'm going to close this as most of the important work has been landed via other PRs. Thanks again!

carllerche closed this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet