Skip to content

fix(#433): stream multipart uploads without buffering files#505

Open
ManuelReschke wants to merge 1 commit into
imroc:masterfrom
ManuelReschke:master
Open

fix(#433): stream multipart uploads without buffering files#505
ManuelReschke wants to merge 1 commit into
imroc:masterfrom
ManuelReschke:master

Conversation

@ManuelReschke

Copy link
Copy Markdown

This change makes multipart uploads stream directly to the HTTP transport instead of building the complete multipart body in a bytes.Buffer first.

  • Stream multipart bodies through io.Pipe, keeping memory usage bounded for large uploads.
  • Preserve an exact Content-Length for SetFile and SetFileBytes by counting multipart framing bytes and adding the known file sizes without reading the file contents into memory.
  • Fall back to an unknown content length for sources whose size is not known, while still streaming their contents.
  • Create a fresh multipart stream for each request attempt so file uploads remain replayable for redirects and retries.
  • Reopen files created with SetFile for each attempt and inspect only the first 512 bytes when detecting their content type.
  • Mark SetFileReader bodies as non-replayable, preventing retries from silently sending an exhausted reader.
  • Propagate multipart field, file, and pipe write errors to the HTTP transport instead of discarding them.

Content-Length behavior

  • SetFile and non-empty SetFileBytes uploads retain an exact Content-Length.
  • Custom uploads with both FileSize and ContentType set also retain an exact Content-Length.
  • Uploads with an unknown size use an unknown content length and are streamed without buffering.
  • Explicit force-chunked uploads keep their existing unknown-length behavior.

Validation performed:

go test ./...
go vet ./...
git diff --check

@imroc imroc left a comment

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.

Approved. This is a well-engineered fix for #433. I verified locally (go build ./... and the full root-package go test ./... — all green) since CI checks are not reported on this fork PR (workflow approval required).

Correctness points confirmed:

  • Content-Length accuracy: multipartContentLength counts the multipart framing plus the known file sizes, and uses the same file.ContentType that writeMultipartFormFile applies (middleware.go), so the declared Content-Length exactly matches the streamed bytes for SetFile / SetFileBytes. For SetFileReader (empty Content-Type) it correctly falls back to unknown length -> chunked encoding. No length mismatch on the wire.
  • Replayability: GetBody (handleMultiPart) builds a fresh io.Pipe + multipart writer per attempt; SetFile reopens the file and SetFileBytes yields a fresh reader each call, so redirects/retries stay replayable. SetFileReader is correctly marked non-replayable via r.unReplayableBody and guarded by the existing retry check (request.go, errRetryableWithUnReplayableBody).
  • Bounded memory: r.Body stays nil (middleware.go, handleMultiPart) and the body streams through the pipe, fixing the large-upload buffering from #433.
  • Pipe write errors are propagated via pw.CloseWithError instead of being discarded.
  • The 3 new tests (streaming+Content-Length, unknown-size reader, non-replayable retry) pass.

Non-blocking observations:

  • An empty SetFile (FileSize==0) falls back to unknown Content-Length because multipartContentLength treats FileSize<=0 as unknown. Acceptable edge case, just noting it.
  • The throwaway multipart.NewWriter(io.Discard) in handleMultiPart exists only to derive the boundary + content type; harmless.

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