Skip to content

Streaming fetch#28

Merged
emilk merged 18 commits intoemilk:masterfrom
jprochazk:master
Jun 15, 2023
Merged

Streaming fetch#28
emilk merged 18 commits intoemilk:masterfrom
jprochazk:master

Conversation

@jprochazk
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

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>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@emilk
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

Not sure what to do about the cargo deny failure

@emilk
Copy link
Copy Markdown
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