Skip to content

Conversation

@smklein
Copy link
Contributor

@smklein smklein commented Dec 20, 2021

Additionally, update test utils to accept transfer-encoding headers, which are transmitted alongside streams.

@smklein smklein requested a review from davepacheco December 20, 2021 20:59
@smklein
Copy link
Contributor Author

smklein commented Dec 20, 2021

This was inspired by @jclulow 's work in buildomat: https://github.com/oxidecomputer/buildomat/blob/main/server/src/main.rs#L270-L281

It seems like streaming file transfer should be our default example, as it integrates pretty well with Hyper.

I'm attempting to make use of this pattern in oxidecomputer/omicron#457 , but tests are failing without the accepted transfer-encoding header support.

@smklein
Copy link
Contributor Author

smklein commented Dec 21, 2021

Added a couple streaming tests, showing a streaming server, and a client which can either "read the whole response into memory" or opt to receive it piece-by-piece.

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

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

These comments are all nitty.

async fn api_streaming(
_rqctx: Arc<RequestContext<usize>>,
) -> Result<Response<Body>, HttpError> {
let mut file = tempfile::tempfile()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any particular reason to use an actual file instead of, say, a Cursor? Or maybe better: a custom Stream that just emits a fixed number of arbitrary bytes?

It'd be neat to have this stream emit some number of bytes that's likely to be larger than available memory. If the test works, that would validate that we're not at any point buffering the whole contents of the response anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used a file to show integration with the hyper_staticfile::FileBytesStream, since that was a use case that motivated adding this support - we have a file server example in here, but IMO streaming files is a really common use-case that I wanted to show.

@smklein smklein merged commit a9a4614 into main Jan 8, 2022
@smklein smklein deleted the updated-file-serving branch January 8, 2022 01:41
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.

3 participants