Skip to content

Stream retry support part 1: Add BufferingAsyncRequestBody that buffers the entire content and sup… #6313

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

Open
wants to merge 3 commits into
base: feature/master/mpu-stream-retry
Choose a base branch
from

Conversation

zoewangg
Copy link
Contributor

@zoewangg zoewangg commented Aug 1, 2025

…ports multiple concurrent subscribers

Motivation and Context

#6198

This is the first PR to support retry in stream based transfer in s3 multipart client.

This PR adds BufferingAsyncRequestBody that is essentially a multicast publisher and can be subscribed multiple times until it's been closed

This PR made the following changes to existing ByteBuffersAsyncRequestBody:

  • implement SdkClosable interface to support clearing buffer
  • it's now compliant to the reactive streams spec

@zoewangg zoewangg requested a review from a team as a code owner August 1, 2025 16:30
@zoewangg zoewangg requested a review from Copilot August 1, 2025 16:42
Copy link

@Copilot 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 introduces BufferingAsyncRequestBody, a new implementation of AsyncRequestBody that buffers content in memory and supports multiple concurrent subscribers for stream retry functionality in the AWS SDK.

  • Adds a thread-safe, multicast publisher that can buffer data incrementally and replay it to multiple subscribers
  • Implements comprehensive test coverage including concurrency tests and reactive streams TCK compliance
  • Provides resource management through proper cleanup of buffered data and subscriber notifications

Reviewed Changes

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

File Description
BufferingAsyncRequestBody.java Core implementation of the buffering async request body with multicast publisher capabilities
BufferingAsyncRequestBodyTest.java Comprehensive unit tests covering functionality, edge cases, and thread safety
BufferingAsyncRequestBodyTckTest.java Reactive Streams TCK compliance test suite
Comments suppressed due to low confidence (1)

core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/BufferingAsyncRequestBody.java:208

  • [nitpick] The error message should be consistent with the one used in the subscribe method. Consider using 'AsyncRequestBody has been closed' to match the error message on line 172.
            subscriptions.forEach(s -> s.notifyError(new IllegalStateException("The publisher has been closed")));

Copy link

sonarqubecloud bot commented Aug 1, 2025


@Override
public Optional<Long> contentLength() {
return Optional.ofNullable(length);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of weird to me that contentLength could return an empty optional, but we require all the data to be buffered first before we start publishing. Should we just require all of the data to be ready at construction time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good point! It'll also reduce the complexity

Copy link
Contributor Author

@zoewangg zoewangg Aug 5, 2025

Choose a reason for hiding this comment

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

Removed this new class and updated the existing ByteBuffersAsyncRequestBody to make it compliant to the reactive stream spec and implement closable interface; it already supports multicast today

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.

4 participants