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

Streaming fetch #28

Merged
merged 18 commits into from
Jun 15, 2023
Merged

Streaming fetch #28

merged 18 commits into from
Jun 15, 2023

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.

ehttp/Cargo.toml Outdated Show resolved Hide resolved
ehttp/Cargo.toml Outdated Show resolved Hide resolved
ehttp/src/streaming/types.rs Outdated Show resolved Hide resolved
ehttp/src/streaming/mod.rs Show resolved Hide resolved
ehttp/src/streaming/mod.rs Show resolved Hide resolved
ehttp/src/streaming/native.rs Outdated Show resolved Hide resolved

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.

ehttp/src/streaming/web.rs Outdated Show resolved Hide resolved
ehttp/src/types.rs Outdated Show resolved Hide resolved
jprochazk and others added 8 commits June 15, 2023 00:14
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
ehttp/src/streaming/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@emilk
Copy link
Owner

emilk commented Jun 15, 2023

You have a failure in cargo test: https://github.com/emilk/ehttp/actions/runs/5272886299/jobs/9541605837

jprochazk and others added 2 commits June 15, 2023 10:04
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@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