Skip to content

Fix SimplePublisher allowing re-subscription after cancellation#6858

Open
zoewangg wants to merge 1 commit intomasterfrom
zoewang/simplePublisherFix
Open

Fix SimplePublisher allowing re-subscription after cancellation#6858
zoewangg wants to merge 1 commit intomasterfrom
zoewang/simplePublisherFix

Conversation

@zoewangg
Copy link
Copy Markdown
Contributor

@zoewangg zoewangg commented Apr 13, 2026

Motivation and Context

When a getObject ResponsePublisher is used as a putObject request body and the server returns a retryable error (e.g., 500), the SDK retries and re-subscribes to the same publisher chain. The SimplePublisher backing the ResponsePublisher (used by CRT HTTP client) had two bugs that caused the retry to hang:

  1. subscribe() was missing a return after rejecting a duplicate subscriber, causing it to fall through and replace the subscriber reference.
  2. On cancellation (triggered by ContentLengthAwareSubscriber.java#L60 after all bytes are consumed), subscriber was set to null, which allowed a new subscriber to be accepted into an exhausted stream with no data or terminal signal.

Modifications

  • SimplePublisher.subscribe(): Added missing return after the duplicate subscriber error branch.
  • SimplePublisher cancel handling: Removed subscriber = null on cancellation. The subscriber reference is kept so that re-subscription is properly rejected by the existing subscriber != null guard.
  • AddingTrailingDataSubscriber.onSubscribe(): Propagate onError to downstream on duplicate subscription instead of silently swallowing it (defensive fix).

Testing

  • SimplePublisherTest: Added subscribeAfterCancel_rejectsWithError and subscribeAfterCancelAndDataConsumed_rejectsWithError

  • AddingTrailingDataSubscriberTest: Added duplicateOnSubscribe_shouldPropagateErrorToDownstream

  • PutObjectRequestBodyFromSimplePublisherRetryTest: End-to-end test using WireMock — verifies putObject retry with a SimplePublisher-backed request body completes promptly instead of hanging

  • Added unit tests

  • Ran existing tests

  • All new and existing tests passed

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn clean install -pl :utils succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry

License

  • I confirm that this pull request can be released under the Apache 2 license

@zoewangg zoewangg requested a review from a team as a code owner April 13, 2026 21:20
@@ -284,8 +286,6 @@ private void doProcessQueue() {
break;
case CANCEL:
failureMessage.trySet(() -> new CancellationException("subscription has been cancelled."));

subscriber = null; // Allow subscriber to be garbage collected after cancellation.
Copy link
Copy Markdown
Contributor Author

@zoewangg zoewangg Apr 13, 2026

Choose a reason for hiding this comment

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

Subscribe gets GC'd when the SimplePublisher itself is GC'd

@zoewangg zoewangg added the no-api-surface-area-change Indicate there is no API surface area change and thus API surface area review is not required label Apr 13, 2026
@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-api-surface-area-change Indicate there is no API surface area change and thus API surface area review is not required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants