convert to use tokio 0.1

[srijs]

All the tests pass! 🎉 (at least locally -- let's see what CI says)

Spend this morning rebasing due to 0.12.x moving so fast!

Fixes #1443

[seanmonstar]

Woo! Nice! (And sorry about 0.12.x, I had been rebasing master onto it for a while, assuming most people weren't actually depending on its history 😭 ).

[srijs]

No worries, I was just surprised by how quickly it had moved since I started a couple of days ago :)

One thing to note here is that I've tried to keep the API mostly the same (biggest exception being Server::run). It sounded like it would be a good idea to postpone improving the API until later, after the "raw" tokio integration landed. Let me know if you feel different!

[seanmonstar]

This is fantastic! 🎉 So much goodness!

Looks like there's a hanging test, client::tests::retryable_request...

[seanmonstar]

All the lines printing about using 1 thread can probably be adjusted now :D

[srijs]

Heh, will do!

[seanmonstar]

I'm curious what the lazy usage is for. Is that because calling all this code outside of tokio::run will panic?

[srijs]

Yeah, most constructors including Client::default now need a thread-local handle. When we change the API I think we should probably defer most or all I/O (like port binding) until the future is polled, but for now that's the compromise I made.

[srijs]

The hanging test is a weird one, it works on my machine (OS X), so I can't reproduce at the moment. I'll grab an AWS instance later to try and reproduce on Linux.

[seanmonstar]

I'll try to rent an AWS instance later to try and reproduce on Linux.

Don't bother! I can debug it myself.

[srijs]

Thanks, much appreciated!

In other news, my PR to split off tokio-tcp (tokio-rs/tokio#224) was merged, so we can hopefully drop the direct dependency on tokio soon-ish

[seanmonstar]

For the hanging test, it's a combination of mutexes in the mock duplex code, and using the ThreadPool executor. The hang went away when I made sure that the task.notify() and the read or write happened without releasing the lock. But the test fails in an error since the tasks are on separate threads, but with only 1 core and mutexes, the code can't happen in the order that is being tested.

The test is specifically designed to be a series of events we can control, because its testing those series of events. I suspect that switching to the tokio's current_thread executor for that test will fix it.

[srijs]

Hmm, so the issue with current_thread is that it's not Send nor Sync, making it unsuitable for a custom executor.

The only way I can think of solving that problem is to make it optional for Client to be Send, which would probably require tracking that as type state, e.g. Client<IsSend> vs Client<IsNotSend> or making the executor a type parameter itself, e.g. Client<TaskExecutor>.

Maybe there is another way to fix the tests so they could run on a multi-threaded executor?

[srijs]

Okay, so I think I fixed the initial problem by using a thread pool of size 1 as an executor. There are some other failures now on Travis that I still can't reproduce locally :/

Any chance you could have another look?

[seanmonstar]

I'll look into that test failure. Besides that, the benches don't compile XD.

[seanmonstar]

So the failing keepalive client test is because of tasks being on separate threads, but with only 1 CPU, the order isn't preserved. Well, on one hand, it's good to find poorly written tests that break on multithreaded!

The test itself is ensuring that after we get a response back, a second request to same host should reuse the existing connection. However, knowing that the connection can be reused requires the Dispatch task to do some housekeeping after submitting the response back to the FutureResponse task. So, by the time the main thread has the response and starts a new request, the Dispatch task on the other thread hasn't had a chance to cleanup state and set itself ready for a new request, so it isn't back in the client's pool.

As a hack, we could just sleep the main thread for a few milliseconds, allowing the dispatch task to clean up...

Perhaps in a separate issue, the design of that can be changed, since changing this test definitely means that users may see on occasion requests not reusing a connection it could have.

[srijs]

Thanks for providing more info on that test! I thought I fixed that particular test earlier by introducing exactly the sort of thread::sleep that you suggested.

The latest Travis run showed two other tests failing instead: one with a ‚resource temporarily not available‘ error, and the other one (a dispatch test) timing out.

I‘ll try to fix the benches later!

[seanmonstar]

I've got the benches passing, and fixed up that test, in https://github.com/hyperium/hyper/tree/new-tokio. I've been pushing to that to trigger CI, and looking at some panics.

Seems one was due to a race condition in the futures mpsc channel, so I've a work around there. Waiting for CI again...

[seanmonstar]

Yay! CI is green! I'll see about merging this ... probably tomorrow, its getting late x_x

[srijs]

@seanmonstar Nice! I pulled your changes into this branch, just in case you had any other feedback you wanted to see addressed before merging.

AppVeyor builds seem to be failing both on this as well as your new-tokio branch, could that be related to our changes?

[seanmonstar]

The issue on Windows was that we needed to call bind on the TcpBuilder before connect. All good now. I also loosened the requirement of Entity, such that the trait itself doesn't require Send, only when used in Client and Server and thus interacts with an Executor. That way, if people want to work with non-Send things, they still can, using hyper::{client, server}::conn.

I merged those changes into your commit, and merged it into 0.12.x here: 603c3e4

Thank you sooo much!