Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-blocking/Evented I/O #395

Closed
jnicholls opened this issue Mar 24, 2015 · 127 comments
Closed

Non-blocking/Evented I/O #395

jnicholls opened this issue Mar 24, 2015 · 127 comments
Assignees
Milestone

Comments

@jnicholls
Copy link

Hyper would be far more powerful of a client & server if it was based on traditional event-oriented I/O, single-threaded or multi-threaded. You should look into https://github.com/carllerche/mio or a wrapper around libuv or something of that sort.

Another option is to split hyper up into multiple crates, and refactor the client & server to abstract the HTTP protocol handling (reading & writing requests/responses onto a Stream) so that someone can use the client and/or server logic on top of their own sockets/streams that are polled in an event loop. Think, libcurl's multi interface + libuv's uv_poll_t.

@seanmonstar
Copy link
Member

We agree. We're actively looking into it. Mio looks promising. We also need
a Windows library, and a wrapper combining the two.

On Tue, Mar 24, 2015, 6:06 AM Jarred Nicholls [email protected]
wrote:

Hyper would be far more powerful of a client & server if it was based on
traditional event-oriented I/O, single-threaded or multi-threaded. You
should look into https://github.com/carllerche/mio or a wrapper around
libuv or something of that sort.


Reply to this email directly or view it on GitHub
#395.

@hoxnox
Copy link

hoxnox commented Apr 23, 2015

Do you already have a vision how to embed mio into hyper? I'm very interested in async client and have enough time to contribute some code.

@seanmonstar
Copy link
Member

I don't have a vision; I haven't looked that hard into how mio works. I'd love to hear suggestions.

@seanmonstar seanmonstar modified the milestone: Rust 1.0 Apr 27, 2015
@seanmonstar seanmonstar modified the milestones: Rust 1.0, 1.0 May 6, 2015
@jnicholls
Copy link
Author

mio will be adding Windows support in the near future, so depending upon it should be a safe bet.

The API surface of hyper's server will not have to change much if at all, but the client will need an async interface, either in the form of closure callbacks, a trait handler, something like a Future or Promise return value, etc.

@dcsommer
Copy link

dcsommer commented Jul 3, 2015

+1 for prioritizing a trait handler. Futures have some amount of overhead, and closure callbacks have even more overhead and can lead to callback hell. If the goal is maximum performance, an async handler interface would be a natural starting point.

@jnicholls
Copy link
Author

Yeah honestly a trait handler with monomorphization/static dispatch is the only way to go.

@teburd
Copy link
Contributor

teburd commented Jul 10, 2015

+1 for the async handler Trait

@steveklabnik
Copy link

@talevy
Copy link

talevy commented Jul 25, 2015

This is very much premature, but figured any activity to this thread is a positive!

I have been playing around with what it would look like to write an asynchronous hyper client.

here it goes: https://github.com/talevy/tengas.

this has many things hardcoded, and is not "usable" by any means. Currently it does just enough
to get an event loop going and allows to do basic GET requests and handle the response within a callback function.

I tried to re-use as many components of hyper as possible. Seems to work!

I had to re-implement HttpStream to use mio's TcpStream instead of the standard one.

I plan on making this more generic and slowly match the original hyper client capabilities.

Any feedback is welcome! Code is a slight mess because it is the first pass at this to make it work.

@seanmonstar
Copy link
Member

I've been investigating mio support, and fitting it in was actually pretty simple (in a branch). I may continue the branch and include the support with a cargo feature flag, but I can't switch over completely until Windows support exists.

@jnicholls
Copy link
Author

A feature flag makes great sense in this case then. There are plenty of
people who would be able to take advantage of hyper + mio on *nix systems;
probably the vast majority of hyper users in fact.

On Sun, Jul 26, 2015 at 5:22 PM, Sean McArthur [email protected]
wrote:

I've been investigating mio support, and fitting it in was actually pretty
simple (in a branch). I may continue the branch and include the support
with a cargo feature flag, but I can't switch over completely until Windows
support exists.


Reply to this email directly or view it on GitHub
#395 (comment).

@jdm
Copy link

jdm commented Jul 27, 2015

Servo would be super interested in hyper + mio to reduce the thread bloat :)

@gobwas
Copy link

gobwas commented Jul 27, 2015

hyper + mio looks very promising =) 👍

@teburd
Copy link
Contributor

teburd commented Jul 27, 2015

I would assume there would be some number of threads with event loops handling http requests rather than one thread with one event loop?

@talevy
Copy link

talevy commented Jul 27, 2015

@seanmonstar is this branch public somewhere?

@seanmonstar
Copy link
Member

Not yet. It doesn't use an event loop yet, I simply switched out usage of
std::net with mio::tcp. Which works fine for small requests that don't
block...

On Mon, Jul 27, 2015, 8:56 AM Tal Levy [email protected] wrote:

@seanmonstar https://github.com/seanmonstar is this branch public
somewhere?


Reply to this email directly or view it on GitHub
#395 (comment).

@teburd
Copy link
Contributor

teburd commented Jul 27, 2015

If hyper can add that feature I'd basically consider it usable for myself in production, otherwise it would probably cause a great deal of thread context switching for my use case (lots and lots and lots of short lived connections)

@gobwas
Copy link

gobwas commented Aug 6, 2015

By my own benchmarks with lots of http connections, rust will be fastest way, if it will have async io:

@jnicholls
Copy link
Author

It is interesting how stable express, vanilla, and spray are in terms of
response times over time. I'm surprised nickel and iron are not equally as
stable; interestingly enough they both have the same shape, so my guess is
it's identical behavior on their primary dependency: hyper :)

On Thu, Aug 6, 2015 at 5:33 AM, Sergey Kamardin [email protected]
wrote:

By my own benchmarks with lots of http connections, rust will be fastest
way, if it will have async io:

https://camo.githubusercontent.com/698a9884f2e7734d4fd27b8af45a4b79ef06c3bd/68747470733a2f2f73332e616d617a6f6e6177732e636f6d2f662e636c2e6c792f6974656d732f30543230316f3053336c324633653149336631782f254430254131254430254244254430254238254430254243254430254245254430254241253230254431253844254430254241254431253830254430254230254430254244254430254230253230323031352d30382d303625323025443025423225323031322e33322e35362e706e67


Reply to this email directly or view it on GitHub
#395 (comment).

@gobwas
Copy link

gobwas commented Aug 6, 2015

@jnicholls fair enough 🍻

@tailhook
Copy link

tailhook commented Aug 8, 2015

@seanmonstar

I don't have a vision; I haven't looked that hard into how mio works. I'd love to hear suggestions.

I have a vision. In short it boils down to splitting hyper into three logical parts:

  1. Types (Headers, Status, Version..., may be some generic version of Request)
  2. Logic. For example the function determining what HTTPReader is used, should be decoupled from real streams. I.e. there should be enum like HTTPReaderKind which then is turned into current HTTPReader with a simple method like kind.with_stream(stream)
  3. And code handling real streams, with convenience Request objects implementing Read, buffering an so on.

The first item is basically ok, except maybe put types into separate crates. But logic is too coupled with streams. Decoupling it should also simplify testing AFAIU.

Then we can do competing experimental asychronous I/O implementations without rewriting too much of hyper. (I will publish my implementation soon). The biggest question on mio right now is how to make things composable. I.e. you can't mix multiple applications in same async loop, until some better abstractions are implemented, so I'm currently experimenting with it.

How does this sound? What I need to start contributing these changes into hyper?

@jnicholls
Copy link
Author

I agree that hyper should decouple the logic of composing and parsing HTTP
requests/responses from the actual I/O. This is what I alluded to in my
original request. Such a change would make it possible to run any kind of
I/O model (in-memory, blocking I/O, non-blocking I/O, etc.) and any
sub-variants thereof (unix readiness model, windows callback/IOCP model)
with any stack that a user would prefer to use (mio, curl multi-interface +
libuv, etc.)

That's a lot of freedom offered by simply splitting up the composition and
parsing logic from the I/O logic. I agree with Paul.

On Fri, Aug 7, 2015 at 8:14 PM, Paul Colomiets [email protected]
wrote:

@seanmonstar https://github.com/seanmonstar

I don't have a vision; I haven't looked that hard into how mio works. I'd
love to hear suggestions.

I have a vision. In short it boils down to splitting hyper into three
logical parts:

  1. Types (Headers, Status, Version..., may be some generic version of
    Request)
  2. Logic. For example the function determining what HTTPReader is used,
    should be decoupled from real streams. I.e. there should be enum like
    HTTPReaderKind which then is turned into current HTTPReader with a simple
    method like kind.with_stream(stream)
  3. And code handling real streams, with convenience Request objects
    implementing Read, buffering an so on.

The first item is basically ok, except maybe put types into separate
crates. But logic is too coupled with streams. Decoupling it should also
simplify testing AFAIU.

Then we can do competing experimental asychronous I/O implementations
without rewriting too much of hyper. (I will publish my implementation
soon). The biggest question on mio right now is how to make things
composable. I.e. you can't mix multiple applications in same async loop,
until some better abstractions are implemented, so I'm currently
experimenting with it.

How does this sound? What I need to start contributing these changes into
hyper?


Reply to this email directly or view it on GitHub
#395 (comment).

@seanmonstar
Copy link
Member

@Ogeon I believe I've not fixed this on the branch.

@Ogeon
Copy link
Contributor

Ogeon commented Mar 29, 2016

Yeah, it's still there. Another thing I've noticed is that wrk reports read errors. I haven't done any deeper investigations into those, though, but I guess you have seen them as well.

@seanmonstar
Copy link
Member

@Ogeon As I should have done, I've added a test in the server tests for that exact instance, and I can now say it is fixed.

As for read errors, you mean errors printed by env_logger from hyper (that the socket closed?), or the read errors reported directly from wrk? The former, I've fixed (it was a useful error log when first working on the branch). The latter, I haven't seen in a long time.

@Ogeon
Copy link
Contributor

Ogeon commented Mar 31, 2016

@Ogeon As I should have done, I've added a test in the server tests for that exact instance, and I can now say it is fixed.

Ah, nice! I can confirm that it works now.

As for read errors, you mean errors printed by env_logger from hyper (that the socket closed?), or the read errors reported directly from wrk? The former, I've fixed (it was a useful error log when first working on the branch). The latter, I haven't seen in a long time.

It's directly reported from wrk. It can look like this for my hello_world example, after a run with 456888 requests:

Socket errors: connect 0, read 456873, write 0, timeout 0

It looks like it's not one for each request, so I'm not really sure what would trigger them. Could it be something I have missed, when implementing it?

@seanmonstar
Copy link
Member

Hm, read errors from wrk are reported each time 'read(fd)' returns -1. Does
that mean the sockets may be closing before having written the end of the
response?

I don't see this in the hello world server example in hyper. What response
are you writing? Headers and body?

On Thu, Mar 31, 2016, 3:54 AM Erik Hedvall [email protected] wrote:

@Ogeon https://github.com/Ogeon As I should have done, I've added a
test in the server tests for that exact instance, and I can now say it is
fixed.

Ah, nice! I can confirm that it works now.

As for read errors, you mean errors printed by env_logger from hyper (that
the socket closed?), or the read errors reported directly from wrk? The
former, I've fixed (it was a useful error log when first working on the
branch). The latter, I haven't seen in a long time.

It's directly reported from wrk. It can look like this for my hello_world
example, after a run with 456888 requests:

Socket errors: connect 0, read 456873, write 0, timeout 0

It looks like it's not one for each request, so I'm not really sure what
would trigger them. Could it be something I have missed, when?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#395 (comment)

@Ogeon
Copy link
Contributor

Ogeon commented Mar 31, 2016

It's the hello_world example from Rustful, and it's the same for other examples there, as well. It should just write "Hello, stranger!" and the headers should just be date, content length, content type, and server name. It looks fine in the browser, so that's why I'm a bit confused. I tried some of the Hyper examples and they didn't generate the errors, so there must be something with how I'm doing it. I'll investigate more later, and see what I can find.

@Ogeon
Copy link
Contributor

Ogeon commented Apr 1, 2016

Ok, I found something. The "simplified" handler goes into wait mode if no body should be read, and waits for a signal that tells it that it's time to start the writing the response. Changing this to make it immediately go into write mode (which will mess with the flow, but should work for hello_world) makes the errors disappear.

I tried to replicate the hello example from Hyper, using the more advanced handler method. It did not call Next::wait and it did not cause any errors. I changed it to call Next::wait, and made it call control.ready(Next::write()) from a thread, and that made the errors appear again.

What do you think?

@seanmonstar
Copy link
Member

@Ogeon hm, I am able to reproduce if I wait() and spawn a thread to call ready(), as you say. When I make a tiny Rust program to open a tcp stream, write a request, and read a response, I don't get any io errors, or reads of 0. Can you see something else here that would make wrk goto error: https://github.com/wg/wrk/blob/master/src/wrk.c#L426-L448

@Ogeon
Copy link
Contributor

Ogeon commented Apr 2, 2016

Hard to say. The error conditions seems to be either a network error (return -1), a 0 read before finishing the response, or some failure in http_parser_execute. There shouldn't be anything to read (unless the errors appears when data is finally received), so the only logical failures would be the network error or the 0 read, given that http_parser_execute returns the number of parsed bytes. It's quite complex, though, so it's hard to say exactly what happens. Either way, 0 reads will still cause problems.

If it does read something, then the problem should be caused by something in http_parser_execute. It's huge and has many jumps to error, but it looks like it would report those errors to the user.

I tried to pass the --timeout 10s option, but that didn't make a difference. That should show up in the timeout counter, instead, so I didn't really expect anything.

@seanmonstar
Copy link
Member

It looks like they forgot to check errno in sock_read to return RETRY, and
so the EAGAIN error is being reported as a read error.

On Sat, Apr 2, 2016, 4:03 AM Erik Hedvall [email protected] wrote:

Hard to say. The error conditions seems to be either a network error
(return -1), a 0 read before finishing the response, or some failure in
http_parser_execute. There shouldn't be anything to read (unless the
errors appears when data is finally received), so the only logical failures
would be the network error or the 0 read, given that http_parser_execute
returns the number of parsed bytes. It's quite complex
https://github.com/wg/wrk/blob/master/src/http_parser.c#L627, though,
so it's hard to say exactly what happens. Either way, 0 reads will still
cause problems.

If it does read something, then the problem should be caused by something
in http_parser_execute. It's huge and has many jumps to error
https://github.com/wg/wrk/blob/master/src/http_parser.c#L2069, but it
looks like it would report those errors to the user.

I tried to pass the --timeout 10s option, but that didn't make a
difference. That should show up in the timeout counter, instead, so I
didn't really expect anything.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#395 (comment)

@Ogeon
Copy link
Contributor

Ogeon commented Apr 2, 2016

Hmm, yeah, looks like that's the case. It's also static, so I guess it won't be replaced by an other function.

@teburd
Copy link
Contributor

teburd commented Apr 4, 2016

I've noticed that the latency, and variation of latency with the mio branch is much higher and seems to have a larger spread than whats currently on master. I ran cargo --release --example server in each branch then ran wrk against it.

Mio

Running 30s test @ http://127.0.0.1:1337/
  10 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    22.23ms   34.30ms   1.03s    99.67%
    Req/Sec     4.76k   693.41     8.08k    88.07%
  1421686 requests in 30.06s, 122.02MB read
Requests/sec:  47290.16
Transfer/sec:      4.06MB
wrk -c 1000 -t 10 -d 30s "http://127.0.0.1:1337/"  2.80s user 11.56s system 47% cpu 30.111 total

Master

Running 30s test @ http://127.0.0.1:1337/
  10 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    54.80us   93.95us  22.16ms   98.91%
    Req/Sec    84.24k    15.81k   97.11k    78.33%
  2512893 requests in 30.07s, 215.68MB read
Requests/sec:  83560.27
Transfer/sec:      7.17MB
wrk -c 1000 -t 10 -d 30s "http://127.0.0.1:1337/"  5.07s user 22.54s system 91% cpu 30.122 total

Can anyone else confirm this? Is this a known issue?

@arthurprs
Copy link
Contributor

The mio branch cpu usage is like half of the threaded. That might be related

@seanmonstar
Copy link
Member

It may help to edit the async example to spawn a server in several threads, so as to compare to the master example which is using 1.25 threads per cpus.

@Ogeon
Copy link
Contributor

Ogeon commented Apr 11, 2016

I just noticed that the way Control::ready behaves has changed. It's no longer possible to call it from on_request and then make on_request return Next::wait() (small example). It won't wake up after on_request. Is this a bug?

@seanmonstar
Copy link
Member

@Ogeon It was indeed a bug. I added the use of an AtomicBool to reduce the amount of usage on the mio's bounded queue (to reduce errors from it being full), and had an incorrect if check. Fixed.

@Ogeon
Copy link
Contributor

Ogeon commented Apr 11, 2016

Alright, nice! 😄 It works just as before, now.

@Ogeon
Copy link
Contributor

Ogeon commented Apr 13, 2016

I've been trying to re-enable OpenSSL support in Rustful today, and it went alright, except when it came to dealing with the different Transport types. The fact that many parts of the API depends on it gives me three alternatives:

  1. Make almost everything in Rustful generic over Transport. This would work if it didn't cause a bunch of coherence problems in some important parts.
  2. Make Http/Https enums for Encoder and Decoder. This would work if the Transport type for HTTPS was public. I tried with <Openssl as Ssl>::Stream, as well, but it looks like it counts as a type parameter and caused conflicting implementations of From.
  3. Mask Encoder<T> and Decoder<T> as Read and Write. My current solution is a variant of this, but it's not so nice, and won't work well if more than simple read/write functionality is ever expected.

It would be nice if the Transport type could be made public for the OpenSSL case, allowing solution 2, or if both HTTP and HTTPS used the same type (I saw HttpsStream in there, but I guess that's something else), allowing simple type aliases for Encoder and Decoder. A completely different solution is, of course, also welcome.

@seanmonstar
Copy link
Member

@Ogeon ah, I hadn't noticed that the OpensslStream type wasn't being exported. Just doing that should be enough, right? Then for https, (if you wish to utilize only openssl), you could use HttpsStream<OpensslStream<HttpStream>>.

@Ogeon
Copy link
Contributor

Ogeon commented Apr 13, 2016

That should do it, as far as I could tell. It did look like it was just OpensslStream<HttpStream>, though, but it could also have been some misleading error messages.

@erikjohnston
Copy link

Ignore me if you've already done this, but if you're using non-blocking sockets with OpenSSL you can't just use the io::{Read, Write} traits, you need to port the code to use ssl_read and ssl_write [1] instead. This is because OpenSSL can return special error codes that indicate that the code must call read if they're currently writing or write if they're currently reading. (Or at least that's my understanding).

If you don't, it will work most of the time and then occasionally fail.

[1] http://sfackler.github.io/rust-openssl/doc/v0.7.9/openssl/ssl/struct.SslStream.html#method.ssl_read

@seanmonstar
Copy link
Member

@erikjohnston yep, I know about it. I haven't handled it yet, as I slowed down trying to design a generic way for the Transport trait to report it, and moved on to other pieces that were missing. I won't ship the version without getting that in place, however.


Basically, my initial thoughts were something like this:

trait Transport {
    // ... 
    fn blocked(&self) -> Option<Blocked> {
        // default implementations assume nothing special
        None
    }
}

enum Blocked {
    Read,
    Write,
}

The blocked method could be checked by the event loop when deciding what events to listen for. If the result is Some, the event will be added to the event list.

So an implementation for openssl could do something like this:

struct OpensslStream {
    stream: openssl::SslStream<HttpStream>,
    blocked: Option<Blocked>,
}

impl Read for OpensslStream {
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        match self.stream.ssl_read(buf) {
            Ok(n) => Ok(n),
            Err(openssl::Error::WantWrite(io)) => {
                self.blocked = Some(Blocked::Write);
                return Err(io);
            },
            // ...
        }
    }
}

Does this seem like it would work for any other transport implementation, like schannel or security-framework? @sfackler @frewsxcv

@sfackler
Copy link
Contributor

That should work, yeah, but it's unfortunate that there's that out of band signaling of what you're blocked on. Maybe there should be a tweaked Read trait that could return the proper information?

@seanmonstar
Copy link
Member

@sfackler well, the stream is present to the Handler as T: Read or T: Write, so user code would be using those traits. This design was to hopefully remove the need for a Handler to notice if the underlying stream protocol needed to read, even if the user just wants Next::write() a bunch.

@Ogeon
Copy link
Contributor

Ogeon commented Apr 13, 2016

@seanmonstar I could implement the nicer solution nr. 2 now, when OpensslStream is public, so all is well. 😄 It's an ok solution. Also, I had to use Encoder<OpensslStream<HttpStream>> and Encoder<OpensslStream<HttpStream>>, as I vaguely mentioned before. I'm not sure if that was what you actually meant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests