Bigstring body - #52
Open
toots wants to merge 4 commits into
Open
Conversation
head now returns the object's headers as content.response_headers, with find_header to look one up without minding the case the server chose, which covers content-type and anything else the API does not model. ls parses an xml listing that carries no per-object headers, so the field is None there rather than an empty list.
Only a reader reaching a flush marker resolves the flush that queued it, so a producer that bounds itself on flush is never woken once its consumer stops reading. The expect-rejected path does exactly that: it skips send_body, and nothing closed the body reader either.
An object body is the largest thing a caller moves through this library and the one it never looks inside, so Body.t gains a Bigstring case and put_bigstring / get_bigstring / Multipart_upload.upload_part_bigstring sit beside the string forms rather than replacing them. A request body reaches the socket as slices off the bigarray instead of one object-sized string, bounded one slice ahead of the consumer as chunk_writer already does, and its sigv4 payload hash is taken over the bigarray directly. A response is gathered into a single off-heap buffer as the fragments arrive, where to_string holds every fragment and then concatenates them. The cli grows a --bigstring flag so integration.sh drives both spellings, each direction checked against the other's.
Reading a file into a Bytes the size of the body, and writing one back out the same way, puts the whole object on the heap that --bigstring exists to keep it off, which made the flag measure as worse than the string path it replaces.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multipart CLI mode ignores the bigstring option, and the public API contract and scope need clarification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
aws-s3/s3.mli — This adds an unrelated source-breaking change to the public content record: downstream code that… |
 |
aws-s3/s3.mli — Because Bigstringaf.t is mutable, merely keeping its bytes valid is insufficient: the payload is… |
|
cli/aws.ml — use_bigstring is silently ignored whenever use_multi=true: the earlier multipart branch still… |
What changed in this PR
Adds off-heap bigstring support for S3 request and response bodies, including CLI integration and transport cancellation handling.
Changes:
- Adds bigstring upload, download, signing, and multipart APIs.
- Adds CLI bigstring transfers and integration coverage.
- Exposes response headers from
headresults.
| File | Description |
|---|---|
integration.sh |
Tests bigstring transfers and build failures. |
cli/dune |
Adds Bigstringaf dependency. |
cli/cli.ml |
Adds the --bigstring option. |
cli/aws.ml |
Implements bigstring file transfers. |
aws-s3/s3.mli |
Exposes bigstring and response-header APIs. |
aws-s3/s3.ml |
Implements the public APIs. |
aws-s3/http.ml |
Cancels unused body producers. |
aws-s3/dune |
Links Bigstringaf. |
aws-s3/body.mli |
Declares bigstring bodies and sinks. |
aws-s3/body.ml |
Implements off-heap response accumulation. |
aws-s3/aws.ml |
Streams, sizes, and signs bigstrings. |
aws-s3/authorization.mli |
Declares bigstring hashing. |
aws-s3/authorization.ml |
Implements bigstring hashing. |
aws-s3.opam |
Adds the Bigstringaf dependency. |
aws-s3-lwt/io.ml |
Releases flush waiters during closure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| last_modified : float; (** Seconds since epoch *) | ||
| key : string; | ||
| etag : etag; (** Etag as a string. this us usually the MD5, unless the object was constructed by multi-upload *) | ||
| response_headers : (string * string) list option; |
Comment on lines
+104
to
+107
| (** {!put} with a body that is never held on the OCaml heap. | ||
|
|
||
| [data] is read for the duration of the request rather than copied up | ||
| front, so its bytes must stay valid until the result is determined. |
| upload_parts t endpoint ~retries ~expect ~credentials ~offset:(offset + size) ~total ~part_number:(part_number + 1) ?chunk_size src | ||
|
|
||
| let cp profile endpoint ~retries ~expect ~confirm_requester_pays ?(use_multi=false) ?first ?last ?chunk_size src dst = | ||
| let cp profile endpoint ~retries ~expect ~confirm_requester_pays ?(use_multi=false) ?(use_bigstring=false) ?first ?last ?chunk_size src dst = |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for sending and receiving requests body as bigarray data.