-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
The design of the Futures-based Body Stream #953
Comments
Could you open a bit how this is supposed to work currently? I'm trying switch to the hyper HEAD and having a problem reading the body with this code: let body_f = response.body().fold(body_vec, |mut acc, chunk| {
acc.extend_from_slice(chunk.as_ref());
Ok(acc)
}).map_err(|_| { response::FcmError::ServerError(None) }); Where compiler gives me:
Now, is there some good practice how to read the body to a vector without blocking? |
I have hit this exact same error when I was writing tests. I'll check in with the tokio team about the error. I also hope to write some guides explaining how to stream a body in hyper. |
@pimeys I think the error you're seeing there is just due to type inference in the compiler and the The error in your example I believe is that |
Pretty much got it working and got my first proper use case with hyper-tokio client to compile: https://github.com/pimeys/fcm-rust/blob/tokio/src/client/mod.rs#L82 I'm trying to write an asynchronious google push notification client with Tokio. I needed to modify Hyper a bit to to be able to modify headers and have access to some internal things which I think I should be able to refactor at some point. But god damn, it works. Now I need to get this working with HEAD, get it fast and get rid of that Handle passing in the constructor, as noted here #1002. But first, I want to have this working on our use case, and we will have a handle to use :) |
@pimeys great! I'm curious about what modifications you need to make, if you could open separate issues about them, so can we keep the focus here of the design of the |
So, the
Does it make sense to also integrate with tokio's Note that tokio's |
This API already will make it possible to avoid copying in several cases, but I'd prefer it use the owning_ref crate. That would let callers can supply anything that can give them a enum Inner {
Owned(Vec<u8>),
Referenced(Arc<Vec<u8>>),
Mem(MemSlice),
Static(&'static [u8]),
// ...possibly EasyBuf as mentioned in the previous comment...
} with this: enum Inner {
Static(&'static [u8]),
Vec(owning_ref::VecRef<[u8]>),
Box(owning_ref::BoxRef<[u8]>),
Arc(owning_ref::ArcRef<[u8]>),
// ...I also mentioned owning_ref::RcRef in #934 but now think it wouldn't actually work...
} and add Here are a couple examples of how I'd use the extra flexibility in a real program (which does HTTP range serving of .mp4 files with the actual frames of video taken from files on disk and everything else from a SQLite database by dividing up into a bunch of "slices" of the file's byte positions generated in various ways):
(I think my use case stresses this API more than most; I'd be happy to tell you more about it if you're interested.) The only other improvement I can think of would be if the stuff that needs to reference a per-request object could avoid reference counting by the chunks having some per-request lifetime bound. But I haven't done any benchmarking to see if an atomic reference count on each chunk is a noticeable performance impact in my program (or any realistic scenario, for that matter), and I'm not even sure what I'm vaguely describing is possible in safe Rust code. |
@scottlamb I tried looking through the owning_ref crate, and had a hard time understanding it's purpose. As for memmaped slices, that sounds like the possibility of a bad time, if the memory is on disk and not in RAM when hyper tries to deref the slice to write into the socket. That blocking file IO could really hurt the performance of the event loop. I'm also not sure how to ask for an async deref, such that the loading could be done on another thread. It sounds like it would complicate a lot of things. |
Okay. I'm not too surprised to hear you say that. I was thinking about it; instead of controlling which thread accesses the chunk, maybe I can get the same effect by calling Anyway, if the mlock approach doesn't work out and I end up wanting help from hyper with controlling the thread that accesses the memory, I'll open a separate issue as it's clearly a whole other thing to consider.
I just worked through an example which I think will make this clearer to both of us. (It showed me that I had the wrong Here I'm using it for the mmap case. I'm starting with a extern crate owning_ref;
extern crate libc;
extern crate memmap;
use std::io;
use std::fs::File;
pub struct Chunk(Inner);
enum Inner {
Static(&'static [u8]),
Vec(owning_ref::VecRef<u8, [u8]>),
Box(owning_ref::ErasedBoxRef<[u8]>),
Arc(owning_ref::ErasedArcRef<[u8]>),
}
impl<T: 'static> From<owning_ref::BoxRef<T, [u8]>> for Chunk {
#[inline]
fn from(r: owning_ref::BoxRef<T, [u8]>) -> Chunk {
Chunk(Inner::Box(r.erase_owner()))
}
}
fn main() {
let f = File::open("f").unwrap();
let mmap = Box::new(
memmap::Mmap::open(&f, memmap::Protection::Read).unwrap());
if unsafe { libc::mlock(mmap.ptr() as *const libc::c_void, mmap.len()) } < 0 {
panic!("{}", io::Error::last_os_error());
}
let mmap = owning_ref::BoxRef::new(mmap);
let mmap = mmap.map(|m| unsafe { m.as_slice() });
let chunk: Chunk = mmap.into();
} Likewise, when I want to trim a vector before sending it out on the wire, I can stuff it into a |
I have a dumb question orthogonal to our In particular, a |
I think I understand now how owning_ref works. Essentially, having I get the appeal of that, and actually find that pretty neat. At the same time, I'm wary of linking hyper's public API to owning_ref, if for no other reason than I feel like owning_ref is an obscure crate. Maybe that feeling is baseless, or maybe there's another way to get a similar affect without relying on that crate? Regarding sending multiple chunks, it assumes you don't have the multiple chunks ready yet to send (I would expect that normally someone has the bytes bundled into a single chunk). So, you must spawn some sort of other task that will try to send more chunks into the body as they become available, and the body has room. This could be a CPU task from futures-cpupool, or some other task in tokio (such as another network request). If doing that feels bad, we should definitely try to find how to make that better. That'd partly be up to the design of tokio itself, and so possibly people would have input on the tokio-proto repo or the tokio gitter room, but it's also something that we can explore (and push the feedback in to tokio). We can ping Alex or Carl if we have specific feedback, too. |
Yeah, that's among the things you can do with it, via something like this: let v = owning_ref::VecRef(b"Hello World".to_vec()).map(|v| &v[6..]);
let chunk: Chunk = v.into();
I dunno what to say about that feeling—on the one hand, it has an order of magnitude fewer downloads than hyper, on the other it's a relatively small amount of code and has been maintained for the past 18 months. For the
How do I do that? I think I need a |
|
fwiw, I think that's a one-liner already (untested): |
I've tested the folding and it works. Although it was not very obvious, good documentation would help here. |
I'm working out a plan on how to build a fork of multipart on top of the async API once the dust settles. For the server-side API, I'm thinking of having a Of course, the main problem that I see two potential solutions:
|
So, something that I think solves both @abonander and @scottlamb's cases, plus various others, is if the
A few questions remain regarding using
|
Hmm, I think that would give me the ability to subset a Chunk, but not to have it backed by something more complex than a |
@scottlamb you're right, I forgot about mmap. Asking in the tokio room, the suggested advice was basically "don't do that", because of the pauses in the event loop that can occur from loading the memory from disk. With those warnings, do you still find it compelling to try to do it anyways? |
I should answer your question about if this is worthwhile with a benchmark. I'll try to work one up, but I apologize in advance for slowness: I have a tiny amount of time for my Rust projects every week and have been spreading that kinda thin, so nothing I do goes fast. I did look at it for a moment tonight and realized a problem with using Intuitively, it seems a little silly IMHO to go to the effort of the switch from sync to async (with all the pain of a non-intuitive programming model, the danger of accidentally doing slow/blocking stuff on the wrong thread, etc), in the name of performance (I think that was the main goal?) and then have to do context switches between every read and write and do extra copies. But maybe it comes down to a choice about what's important to optimize: small requests or large requests, serving from the filesystem or from elsewhere, etc. When I mention using mmap and avoiding copies, I'm thinking about filesystem-based responses much larger than the CPU cache. |
What about |
I just realized we're talking about serving files. You can also use Addendum: Linux also has |
I brought up
|
See issues: hyperium/hyper#953 Kimundi#24 I'm not sure what the right way to do this is (maybe a second erased type, SyncErased?) but this is a quick hack that will let me throw together a benchmark of zero-copy mmap in hyper.
See hyperium#953 for context.
I'd love an option to get the raw socket behind it, as there's times where you want to avoid userland completely, and just use a Right now, this doesn't seem doable, or did I miss something during my experiments? (Granted, this is not really asynchronous, but this is still a very real use-case) |
@Michael-Zapata There is not an option to do that for now. The ability to borrow the socket would probably require some coordination with tokio, since there is tokio-proto types in-between the user and the socket. It may be slightly easier to instead allow passing some |
I've worked on making the user-supplied body stream generic, and it exists for both the server and client in this branch: https://github.com/hyperium/hyper/compare/outgoing-generic A few things I either need to fiddle with, or would welcome feedback on, are:
|
I don't think it's worth worrying about library writers shooting themselves in the foot by falling back on the default type param. It's pretty clear to me that impl<C> Client<C, hyper::Body> where C: Connect {
/// Send a GET Request using this Client.
#[inline]
pub fn get(&self, url: Url) -> FutureResponse {
self.request(Request::new(Method::Get, url))
}
} (Or alternately, I doubt you'll get many users who are too lazy to write However, this leads into a separate concern I have, but one that I don't think will be easy to resolve because it also digs into Tokio a good bit: I don't think |
@seanmonstar wrote:
Maybe a dumb question, but what is this error used for? I think it's basically just for hyper to know that it should drop the connection to indicate error to the HTTP peer? I think even
(By the way, there probably should be some way for hyper::server to tell the application if the whole response including the last chunk was fully written or not, but I think that's a separate concern from the body stream.) @abonander wrote:
I think the server side is actually similar: there's only one |
This kinda goes for |
Huh. I traced through the code in tokio and played with some examples, and found that a user will never receive the The purpose of requiring it on the Response side is currently a limited in tokio: you can only define 1 error type to be used through out. I know from conversations that the tokio team want to improve on error handling in newer versions, so maybe some of these points go away. For the time being, though, I wonder if it makes sense to just be an The points about the |
Actually, thanks to another coversation in the tokio room, I realized that it is possible for users make use of the It is possible to wrap the struct Example;
impl Service for Example {
type Request = Result<hyper::server::Request, hyper::Error>;
// other types are the same
fn call(&self, incoming: Self::Request) -> Self::Future {
match incoming {
Ok(req) => {
},
Err(parse_error) => {
// you could check for some error types
// like Error::TooLarge, you could respond with a 414
},
}
}
} It's possibly worth wondering if |
@seanmonstar I mean, I guess if there's no type erasure going on in a deeper abstraction layer that this could be rolled into somehow then the point is moot. |
See hyper issue: hyperium/hyper#953 I wanted this for the ability to define a mmap-friendly chunk, but I haven't used that yet. I need to put some thought into what chunk enum to use; owning_ref and reffers are possibilities. In the meantime, I'm keeping it as generic as possible via silly-verbose where clauses. Another nice benefit is that I no longer have to use .send_all() and a tokio_core::reactor::Remote to push it through a mpsc; I can directly return a flatten stream.
cc rust-lang/futures-rs#390, a possible extension that came up during integration in mozilla/sccache#70 |
I'm in the process of blogging about futures and the changes to hyper for my company's blog. I have some questions about streaming responses with hyper. I put together a small example that just creates an infinite stream and sends out a Firstly, is this the proper way to handle this sort of streaming? I want to eventually rewrite esper and will be keeping open a connection for an Event Source stream so I can send data as events are received from other clients. Is there a better way to send a stream of Secondly, when I run this example and connect a client to the stream, I see the CPU usage for the process spikes to 100%. I think this is likely because I am doing something wrong with the |
@mikeycgto The link to the example doesn't resolve, so I'm able to comment on most of your questions... |
Sorry about that @seanmonstar! The repo was private and I have now made it public. Thanks! |
That's certainly a way to do it.
I don't know about a 'better' way, but the outgoing (user-supplied) body stream is generic, so you can certainly create your own
I'm not familiar at all with how the tokio-timer crate works. To try to locate where the CPU usage is coming from, you could compare with different kind of infinite stream. A naive implementation could spawn a thread, use |
@mikeycgto, I wonder if your busy-looping is a bug in tokio or mio. I'm also seeing terrible performance with somewhat different code (a I don't understand the problem well enough to know if it's related to what you're seeing or not. |
@scottlamb @mikeycgto the busy-looping may have been in part due to code in hyper, which was adjusted in #1074. Still seems to me that tokio shouldn't try to flush if it didn't write anything, but oh well. |
No longer seeing the busy looping. Thanks Sean! |
hyper now uses the bytes crate, and so there is an I do wonder if the |
I'm leaning towards closing this as fixed, unless there's an issue that hasn't been addressed yet. |
Kind of a naive idea, but I was wondering if there could be some way to have a return stream of |
@abonander If you keep hold of a In that case though, it's probably better for this stuff to make use of |
For anyone arriving here by searching for hyper and mmap, the body_image master branch (candidate for body_image 0.4.0 release) now has zero-copy/async. support for memory mapped http bodies using (glibc) Thanks for publishing your bench results, @scottlamb. I have some non-hyper specific tokio benchmarks included in body_image as well (see CHANGELOG.md for those results.) An |
Currently exists as a
futures::Stream
ofhttp::Chunk
. The idea ofChunk
is to keep the internals private, to allow for optimizations to happen without breaking changes, and then addFrom
implementations that users can take advantage of.Besides
Vec<u8>
, it will also likely have a form ofAppendBuf
, which will allow the sending slices from the same buffer when reading in a message body, instead of allocating an arbitrarily sizedVec
, reading, and then shrinking.It could also want some way to prevent double copies, as much as possible, when a user is creating
Chunk
s./cc #934
Thanks to discussion below, here's the action items to complete this:
Stream
MemBuf
with the bytes crate inChunk
The text was updated successfully, but these errors were encountered: