Add an async interface using futures-rs #141

Marwes · 2018-01-22T17:09:31Z

This adds support for communicating with redis instances asynchronously using futures and tokio. Putting this up now to show the changes and additions I have done so far.

Since the current response parser is tied to reading synchronously from a Read instance I rewrote it using https://github.com/Marwes/combine. I choose combine since I know it (obviously) and I wanted to try the state machine support I have been working on in tandem with this PR (this PR is what made me come up with the idea). Though I am a bit biased I do think it should be a good fit after I have worked out the bugs so I hope the change is acceptable. (I did see the Nom parser in #129 but didn't use it as I'd still need to code the state machine manually whereas combine creates it automatically).

query(_async) is currently the only added API and there is also a bunch of cleanup to be done.

Closes #103

Marwes · 2018-01-31T16:15:53Z

This should be "done" now. Only the basic query_async functions were added but everything else could be done independently of that.

softprops · 2018-02-02T06:40:06Z

Awesome. So happy to see this moving forward

mitsuhiko · 2018-02-02T08:04:58Z

Thanks! I will be playing with this.

mitsuhiko · 2018-02-02T08:15:05Z

Really nice. This makes me very happy :) I would merge it as such and then fix up some smaller things such as missing documentation on master. I would not waste too much time on that yet in case it turns out we need to tweak the API more.

Would just need a release of combine before I can merge this.

mitsuhiko

Only thing that needs changing before merging from my part is that combine points to a release.

badboy · 2018-02-02T08:50:32Z

👍 so good to see work here happening. I’ll look over the code today as well.

Marwes · 2018-02-02T09:34:25Z

src/connection.rs

+    pub trait ConnectionLike: Sized {
+        /// Sends an already encoded (packed) command into the TCP socket and
+        /// reads the single response from it.
+        fn req_packed_command(self, cmd: Vec<u8>) -> RedisFuture<(Self, Value)>;


I may have stuck a bit to close to the synchronous versions by using this pattern actually. I will be making a separate PR (building on this one) hopefully today where the Connection allows multiple commands to be in flight concurrently without needing an explicit pipeline. The trade off is that it won't be able to handle explicitly built pipelines (at least not initially) and there is some additional overhead due to needing Arc/Rc.

Marwes · 2018-02-02T10:16:25Z

src/parser.rs

-        match pieces.next() {
-            Some(detail) => fail!((kind, desc, detail.to_string())),
-            None => fail!((kind, desc)),
+        match parser.poll()? {


I should mention that instead of calling the asynchronous parsing here, we could parse directly from the Read instance. The only drawback is that the parser needs to be instantiated twice (once for I == PartialStream<easy::Stream<&[u8]>> and once for I == ReadStream<R>.

Marwes · 2018-02-02T10:18:15Z

src/script.rs

-                keys: vec![],
-            }
-            .invoke(con)
+            script: self,


Sorry for the churn caused by accidentally having rustfmt on. I could revert some of the changes If you want but I fear the files I modified may have to stick. (Would be nice to run through rustfmt on the whole repo though in my opinion).

@Marwes i'm fine enforcing rustfmt. I tried to do it in the past but it completely broke with some of the macros at the point. Need to reinvestigate this.

Marwes · 2018-02-02T10:21:13Z

Would just need a release of combine before I can merge this.

I should release 3.0.0-beta.1 (beta == only minor breaking changes planned) later today unless I get caught up in something and don't have time.

badboy · 2018-02-02T11:56:05Z

src/connection.rs

@@ -622,3 +621,243 @@ pub fn transaction<K: ToRedisArgs,
        }
    }
 }
+
+pub mod async {


The file is already quite long and here we have another module.
IMO this should be extracted into its own file here.

Just wanted to say. Also makes reading the diff hard because the types have the same name :)

I could change to AsyncConnection etc but I feel it is more idiomatic to write async::Connection to avoid ambiguities in that case.

badboy · 2018-02-02T11:57:44Z

src/lib.rs

+pub use types::{/* utility functions */ from_redis_value, /* error kinds */ ErrorKind,
+                /* conversion traits */ FromRedisValue, /* utility types */ InfoDict,
+                NumericBehavior, /* error and result types */ RedisError, RedisResult,
+                ToRedisArgs, /* low level values */ Value, RedisFuture};


This auto-formatted change makes it both harder to read and to change.
IMO we should keep the old formatting and mark it so rustfmt does not touch it.

Marwes · 2018-02-02T14:29:24Z

src/async.rs

@@ -0,0 +1,236 @@
+use std::fmt::Arguments;


Would it be better to have connection::sync and connection::async modules?

As long as we still export connection::sync::Connection (and other types from that module) on the root module, we could split it up and not break existing usage.

@badboy we never exposed modules anyways. Only the toplevel thing.

@mitsuhiko Exactly, that's why I just wanted to mention that we re-export it to keep it working

I left it under src/async.rs since I would need to re-export it as async under the root anyway.

gsquire · 2018-02-02T23:21:16Z

I guess I can close my nom attempt which is a good thing!

badboy · 2018-02-03T12:44:40Z

@gsquire Thanks for your effort anyway!

Marwes · 2018-02-04T09:03:15Z

Released a new version of combine and updated this PR to point to it https://crates.io/crates/combine

Marwes · 2018-02-12T11:39:35Z

Is there anything blocking this from being merged?

badboy · 2018-02-12T11:47:40Z

IMO 2 things:

Documentation. We have usage examples in the root for the sync interface. Now that we have an async module, we should put some examples on how to use it there
Stable release of combine v3. I don't think it would be good to rely on the beta release.

Marwes · 2018-05-24T17:07:10Z

Did a rebase and squash since most of the commits won't compile due to depending on dead git branches. Still lacking unix stream support due to tokio-uds not having a new release but that can be added in without a breaking release at a later point.

Marwes · 2018-05-28T13:58:53Z

I believe all remaining issues have been resolved(?) (pointing combine to a real 3.0 release was the last one). cc @mitsuhiko @badboy

jwilm · 2018-05-29T16:56:58Z

Did a rebase and squash since most of the commits won't compile due to depending on dead git branches. Still lacking unix stream support due to tokio-uds not having a new release but that can be added in without a breaking release at a later point.

I haven't been following closely, but does that mean that Unix stream support is broken for the sync interface as well? We rely on this in our production environment and would need to pin to an earlier commit.

Marwes · 2018-05-29T22:03:26Z

I haven't been following closely, but does that mean that Unix stream support is broken for the sync interface as well? We rely on this in our production environment and would need to pin to an earlier commit.

No, that only affects the async interface (they are largely separate implementations in that respect)

jwilm · 2018-05-29T23:37:28Z

No, that only affects the async interface (they are largely separate implementations in that respect)

👍 Thanks for clearing that up. I recall some discussion around having the implementations be unified, and it's good to hear there won't be any breaking changes due to this feature on the sync side. At least, until they unify :).

Marwes · 2018-06-05T09:12:04Z

Bump (@mitsuhiko @badboy )

Marwes · 2018-06-18T13:45:40Z

Ping @mitsuhiko and @badboy . Anything I can do to help get this and #143 merged?

badboy · 2018-06-18T13:52:09Z

I'm just back from a work week. I'll put this on my schedule for the week.

badboy

Good job!
The rustfmt made it a bit harder to review, but I'm fine with it being done now and not having to deal with it later.
@mitsuhiko's "Request changes" should be resolved.

Some small things remaining and one last question:

Is it likely that we can switch away from a boxed future in the future (pun intended) by using -> impl X? (No need to do it here now)

Furthermore, we should at least run cargo bench --no-run on CI with nightly to ensure the code actually compiles. That can be done in another bug.

badboy · 2018-06-22T19:09:08Z

Cargo.toml

@@ -19,18 +19,29 @@ build = "build.rs"
 [features]
 with-rustc-json = ["rustc-serialize"]
 with-unix-sockets = ["unix_socket"]
+# with-async-unix-sockets = ["tokio-uds"]


Please remove this comment.

badboy · 2018-06-22T19:09:16Z

Cargo.toml

+tokio-reactor = "0.1"
+tokio-tcp = "0.1"
+tokio-io = "0.1"
+# tokio-uds = { git = "https://github.com/Marwes/tokio-uds", branch = "only_tokio_upgrade", optional = true }


Also this comment.

badboy · 2018-06-22T19:23:15Z

benches/bench_basic.rs

+extern crate tokio_core;
+
+use futures::Future;
+use tokio_core::reactor::Core;


tokio_core is not in the dependencies anymore. This whole benchmark needs a rewrite.

badboy · 2018-06-22T19:23:55Z

benches/bench_basic.rs

+fn bench_simple_getsetdel_async(b: &mut Bencher) {
+    let client = get_client();
+    let mut core = Core::new().unwrap();
+    let con = client.get_async_connection(&core.handle());


get_async_connection doesn't take a handle anymore, so I guess this all needs to change.

Fixed (and I enabled cargo check for travis)

badboy · 2018-06-22T19:24:26Z

benches/bench_basic.rs

    bench_simple_getsetdel_pipeline,
    bench_simple_getsetdel_pipeline_precreated,
    bench_long_pipeline,
    bench_encode_pipeline,
-    bench_encode_pipeline_nested
+    bench_encode_pipeline_nested,
+    bench_long_pipeline


This is now duplicated.

badboy · 2018-06-22T19:27:33Z

src/async.rs

+                    .map(|con| ActualConnection::Tcp(BufReader::new(con))),
+            )
+        }
+        #[cfg(feature = "with-async-unix-sockets")]


It's weird seeing this feature, when it's not available in Cargo.toml.
What do you think about extracting the async-unix-socket code from this PR, but immediately opening one adding it, pending the updated tokio-uds crate?

tokio-uds actually got a release so I removed the feature and enable tokio-uds support with the normal uds feature https://crates.io/crates/tokio-uds

badboy · 2018-06-22T19:36:05Z

src/parser.rs

-            Some(detail) => fail!((kind, desc, detail.to_string())),
-            None => fail!((kind, desc)),
+        match parser.poll()? {
+            Async::NotReady => panic!("Blocking read received WouldBlock error"),


Seeing a panic makes me kinda worry:
Is it likely that code is ever hit? Is it an actual error that can't be handled at all?

Changed it to return WouldBlock again which is the only reason why it could happen.

badboy · 2018-06-22T19:37:31Z

tests/parser.rs

+extern crate futures;
+extern crate partial_io;
+#[macro_use]
+extern crate quickcheck;


Yey for seeing quickcheck used here.

…ync Read instance

Version 0.2 were released a few weeks ago so we might as well enable it immediately

Marwes · 2018-06-25T15:36:52Z

The rustfmt made it a bit harder to review, but I'm fine with it being done now and not having to deal with it later.

Sorry about that. I think I had it split up before but I ended up with a nasty rebase and had to remove it to make it easier. Tried to break it up into two commits again but looked pretty nasty still.

Is it likely that we can switch away from a boxed future in the future (pun intended) by using -> impl X? (No need to do it here now)

Not in the trait methods which return a future, but in all the other functions then certainly.

Furthermore, we should at least run cargo bench --no-run on CI with nightly to ensure the code actually compiles. That can be done in another bug.

Did it as cargo check --benches which I think should do basically the same but might be a bit faster.

Marwes · 2018-06-25T15:39:50Z

@mitsuhiko's "Request changes" should be resolved.

I think I resolved everything (can't find anything I missed at least)?

Now uses combine v3.0.0 (stable release)

badboy · 2018-06-26T09:18:34Z

Hmmm, the tests fail on Windows:

	thread 'test_args' panicked at 'called `Result::unwrap()` on an `Err` value: An invalid argument was supplied. (os error 10022)', libcore\result.rs:945:5

	thread 'test_pipeline_transaction' panicked at 'called `Result::unwrap()` on an `Err` value: An invalid argument was supplied. (os error 10022)', libcore\result.rs:945:5

Marwes · 2018-06-26T09:41:07Z

Kicking of a println test on appveyor to see if it is an easy fix, otherwise I will try and get this work locally on windows tonight.

badboy · 2018-06-26T10:52:43Z

@Marwes Just ping me again if this is resolved. I'm happy to merge it then :)

These are rarely necessary and should hopefully fix the issue on windows since a rather weird use of net2 could be removed

Marwes · 2018-06-26T11:06:21Z

@badboy I made some mistake when using the net2 crate to connect to tcp using an explicit tokio handle. Since an explicit handle should rarely (if ever) be necessary I just removed the use of an explicit handle which made it work.

If someone ends up needing a function that takes an explicit handle we can revisit this and try to implement it then instead.

Everything is green now!

badboy · 2018-06-26T12:11:41Z

Great! Merging it now!

Terkwood · 2018-06-26T13:34:59Z

Great work! Looking forward to trying this out!

Marwes mentioned this pull request Jan 24, 2018

feat: Encode parsers as resumable state machines Marwes/combine#128

Merged

Marwes force-pushed the async branch 5 times, most recently from d7fe429 to c9fba9e Compare January 30, 2018 11:32

Marwes changed the title ~~[WIP] Add an async interface using futures-rs~~ Add an async interface using futures-rs Jan 31, 2018

Marwes closed this Jan 31, 2018

Marwes reopened this Jan 31, 2018

mitsuhiko previously requested changes Feb 2, 2018

View reviewed changes

Marwes commented Feb 2, 2018

View reviewed changes

badboy reviewed Feb 2, 2018

View reviewed changes

Marwes commented Feb 2, 2018

View reviewed changes

Marwes mentioned this pull request Feb 2, 2018

Allow the async connection to have multiple in flight requests #143

Merged

Marwes force-pushed the async branch from f09c017 to 6daf780 Compare February 4, 2018 08:57

Marwes force-pushed the async branch from 9c2997b to f6fcdcd Compare February 12, 2018 12:22

badboy requested changes Jun 22, 2018

View reviewed changes

Markus Westerlind and others added 6 commits June 25, 2018 17:24

Return WouldBlock instead of panicking for synchronous reads on an as…

6282074

…ync Read instance

Re-enable tokio_uds

b0d81d5

Version 0.2 were released a few weeks ago so we might as well enable it immediately

Remove duplicate benchmark

0c91377

Fix the benchmark so it uses tokio

2b16bb7

Check that the benchmarks compiles on travis

ad82c60

Merge branch 'master' into async

ca44349

badboy approved these changes Jun 26, 2018

View reviewed changes

Remove the functions taking an explicit Handle

e3180ea

These are rarely necessary and should hopefully fix the issue on windows since a rather weird use of net2 could be removed

Marwes force-pushed the async branch from 579f9e5 to e3180ea Compare June 26, 2018 10:53

badboy merged commit 0d6c726 into redis-rs:master Jun 26, 2018

Marwes deleted the async branch June 26, 2018 12:37

badboy mentioned this pull request Jan 28, 2019

Increase performance #151

Closed

Add an async interface using futures-rs #141

Add an async interface using futures-rs #141

Conversation

Marwes commented Jan 22, 2018

Marwes commented Jan 31, 2018

softprops commented Feb 2, 2018

mitsuhiko commented Feb 2, 2018

mitsuhiko commented Feb 2, 2018

mitsuhiko left a comment

Choose a reason for hiding this comment

badboy commented Feb 2, 2018

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

Marwes commented Feb 2, 2018

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

gsquire commented Feb 2, 2018

badboy commented Feb 3, 2018

Marwes commented Feb 4, 2018

Marwes commented Feb 12, 2018

badboy commented Feb 12, 2018

Marwes commented May 24, 2018

Marwes commented May 28, 2018

jwilm commented May 29, 2018

Marwes commented May 29, 2018

jwilm commented May 29, 2018

Marwes commented Jun 5, 2018

Marwes commented Jun 18, 2018

badboy commented Jun 18, 2018

badboy 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

Choose a reason for hiding this comment

Marwes commented Jun 25, 2018

Marwes commented Jun 25, 2018

badboy commented Jun 26, 2018

Marwes commented Jun 26, 2018 • edited

badboy commented Jun 26, 2018

Marwes commented Jun 26, 2018

badboy commented Jun 26, 2018

Terkwood commented Jun 26, 2018

Marwes commented Jun 26, 2018 •

edited