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

Allow streaming responses of types that do not implement io::Read #813

Closed
lovasoa opened this issue Nov 4, 2018 · 8 comments
Closed

Allow streaming responses of types that do not implement io::Read #813

lovasoa opened this issue Nov 4, 2018 · 8 comments
Labels
request Request for new functionality

Comments

@lovasoa
Copy link

lovasoa commented Nov 4, 2018

Currently, the only way to stream data from the server to the client is to have a type implementing
io::Read.

This is quite a constraint, because of the type signature of the read method : the object has to be able to write a given amount of data to a given buffer, save its state, and then resume writing on the next call to read. Most libraries that allow the generation of data in a streaming fashion do not support that. Instead of implementing Read, they implement an object that takes a writer in the form of a generic Write argument, and write their result directly into that object.

Proposition

  1. Create a new trait called Writable with a single method:
fn write<W: Write>(&mut self, destination: &mut W) -> Result<()>
  1. Implement this trait for all Read values :
impl<T: Read> Writable for T { ... }
  1. Change the type signature of the methods in Stream and Response to accept arguments of type Writable instead of Read

Use-cases

#499 Is a good example of why streaming values that do not implement Read is a good idea.

An example is the handlebars library. Its Handlebar struct has a render_to_write method that can stream a template into a Write object.

Another example is serde and its Serializer that also takes a Write object.

lovasoa added a commit to lovasoa/Rocket that referenced this issue Nov 4, 2018
@SergioBenitez
Copy link
Member

I'm in full agreement that this isn't the ideal interface, and while I also agree that there are workable solutions to the problem, it is unlikely that any which modify Responder will be merged. When we migrate to async (#17), the plan is to allow Responders to write directly to the outgoing stream. This includes writing things like headers, and, of course, the response body. Critically, the interface must remain easily composable.

I'd be happy to merge code that provides an adapter that implements Read and can be written to, however, likely with a performance and resource penalty. This way, however, Responder doesn't change, nor do any existing implementations.

@SergioBenitez SergioBenitez added the request Request for new functionality label Nov 5, 2018
@lovasoa
Copy link
Author

lovasoa commented Nov 5, 2018

Well, if the interface cannot change, then I wonder whether it is really relevant to provide an inefficient Read <-> Write adapter inside rocket. If someone really wants it (and the associated performance penalty), it can be provided as a separate crate.

@lovasoa
Copy link
Author

lovasoa commented Nov 5, 2018

Here is an example of such an adapter using the pipe crate :

/// Starts the given function in a thread and return a reader of the generated data
pub fn piper<F: Send + 'static>(writefn: F) -> PipeReader
    where F: FnOnce(PipeWriter) -> Result<()> {
    let (reader, writer) = pipe();
    thread::spawn(move || writefn(writer));
    reader
}

See gist

lovasoa added a commit to lovasoa/Rocket that referenced this issue Nov 6, 2018
@SergioBenitez
Copy link
Member

It can absolutely be an external crate. The only reason to upstream it into Rocket is to give it greater visibility.

@SergioBenitez
Copy link
Member

As this can be done entirely externally, and as we'll eventually resolve this when we migrate to async, I'm closing this out.

@lovasoa
Copy link
Author

lovasoa commented Nov 12, 2018

Well, this cannot be done entirely externally without some ugly glue code, and a performance penalty.

@SergioBenitez : Why not leaving this issue open until this is implemented (as part of the migration to async or otherwise) ? This would allow people (including me 😃 ) to track this issue specifically.

@SergioBenitez
Copy link
Member

@lovasoa I don't typically leave issues open when another issue supersedes it. In this case, that issue is #17. I'm okay with accepting a PR that resolves this, but I am not committing to implementing it myself nor explicitly advocating that others do.

@lovasoa
Copy link
Author

lovasoa commented Aug 2, 2019

Hello ! Now that the work on async is closer to being released, what is the story about this ? Will it be possible to use libraries that generate data by taking a Write argument ?

@jebrosen

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

No branches or pull requests

2 participants