Skip to content

Conversation

@jprochazk
Copy link
Collaborator

Add ehttp::streaming when the streaming feature is enabled.

On the web, ehttp::streaming::fetch uses wasm-streams to handle the conversion of a Response body to a Rust async Stream.

On native, ehttp::streaming::fetch uses ureq the same way as blocking fetch, but using into_reader on the body instead of read_to_end.

@jprochazk jprochazk mentioned this pull request Jun 14, 2023
2 tasks
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Looks great! I assume you have tested it both locally and on web.

It would be really nice with an usage example in example_eframe, but it can be added in a separate PR.


let mut reader = resp.into_reader();
loop {
let mut buf = vec![0; 2048];
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a rational for the 2048 size?

MTU is usually around 1500 bytes, but I'm not sure that really matters here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just rounded up common path MTU to something that seemed reasonable.

std::thread::Builder::new()
.name("ehttp".to_owned())
.spawn(move || fetch_streaming_blocking(request, on_data))
.expect("Failed to spawn ehttp thread");
Copy link
Owner

Choose a reason for hiding this comment

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

This expect could be replaced with calling on_data(Err(…))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would require putting the callback in an Rc, but I guess that's fine. Should I make the same change in the non-streaming native fetch?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah right, let it be then.

Co-authored-by: Emil Ernerfeldt <[email protected]>
@emilk
Copy link
Owner

emilk commented Jun 15, 2023

@jprochazk
Copy link
Collaborator Author

Not sure what to do about the cargo deny failure

@emilk
Copy link
Owner

emilk commented Jun 15, 2023

The cargo deny failure is unrelated to this PR. I'll fix it on master.

@emilk emilk merged commit fa7b424 into emilk:master Jun 15, 2023
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.

2 participants