Fix intermittent NPE race in EmittingSubscription when cancel() nulls subscriber during emit

[joviegas]

Fixes #7351

Issue

A customer downloading a small object with a multipart-enabled async S3 client (getObject/downloadFile with multipartEnabled(true)) intermittently gets a NullPointerException instead of their object. The same call succeeds on retry, and the object is small enough to need no multipart at all. It shows up more on fast endpoints and busy hosts, so it hits local testing and CI hardest.

Root cause

EmittingSubscription.downstreamSubscriber is a non-volatile field shared by two threads. For a single-part object, the caller thread runs the emit loop while the SDK response-completion thread, seeing partsCount == null, calls cancel(), which sets the field to null. The loop checks isCancelled before onNext, but the cancel can land in the gap between that check and the dereference, so onNext is called on a null reference and throws.

Fix

EmittingSubscription: make downstreamSubscriber final and stop nulling it in cancel(). Cancellation is already signaled by the existing isCancelled flag, so the reference no longer needs to be cleared. A final reference cannot be observed as null.

FileAsyncResponseTransformerPublisher (its only user, same defect one frame away): this class is the Publisher, so Reactive Streams rule 3.13 requires it to drop the subscriber on cancel. It keeps nulling, but the field is now volatile and every read snapshots it into a local and null-checks before use.

ParallelMultipartDownloaderSubscriber: the subscriber called request() (line 201) and cancel() (line 355) from two threads without synchronizing them, which Reactive Streams rule 2.7 requires. Both calls now hold the existing subscriptionLock.

Testing

• New EmittingSubscriptionTest: demand/emit, non-positive demand, cancel, and a concurrency test racing cancel() against the emit loop. The concurrency test fails on the old code and passes on the fix.
• Existing FileAsyncResponseTransformerPublisher unit tests and its Reactive Streams TCK suite pass, including the rule 3.13 test that verifies the publisher drops the subscriber on cancel (the behavior the publisher change touches).
• Existing multipart download tests and the ParallelMultipartDownloaderSubscriber TCK pass with the lock change.

Testing

• New EmittingSubscriptionTest: demand/emit, non-positive demand, cancel, and a concurrency test racing cancel() against the emit loop. The concurrency test fails on the old code and passes on the fix.
• Existing FileAsyncResponseTransformerPublisher unit tests and its Reactive Streams TCK suite pass, including the rule 3.13 test that verifies the publisher drops the subscriber on cancel (the behavior the publisher change touches).

Screenshots (if appropriate)

Types of changes

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

License

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

[zoewangg]

request and cancel shouldnt run concurrently though. The subscriber should make sure they run synchronously

[joviegas]

Good call out. This java doc comment can be misleading. So removing it

[zoewangg]

• Should we handle additional onNext signals after cancel in ParallelMultipartDownloaderSubscriber per Reactive Streams rule 2.8?
• It seems we should also add lock when we invoke subscription.cancel(); in ParallelMultipartDownloaderSubscriber

[joviegas]

Should we handle additional onNext

Extra onNext signals are already no-op'd here:

aws-sdk-java-v2/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/ParallelMultipartDownloaderSubscriber.java

Lines 224 to 229 in 3d2b09c

	totalPartsFuture.thenAccept(
	totalParts -> {
	if (currentPartNum <= totalParts) {
	processingRequests(asyncResponseTransformer, currentPartNum, totalParts);
	}
	});

Each onNext gets a unique increasing part number from nextPart(),and for a single-part object totalParts is 1, so anything past part 1 falls outside currentPartNum <= totalParts and is dropped. Do you think we need any additional handling beyond this check?

[joviegas]

It seems we should also add lock

Added lock to cancel and request now

[github-actions]

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.