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

Add an encapsulated file stream in axum-extra to make it more conveni… #3047

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

YanHeDoki
Copy link

@YanHeDoki YanHeDoki commented Nov 24, 2024

Motivation

In use will need to return a file stream, encapsulates a file stream structure to save the return time to add the header

Solution

In axum-extra add a response structure, the construction needs to pass a file stream, in the response will add the header: “application/octet-stream”, I added the documentation and examples these will be more detailed

 use axum::{
     http::StatusCode,
     response::{Response, IntoResponse},
     Router,
     routing::get
 };
use tokio::net::TcpListener;
 use axum_extra::response::file_stream::FileStream;
 use tokio::fs::File;
 use tokio_util::io::ReaderStream ;

#[tokio::main]
async fn main() {
    let app = Router::new().route("/", get(file_stream));
    let listener = TcpListener::bind("127.0.0.1:3000").await.unwrap();
    axum::serve(listener, app).await.unwrap();
}

 async fn file_stream() -> Result<Response, (StatusCode, String)> {    
     let     stream=ReaderStream::new(File::open("file_path").await.map_err(|e| (StatusCode::NOT_FOUND, format!("File not found: 
       {e}")))?);
     let file_stream_resp = FileStream::new(stream)
         .file_name("You want to set the file name");

     Ok(file_stream_resp.into_response())
 }

See examples/stream-to-file for a short demonstration I've added usage to this example.

I just started using rust, so some of the code may not be well written

axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
examples/stream-to-file/Cargo.toml Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Show resolved Hide resolved
axum-extra/Cargo.toml Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
examples/stream-to-file/src/main.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@yanns yanns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me 💯

@yanns
Copy link
Collaborator

yanns commented Dec 1, 2024

Can you add an entry in the https://github.com/tokio-rs/axum/blob/main/axum-extra/CHANGELOG.md ?

@YanHeDoki
Copy link
Author

Can you add an entry in the https://github.com/tokio-rs/axum/blob/main/axum-extra/CHANGELOG.md ?

Yes, I added it.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few things I'd like to see improved before we ship this, but maybe it's easier if we merge now and I post a follow-up PR. It's nothing big.

axum-extra/src/lib.rs Outdated Show resolved Hide resolved
axum-extra/src/response/mod.rs Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
@YanHeDoki
Copy link
Author

There's a few things I'd like to see improved before we ship this, but maybe it's easier if we merge now and I post a follow-up PR. It's nothing big.

Okay, I improved it. Can you review it again?

@ttys3
Copy link
Contributor

ttys3 commented Dec 2, 2024

the usage of this PR feature seems rather limited.

for example, it seems that it does not handle HTTP range requests

and it limited the response header. it fixed to application/octet-stream and attachment; filename=xxx

and even in download mode, it does not support inline, only support attachment model.

people may want to steam a video file, and need HTTP range requests support. application/octet-stream is too limited.

as a framework, the feature builtin should be more general.

I suggest put this as an external crate, if it is limited to application/octet-stream

what do you think? @yanns @jplatte

@yanns
Copy link
Collaborator

yanns commented Dec 2, 2024

@ttys3 yes I see your point.
It's a bit sad for @YanHeDoki to receive this feedback after many iterations, but I guess it's the right decision.
@YanHeDoki would you mind putting this code into an external crate and add it to the ecosystem?

@jplatte
Copy link
Member

jplatte commented Dec 2, 2024

Wait, why would we not add stuff like range header support to this and have it in axum-extra then? It's a very common feature request.

axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
@YanHeDoki
Copy link
Author

I added file streaming support for range headers with the try_range_response and into_range_response methods, also add documentation and tests and use examples

I just wanted simple support for streaming file downloads, so I didn't make it complicated, and perhaps it should have been added to the attachment feature would have been better. maybe continue to improve the functionality

@yanns @jplatte

@YanHeDoki
Copy link
Author

@ttys3 yes I see your point. It's a bit sad for @YanHeDoki to receive this feedback after many iterations, but I guess it's the right decision. @YanHeDoki would you mind putting this code into an external crate and add it to the ecosystem?

But as a standalone crate is it too simple, maybe it should be added to the attachment feature?

Comment on lines 276 to 294
let mut buffer = vec![0; buffer_size];

let stream = async_stream::try_stream! {
let mut total_read = 0;

while total_read < end {
let bytes_to_read = std::cmp::min(buffer_size as u64, end - total_read);
let n = file.read(&mut buffer[..bytes_to_read as usize]).await.map_err(|e| {
std::io::Error::new(std::io::ErrorKind::Other, e)
})?;
if n == 0 {
break; // EOF
}
total_read += n as u64;
yield buffer[..n].to_vec();

}
};
Ok(stream)
Copy link
Contributor

@paolobarbolini paolobarbolini Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to replace this entire part with AsyncReadExt::take and then going back to ReaderStream

@jplatte
Copy link
Member

jplatte commented Dec 3, 2024

I still think it makes sense to have this as a feature. It's independent of Attachment in its purpose, and can be combined with it if wanted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to split the example change into a separate PR.

Comment on lines +16 to +17
/// Alias for `tokio_util::io::ReaderStream<File>`.
pub type AsyncReaderStream = ReaderStream<File>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the type alias? ReaderStream<File> is not a very complex type.

axum-extra/src/lib.rs Outdated Show resolved Hide resolved
axum-extra/src/lib.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
axum-extra/src/response/file_stream.rs Outdated Show resolved Hide resolved
@jplatte
Copy link
Member

jplatte commented Dec 3, 2024

I've pushed a bunch of minor changes, hope that's fine. Previous review comments should still be valid.

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

Successfully merging this pull request may close these issues.

5 participants