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 multipart support #2382

Merged
merged 9 commits into from Dec 12, 2022
Merged

Streaming multipart support #2382

merged 9 commits into from Dec 12, 2022

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Sep 30, 2022

Closes #2249

Something we're not currently handling here, that'd also need updating is how we handle non-rewindable cases.

If the file instance is a non-seekable IOBase then we ought to raise a StreamConsumed error if we attempt to read from the file more than once. This occurs if we attempt to upload a streaming multipart file, but then get a redirect. Everything's fine if we've got a rewindable file instance, but if we've got a non-rewindable case then we need to error out, since we can't repeat the upload at that point.


Edit: My description of this ticket wasn't really sufficient, so...

If we have a multipart file upload but cannot determine it's length, then used chunked transfer encoding, instead of reading the entirety of the file into memory in order to determine the Content-Length.

@tomchristie tomchristie added the enhancement New feature or request label Oct 6, 2022
@adriangb
Copy link
Member

Wouldn't we get the redirect before we start reading files? I would have thought we'd assemble the multipart stuff into an AsyncIterable[bytes] that lazily does work -> if we get redirected we never start pulling from it.

@tomchristie
Copy link
Member Author

tomchristie commented Oct 13, 2022

Wouldn't we get the redirect before we start reading files?

Nope. You send the complete request, then you get the complete response.

If the response is a redirect, then you need to send the complete request again. (Which is fine if you're sending a file since you can just rewind to the start, but isn't recoverable if you're eg. streaming some data from a generator)

@ghost
Copy link

ghost commented Dec 1, 2022

Any update when this will be added?

@tomchristie
Copy link
Member Author

@justinjeffery-ipf This pull request is currently awaiting a review. If you'd like I can send you an invite to the GitHub organisation, which would mean you'd have permission to make approving reviews?

Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

LGTM

@tomchristie tomchristie merged commit b97c059 into master Dec 12, 2022
@tomchristie tomchristie deleted the streaming-multipart-support branch December 12, 2022 16:14
@adriangb
Copy link
Member

Awesome thanks Tom and Michael!

@tomchristie tomchristie mentioned this pull request Dec 20, 2022
@ghost ghost mentioned this pull request Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for streaming multipart/form-data
3 participants