refactor(lib): convert to futures 0.2.0-beta #1470

srijs · 2018-03-20T09:55:12Z

Noteworthy changes:

Removed the dependency on tokio-service, instead copied over the Service and NewService traits for now. I looked into tower, but they're still on futures 0.1, and also made some changes to the Service trait.
Not specifying an executor when creating a client will now cause a single background thread to be spawned to handle connection-related tasks. This change was needed since we couldn't rely on the current executor for Pool::spawn_expired_interval, but needed a Send-able executor instead.
The logic in proto::h1::io to detect vectored I/O needed to adapt to changes in the AsyncWrite trait, I removed WriteBufAuto and am now relying on the result of poll_vectored_write instead.

Aside from that, relatively straight-forward changes. Oh, and of course...

Blockers to merging:

seanmonstar

Wow! Thanks for working on this big refactor!

Besides the comments I've left inline, one thing I wonder about is depending on the unstable-futures feature of tokio. It's not clear to me that tokio plans to treat that feature with semver, so it might not be something to depend on directly.

I started on a futures-compat crate, to allow people to use libraries that haven't upgraded yet. I wonder if we make use of that until tokio's support is no longer unstable?

seanmonstar · 2018-03-20T18:44:47Z

src/client/conn.rs

@@ -186,7 +187,7 @@ where
    pub fn send_request(&mut self, req: Request<B>) -> ResponseFuture {
        let inner = match self.dispatch.send(req) {
            Ok(rx) => {
-                Either::A(rx.then(move |res| {
+                Either::Left(rx.then(move |res| {


I think this can be made slightly nicer in 0.2 with FutureExt::left and FutureExt::right, meaning we no longer need to import Either, ya?

Looks like it, yeah!

Use .left() and .right() combinators

seanmonstar · 2018-03-20T18:46:54Z

src/client/connect.rs

                    }
                },
                State::Resolving(ref mut future) => {
-                    match try!(future.poll()) {
-                        Async::NotReady => return Ok(Async::NotReady),
+                    match try!(future.poll(cx)) {


Hadn't noticed before, this could just be try_ready!, huh? Don't need to fix, but if you wanted 😉

No worries, I'll fix it up!

use try_ready! instead of explicit match

seanmonstar · 2018-03-20T18:50:24Z

src/client/mod.rs

                    .and_then(move |(io, connected)| {
                        conn::Builder::new()
                            .h1_writev(h1_writev)
                            .handshake_no_upgrades(io)
                            .and_then(move |(tx, conn)| {
-                                executor.execute(conn.map_err(|e| debug!("client connection error: {}", e)))?;
+                                executor.execute(conn.then(|result| {


I think there's no FutureExt::recover for this pattern, right?

Oh, neat!

use recover

seanmonstar · 2018-03-20T18:51:32Z

src/client/mod.rs

+            // XXX: should wait on the Checkout? Problem is
+            // that if the connector is failing, it may be that we
+            // never had a pooled stream at all
+            ClientError::Normal(either.either(|(e, _)| e, |(e, _)| e))


This pattern is kind of confusing, could be fixed later though.

Yeah, Either used to have this useful split, that's gone away in futures 0.2 unfortunately.

Maybe just replace it with a match?

We can change this to use Either::factor_first once rayon-rs/either#19 is merged!

seanmonstar · 2018-03-20T18:54:33Z

src/client/mod.rs

+                    //
+                    // If the executor doesn't have room, oh well. Things will likely
+                    // be blowing up soon, but this specific task isn't required.
+                    let _ = executor.execute(future::poll_fn(move |cx| {


The check of poll_ready before spawning on the executor was to try to let the connection be available for reuse earlier, which was noticeable when using tokio's runtime, since the task might end up on another thread and thus insert back into the pool after the user got a finished response.

If the problem was calling poll_ inside a combinator, I've been meaning to add a pub(crate) fn is_ready to SendRequest, that would simply check wants atomic state without parking.

I'll look into that!

look into adding SendRequest::is_ready method

Luckily my PR to add a Context arg to future::lazy just landed in time for futures 0.2 beta, so I fixed it using that instead!

seanmonstar · 2018-03-20T19:16:24Z

src/proto/h1/io.rs

+            // If there's more than one IoVec available, attempt to
+            // determine the best buffering strategy based on whether
+            // the underlying AsyncWrite object supports vectored I/O.
+            if n == bufs[0].len() {


Yea, this is how I'd probably do this too. 👍

seanmonstar · 2018-03-20T19:20:59Z

src/proto/h1/io.rs

        } else {
            loop {
-                let n = try_ready!(self.io.write_buf(&mut self.write_buf.auto()));
+                let (n, strategy) = try_ready!(write_buf(&mut self.io, cx, &mut self.write_buf));
+                if self.write_buf.strategy == Strategy::Auto {


I think this can still be done with WriteBufAuto, that allows only doing the strategy checking as long as it's still Auto.

Can you go into more detail about that? Why would we need WriteBufAuto?

I mostly was thinking of these reasons:

Allows the strategy swapping code to be separate from this, which is just wanting to write until blocked.

The checks to determine what strategy to switch to could probably only have to happen when strategy it still auto.

Ah, cool! I can definitely factor out the strategy swapping code, I guess WriteBufAuto would now implement some sort of write_to method that takes a AsyncWrite?

factor out strategy swapping code

seanmonstar · 2018-03-20T19:23:42Z

src/server/mod.rs

-                .map_err(move |err| error!("server connection error: ({}) {}", addr, err));
-            spawn(fut);
-            Ok(())
+            let fut = protocol.serve_connection(socket, s).then(move |result| {


I think this can be FutureExt::recover also.

Will do!

use recover

seanmonstar · 2018-03-20T19:26:52Z

src/server/mod.rs

-            let service = new_service.new_service()?;
+            let service = match new_service.new_service() {
+                Ok(service) => service,
+                Err(err) => return future::Either::Left(future::err(err))


Why does this return Either now? It's not immediately clear to me.

It returns Either because further down we return the Spawn future from the same function. The Spawn future is lazy and needs to be polled to spawn the future (since it's using the executor from the context to spawn).

Oh, weird! (That spawn API feels a little odd, but not your fault :))

change to use .left() and .right()

seanmonstar · 2018-03-20T19:34:54Z

src/lib.rs

@@ -18,7 +18,7 @@

 extern crate bytes;
 #[macro_use] extern crate futures;


I think there's a recommendation that libraries shouldn't depend on futures directly, but rather on the individual crates (like futures-core, futures-channel, etc). The idea is that hyper could keep working if some extension needed a breaking change, and we only ever publicly claimed to depend on futures-core...

It'd be good to take inventory of what public dependencies we may be exposing here, either directly or indirectly:

futures-core since things like FutureResponse implement Future.

futures-io since we accept AsyncRead and AsyncWrite...

I think all usage of futures-channel is no longer publicly exposed, so not a public dependency, right?

While we use FutureExt internally, any types returned from it aren't viewable from outside the crate, I think.

Good points! We have the same situation with tokio, where I was gonna fix that up in another PR.

Your call whether to do it as part of this PR, or whether I should follow up with a combined futures and tokio PR for it!

srijs · 2018-03-20T21:04:10Z

I'll take a look at futures-compat, thanks! One thing is that we'll also need is compat between tokio::io::Async{Read, Write} and futures::io::Async{Read, Write}. Would you consider that in-scope for futures-compat, or would that live somewhere else?

On the other hand, I would expect tokio to drop this feature and go all-in on 0.2 when it's released? Or is your worry about breaking hyper master in the meantime?

I'll look into the build errors tomorrow if you don't beat me to it!

aturon · 2018-03-20T22:32:17Z

@srijs @seanmonstar

The unstable bit is just waiting until futures 0.2.0 is published. We're putting out a beta this afternoon and otherwise are pretty much ready to go. So I would advise not investing time working around that flag.

srijs · 2018-03-21T05:52:08Z

Addressed a bunch of stuff, as well as fixed everything to work with the futures 0.2 beta release!

@seanmonstar I was fighting the conn_drop_prevents_pool_checkout test this afternoon because it was flakey. The reason (iiuc) was a race condition between the tokio runtime shutting down and the client sending the second request (and by extension the server thread reading from the connection). I've made some changes that I think are okay, to make the test run deterministically, but I'd appreciate you taking a look and telling me if the changes are not okay.

srijs · 2018-03-21T10:15:51Z

It's not immediately apparent to me why the 2/4 AppVeyor builds failed. Any pointers?

seanmonstar · 2018-03-21T21:11:48Z

It's not immediately apparent to me why the 2/4 AppVeyor builds failed. Any pointers?

Hm, when it just exists like that with an error code, my assumption is that the process was aborted (either a double panic, or segfault). I'll restart and see poke around.

seanmonstar · 2018-03-21T21:47:26Z

So, searching for the exit code, its a segfault basically. Derefencing a bad pointer. This might be fixed in tokio-rs/tokio#243

srijs · 2018-03-22T07:34:38Z

Still seeing issues while using Carl's races branch. Commented on the PR in tokio.

srijs · 2018-03-23T09:34:11Z

@seanmonstar Nice work finding that use-after-free bug in tokio! 👏

I've targeted your branch with the fix, and at least locally the tests are running reliably now. Pushed the changes to also check with Travis and AppVeyor.

srijs · 2018-03-23T22:01:42Z

@seanmonstar Addressed your feedback so far, I think this should be ready for a second round of review!

Tests run smooth on OS X and Linux, however the AppVeyor builds seem to hang occasionally.

The last thread of discussion that's still open was around whether to split the futures and tokio dependencies into dependencies on their subcrates. I'm happy to do it in this PR if you want me to, otherwise I can follow up with another PR once tokio and futures stabilise a bit.

seanmonstar

\o/

seanmonstar · 2018-03-24T00:35:13Z

src/client/dns.rs

+                .map(|i| IpAddrs { iter: i });
+            sender.send(result).ok();
+            Ok(())
+        }))).unwrap();


It looks like the receiver will return an error if the sender was dropped, so we should be able to just ignore this spawn error, right?

Good point!

ignore dns spawn error

seanmonstar · 2018-03-24T00:44:58Z

src/client/mod.rs

@@ -85,9 +86,9 @@ impl<C, B> Client<C, B> {
    fn configured(config: Config<C, B>, exec: Exec) -> Client<C, B> {
        Client {
            connector: Arc::new(config.connector),
-            executor: exec,
+            executor: exec.clone(),


We should be able to completely remove the executor field of the client, and always use cx.

(HttpConnector should still own its own executor, but that's because it needs to spawn blocking tasks. It should be enough to allow configuring the HttpConnector manually.)

remove custom executor

srijs · 2018-03-24T06:24:35Z

All done! I'm using my branch of futures momentarily until rust-lang/futures-rs#902 is merged in some form.

seanmonstar · 2018-03-26T21:39:05Z

@srijs Awesome work! I started a checklist in the original description to track what is blocking merging this. I realized I didn't know all of what that is, if you'd like to add to this list...

srijs · 2018-03-27T01:19:10Z

@seanmonstar Good stuff! I added 2 PRs that we should get merged, one of them is seanmonstar/want#1.

As for other merge blockers, would you consider anything that needs a release (and is sourced from git right now) a blocker, or is that okay to fix up later as the new versions get published?

seanmonstar · 2018-03-29T17:47:09Z

OK, so I'm thinking: screw it, we just merge this, and track that other stuff in new issues. Having it stagnate here doesn't help, and prevents working on newer things that need these changes. So, unless there's objections, I'll just merge as-is in a couple hours :D

This reverts commit a12f7be. Much sadness 😢.

xrl · 2018-06-09T19:42:39Z

@seanmonstar @srijs why was this work reverted?

seanmonstar · 2018-06-09T19:49:14Z

@xrl see here for why: #1481

srijs mentioned this pull request Mar 20, 2018

Update to futures 0.2 #1448

Closed

seanmonstar reviewed Mar 20, 2018

View reviewed changes

srijs force-pushed the futures-0.2 branch 2 times, most recently from 6f0c507 to 77aaa89 Compare March 21, 2018 05:50

srijs changed the title ~~refactor(lib): convert to futures 0.2.0-alpha~~ refactor(lib): convert to futures 0.2.0-beta Mar 21, 2018

srijs force-pushed the futures-0.2 branch from 77aaa89 to d3886f4 Compare March 22, 2018 07:18

srijs force-pushed the futures-0.2 branch from d3886f4 to 4504b13 Compare March 23, 2018 09:32

srijs force-pushed the futures-0.2 branch from 4504b13 to 0ae4c6e Compare March 23, 2018 21:56

srijs force-pushed the futures-0.2 branch from 0ae4c6e to 7b1d48f Compare March 23, 2018 22:15

seanmonstar reviewed Mar 24, 2018

View reviewed changes

srijs force-pushed the futures-0.2 branch from 7b1d48f to 87bea01 Compare March 24, 2018 06:23

srijs force-pushed the futures-0.2 branch from 87bea01 to ce92cc5 Compare March 24, 2018 06:52

refactor(lib): convert to futures 0.2.0-beta

a5e76ff

srijs force-pushed the futures-0.2 branch from ce92cc5 to a5e76ff Compare March 24, 2018 06:58

seanmonstar merged commit a12f7be into hyperium:master Mar 29, 2018

srijs deleted the futures-0.2 branch March 30, 2018 00:35

seanmonstar added a commit that referenced this pull request Apr 7, 2018

Revert "refactor(lib): convert to futures 0.2.0-beta (#1470)"

cc210d0

This reverts commit a12f7be. Much sadness 😢.

seanmonstar added a commit that referenced this pull request Apr 10, 2018

Revert "refactor(lib): convert to futures 0.2.0-beta (#1470)"

625e4da

This reverts commit a12f7be. Much sadness 😢.

		@@ -18,7 +18,7 @@

		extern crate bytes;
		#[macro_use] extern crate futures;

refactor(lib): convert to futures 0.2.0-beta #1470

refactor(lib): convert to futures 0.2.0-beta #1470

Conversation

srijs commented Mar 20, 2018 • edited Loading

seanmonstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 20, 2018 • edited Loading

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

srijs Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

srijs commented Mar 20, 2018 • edited Loading

aturon commented Mar 20, 2018

srijs commented Mar 21, 2018 • edited Loading

srijs commented Mar 21, 2018

seanmonstar commented Mar 21, 2018

seanmonstar commented Mar 21, 2018

srijs commented Mar 22, 2018 • edited Loading

srijs commented Mar 23, 2018 • edited Loading

srijs commented Mar 23, 2018 • edited Loading

seanmonstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijs Mar 24, 2018 • edited Loading

Choose a reason for hiding this comment

srijs commented Mar 24, 2018 • edited Loading

seanmonstar commented Mar 26, 2018

srijs commented Mar 27, 2018 • edited Loading

seanmonstar commented Mar 29, 2018

xrl commented Jun 9, 2018

seanmonstar commented Jun 9, 2018

srijs commented Mar 20, 2018 •

edited

Loading

srijs Mar 20, 2018 •

edited

Loading

srijs Mar 20, 2018 •

edited

Loading

srijs Mar 20, 2018 •

edited

Loading

srijs Mar 24, 2018 •

edited

Loading

srijs Mar 20, 2018 •

edited

Loading

srijs Mar 21, 2018 •

edited

Loading

srijs Mar 20, 2018 •

edited

Loading

srijs Mar 20, 2018 •

edited

Loading

srijs Mar 21, 2018 •

edited

Loading

srijs Mar 20, 2018 •

edited

Loading

srijs commented Mar 20, 2018 •

edited

Loading

srijs commented Mar 21, 2018 •

edited

Loading

srijs commented Mar 22, 2018 •

edited

Loading

srijs commented Mar 23, 2018 •

edited

Loading

srijs commented Mar 23, 2018 •

edited

Loading

srijs Mar 24, 2018 •

edited

Loading

srijs Mar 24, 2018 •

edited

Loading

srijs commented Mar 24, 2018 •

edited

Loading

srijs commented Mar 27, 2018 •

edited

Loading