Skip to content

Conversation

@TaQuanMinhLong
Copy link

@TaQuanMinhLong TaQuanMinhLong commented Dec 26, 2024

Hi, I think ResponseDataStream is missing the implementation of AsyncRead trait, which allows us to pass the stream to a reader instead of putting the whole file in memory


This change is Reviewable

@durch durch changed the base branch from master to 0.37.0-alpha.1 September 16, 2025 18:10
@durch durch requested a review from Copilot September 16, 2025 18:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds an AsyncRead trait implementation for ResponseDataStream to enable streaming data to readers without loading entire files into memory. This is useful for processing large S3 objects efficiently.

  • Implements tokio::io::AsyncRead for ResponseDataStream under the with-tokio feature flag
  • Updates the with-tokio feature dependency to include futures/alloc for the implementation

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
s3/src/request/request_trait.rs Adds AsyncRead implementation for ResponseDataStream with proper error handling and buffer management
s3/Cargo.toml Updates with-tokio feature to include futures/alloc dependency

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

let bytes = self.get_mut().bytes();
match bytes.poll_next_unpin(cx) {
std::task::Poll::Ready(Some(Ok(chunk))) => {
buf.put_slice(&chunk); // Put the chunk into the buffer
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

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

The implementation doesn't check if the chunk size exceeds the buffer's remaining capacity. This could cause a panic if the chunk is larger than the available buffer space. Consider using buf.remaining() to check capacity and handle partial writes appropriately.

Copilot uses AI. Check for mistakes.
std::task::Poll::Ready(Some(Err(error))) => {
std::task::Poll::Ready(Err(std::io::Error::new(std::io::ErrorKind::Other, error)))
}
std::task::Poll::Ready(None) => std::task::Poll::Ready(Ok(())),
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

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

When the stream ends (None), the implementation should signal EOF by returning 0 bytes read. The current implementation returns Ok(()) but doesn't indicate to the reader that no more data is available. Consider checking if any bytes were written to the buffer and handle EOF correctly according to AsyncRead semantics.

Suggested change
std::task::Poll::Ready(None) => std::task::Poll::Ready(Ok(())),
// If no bytes were written to the buffer in this poll, signal EOF by returning Ok(())
std::task::Poll::Ready(None) => {
// If the buffer was not filled in this poll, this signals EOF (0 bytes read)
std::task::Poll::Ready(Ok(()))
},

Copilot uses AI. Check for mistakes.
@durch
Copy link
Owner

durch commented Sep 16, 2025

@TaQuanMinhLong good call out, but I'll implement it a bit differently, there are some issues here, and a lack of tests, but good callout

@durch durch closed this Sep 16, 2025
durch added a commit that referenced this pull request Sep 16, 2025
Implements AsyncRead trait for ResponseDataStream to enable streaming S3 objects
without loading them entirely into memory. This allows use of standard Rust I/O
utilities like tokio::io::copy() for more efficient and ergonomic file transfers.

- Add tokio::io::AsyncRead implementation for tokio runtime
- Add async_std::io::Read implementation for async-std runtime
- Add comprehensive tests for both implementations
- Add surf to with-async-std feature for proper async-std support

This addresses the issue raised in PR #410 and enables memory-efficient streaming
for large S3 objects.
durch added a commit that referenced this pull request Sep 16, 2025
Implements AsyncRead trait for ResponseDataStream to enable streaming S3 objects
without loading them entirely into memory. This allows use of standard Rust I/O
utilities like tokio::io::copy() for more efficient and ergonomic file transfers.

- Add tokio::io::AsyncRead implementation for tokio runtime
- Add async_std::io::Read implementation for async-std runtime
- Add comprehensive tests for both implementations
- Add surf to with-async-std feature for proper async-std support

This addresses the issue raised in PR #410 and enables memory-efficient streaming
for large S3 objects.
durch added a commit that referenced this pull request Sep 16, 2025
* Kick off 0.37.0 version, update dependencies, clippy lints

* fix(s3): exclude content-length for ListBuckets (#428)

* Switched futures to futures-util (#426)

Co-authored-by: Mohammed Jassim Alkarkhi <[email protected]>

* feat: Add builder pattern for PUT operations with custom headers

Implements a flexible builder pattern for S3 PUT operations that allows
setting custom headers, content type, and metadata on a per-request basis.
This replaces the approach from PR #425 with a more extensible design.

Key changes:
- Add PutObjectRequest builder for regular uploads with custom headers
- Add PutObjectStreamRequest builder for streaming uploads
- Support custom headers including Cache-Control, Content-Encoding, metadata
- Integrate with existing multipart upload flow for large files
- Use Result types for builder methods to handle invalid header values

The builder pattern provides a cleaner API that avoids method proliferation
while maintaining backward compatibility with existing PUT methods.

* Fix Bucket::get_object_range_to_writer() sync impl (#413)

* R2: EU jurisdiction endpoint (#409)

* Correct R2/Custom region comments

* feat(region): add Cloudflare R2 EU jurisdiction support

Add new R2Eu variant to Region enum to support Cloudflare R2's EU
jurisdiction endpoint with format "{account_id}.eu.r2.cloudflarestorage.com"

* Update aws-region/src/region.rs

Co-authored-by: Copilot <[email protected]>

---------

Co-authored-by: Drazen Urch <[email protected]>
Co-authored-by: Copilot <[email protected]>

* bucket: ensure Bucket::exists() honours 'dangereous' config (#415)

* bucket: ensure Bucket::exists() honours 'dangereous' config

* Update s3/src/bucket.rs

Co-authored-by: Copilot <[email protected]>

---------

Co-authored-by: Drazen Urch <[email protected]>
Co-authored-by: Copilot <[email protected]>

* refactor: restructure Makefiles to run fmt and clippy before tests

- Separated formatting, linting, and testing phases in build process
- fmt and clippy now run for all feature combinations first
- Tests run only after successful formatting and linting checks
- Improved CI workflow efficiency by catching style issues early

* fix: ensure Bucket::exists() honors dangerous SSL config (PR #415)

- Renamed list_buckets_ to _list_buckets for consistency
- Fixed error variant configuration for InvalidHeaderName
- Added test coverage for dangerous SSL config with exists() method

* fix: add RUST_S3_SKIP_LOCATION_CONSTRAINT env var for LocalStack compatibility

Fixes #411 by allowing users to skip sending location constraints in bucket
creation requests. LocalStack and some other S3-compatible services don't
support location constraints in the request body and return
InvalidLocationConstraint errors.

Users can now set RUST_S3_SKIP_LOCATION_CONSTRAINT=true (or 1) to skip
sending the location constraint when creating buckets.

* feat: add AsyncRead implementation for ResponseDataStream

Implements AsyncRead trait for ResponseDataStream to enable streaming S3 objects
without loading them entirely into memory. This allows use of standard Rust I/O
utilities like tokio::io::copy() for more efficient and ergonomic file transfers.

- Add tokio::io::AsyncRead implementation for tokio runtime
- Add async_std::io::Read implementation for async-std runtime
- Add comprehensive tests for both implementations
- Add surf to with-async-std feature for proper async-std support

This addresses the issue raised in PR #410 and enables memory-efficient streaming
for large S3 objects.

* Prepare for PR #407 merge - update dependencies and fix formatting

* fix: implement bounded parallelism for multipart uploads to prevent memory exhaustion

- Adds dynamic memory-based concurrency control for multipart uploads
- Uses FuturesUnordered to maintain a sliding window of concurrent chunk uploads
- Calculates optimal concurrent chunks based on available system memory (2-10 chunks)
- Prevents memory exhaustion issue where all chunks were loaded before uploading (fixes #404)
- Adds optional sysinfo dependency, only enabled with async features
- Maintains backward compatibility and performance through bounded parallelism

This solution fixes the "Broken pipe" errors that occurred with large file uploads
by ensuring we only keep a limited number of chunks in memory at once, while still
maintaining good upload performance through parallel chunk uploads.

* perf: increase max concurrent chunks from 10 to 100 for better performance

The previous limit of 10 concurrent chunks was too conservative and caused
significant performance degradation. Increasing to 100 allows better
parallelism while still maintaining memory-based limits.

* fix: correct delete_bucket_lifecycle to use DeleteBucketLifecycle command

The delete_bucket_lifecycle method was incorrectly using Command::DeleteBucket
instead of Command::DeleteBucketLifecycle, which could have caused entire buckets
to be deleted instead of just their lifecycle configuration.

Closes #414

* docs: clarify ETag handling in response_data for PUT operations

Added detailed comments explaining why the library extracts ETag from headers
and returns it as the response body when etag=true. This behavior is used for
PUT operations where S3 returns empty or non-useful response bodies, and the
calling code needs easy access to the ETag value.

Added TODO comments for future refactoring to properly handle response bodies
and ETags separately, though this would be a breaking change.

Closes #430

* fix: remove trailing slashes from custom endpoints to prevent signature mismatches

Custom endpoints with trailing slashes (like 'https://s3.gra.io.cloud.ovh.net/')
were causing 403 Signature Mismatch errors. The trailing slash was being included
in the host header, causing signature validation to fail.

This fix ensures that trailing slashes are stripped from custom endpoints,
making the behavior consistent with AWS CLI and other SDKs.

Added comprehensive tests to verify the fix handles various edge cases including
multiple trailing slashes and endpoints with ports.

Closes #429

* fix: preserve standard ports in presigned URLs for signature validation

When custom endpoints explicitly specify standard ports (80 for HTTP, 443 for HTTPS),
these ports must be preserved in presigned URLs. The URL crate's to_string() method
omits standard ports by design, but this causes signature validation failures when
servers expect the port in the Host header.

This fix rebuilds the presigned URL string with the original host (including port)
when a custom region explicitly specifies a standard port. Non-standard ports
continue to work as before since they are naturally preserved by URL parsing.

Added comprehensive tests to verify correct behavior for both standard and
non-standard ports.

Closes #419

* Bump versions, fmt

---------

Co-authored-by: George <[email protected]>
Co-authored-by: Mohammed Jassim Alkarkhi <[email protected]>
Co-authored-by: Mohammed Jassim Alkarkhi <[email protected]>
Co-authored-by: Jasin Bushnaief <[email protected]>
Co-authored-by: Ruben De Smet <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: whitty <[email protected]>
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