-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
(async) Add a Write-like interface for responses #1066
Comments
Related: #813 |
I spent some time with @carllerche chatting about bringing such an interface to Hyper in 0.13, and my understanding is that he's onboard. If the main interface in Hyper 0.13 is a write-based one, then implementing this in Rocket should be relatively straightforward. Implementing a read-based interface on-top of a write-based interface is itself straightforward, of course. To clarify, and for posterity, a write-based interface would allow you to write a impl Responder for MyType {
fn respond_to(self, request: &Request, out: &mut Out) -> impl Future<_> {
for _ in 0..5 {
sleep(5).await;
write!(out, "data: hello\n").await;
}
write!(out, "\n").await;
}
} This responder writes out 5 server-sent events, each with data of "hello", one every 5 seconds. A read-based implementation of the above, while not significantly more code, is, in my opinion, significantly more difficult to write and understand: struct MyType {
remaining: usize,
next_write: Time,
}
impl Read for MyType {
fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
let now = Time::now();
if now > self.next_write && self.remaining > 0 {
write!(buf, "data: hello\n")?;
self.next_write = now + 5.seconds();
self.remaining -= 1;
if self.remaining == 0 {
write!(buf, "\n")?;
}
}
}
} For many types of responses, a read-based interface is the write approach, especially as many types already have a read-based implementation ( |
I am concerned about the ergonomics of a read-like operation that has to both consume and expose a write-like interface, or vice versa. One particular example is automatic The solution to this point may be "don't do anything that requires the body to actually 'finish'" -- with SSE, it might never finish! -- and that is fine but we should be aware of it and any similar code that already exists and make this point very clear in the documentation and migration guide or CHANGELOG. |
As I mentioned to @SergioBenitez, I am open to the best solution. I have not spent a lot of time considering the proposed API. For me, it would be a question of weighing the pros / cons of each avenue. Either way, I don't think it should impact the core |
I've been trying to draft pseudocode implementations of The flow currently defined by all of tower-service, hyper, and Rocket is approximately If we kept the current async fn streaming_route() -> AsyncWrittenResponse {
AsyncWrittenResponse::from(|writer| async move {
for _ in 0..5 {
sleep(5).await;
write!(writer, "data: hello\n").await;
}
write!(writer, "\n").await;
})
} |
I have had this at the back of my mind for a while but not given too much dedicated attention to it. Here are some of my thoughts on the low-level I will refer to operations like SSE as a "pushing response", and reading a file off of the disk as a "pullable response". I will use synchronous terminology, but the arguments almost entirely apply the same to the asynchronous equivalent.
|
I think it's time to revive and tackle this this issue now that I've had some more time to work with things and hyper 0.13 is released. There are actually two/three orthogonal aspects to consider here:
Note that:
Rocket 0.4 exposes a pull-based model with Adapting Rocket's current pull-based
I propose to solve these as follows:
Note that similar concerns apply to Another note: @SergioBenitez mentioned:
I actually found that this is not necessarily the case. I made an SSE implementation (https://git.jebrosen.com/jeb/rocket-rooms/src/commit/28479ce5d73b761f3c32f397726f8d28487ba0d3/src/sse/v2.rs) that converts a A stream-based version of the "write-like" example might look like this:
|
One of the main trade-offs you were mentioning is that middleware, like etag and gzip, wants to have a pull interface, while event systems (SSE, WebSocket, long polling) want a push interface. While it would probably be a little bit of a wart, I've seen other places that will offer a pull interface by default, and use a Though actually grabbing the raw socket like Discourse's Hijack middleware does is probably a non-starter since Rocket supports HTTP/2, and it uses a single socket is used for multiple responders at once. You can code up your own implementation of |
Websockets and see just don't fit in the normal middleware flow because they aren't necessarily http and so it doesn't apply. By definition the middleware idea of a response goes out the window. It should just be documented that if you 'hijack' a connection and upgrade it into a websocket, the response stuff just won't fire. But it might make sense to add another middleware hook for these kinds of events so things like logging middleware could could be like 'oh it got converted, I need to do something'. Also, does this bring in the idea perhaps of websocket or sse 'middleware'? Something that can wrap around their connection lifecycle and provide hooks useful for other things as well? |
This reworks the entire 'response::stream' module for async streams. Resolves #1066.
This reworks the entire 'response::stream' module for async streams. Resolves #1066.
This reworks the entire 'response::stream' module for async streams. Resolves #1066.
We should have an
AsyncWrite
responder type and/or changeResponse
to be based onAsyncWrite
instead of or alongsideAsyncRead
. This will make it easier to implement SSE among other things.For most cases routes should be able to use a channel to work around this while it is not implemented, but it will be less efficient and code might be more difficult to understand.
The text was updated successfully, but these errors were encountered: