Fixed the issue where the SDK did not offload future completion

[zoewangg]

Motivation and Context

Fixed the issue where the SDK did not offload future completion to the future completion executor

RCA

Happy case workflow (Always complete on futureCompletionExecutor)

• responseFuture gets completed by futureCompletionExecutor after the future in the AsyncResponseHandler#prepare gets completed

aws-sdk-java-v2/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

Lines 210 to 216 in e4c4612

	responseHandlerFuture.whenCompleteAsync((r, t) -> {
	if (t == null) {
	responseFuture.complete(r);
	} else {
	responseFuture.completeExceptionally(t);
	}
	}, futureCompletionExecutor);

• toReturn future gets completed after responseFuture gets completed. This will happen in the futureCompletionExecutor.

aws-sdk-java-v2/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

Lines 103 to 108 in e4c4612

	executeFuture.whenComplete((r, t) -> {
	if (t != null) {
	toReturn.completeExceptionally(t);
	} else {
	toReturn.complete(r);
	}

Error case workflow (Most of the time complete on futureCompletionExecutor)

• responseFuture gets completed exceptionally before responseHandlerFuture gets completed.

aws-sdk-java-v2/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

Lines 158 to 160 in e4c4612

	public void onError(Throwable error) {
	responseFuture.completeExceptionally(error);
	wrappedHandler.onError(error);

• toReturn future gets completed after responseFuture gets completed. This will happen in the event loop thread due to the use of whenComplete.

aws-sdk-java-v2/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/MakeAsyncHttpRequestStage.java

Lines 103 to 108 in e4c4612

	executeFuture.whenComplete((r, t) -> {
	if (t != null) {
	toReturn.completeExceptionally(t);
	} else {
	toReturn.complete(r);
	}

Modifications

• Removed WrappedErrorForwardingResponseHandler. I'm not sure why it's needed since we are relying on responseHandlerFuture to complete the responseFuture anyway.
  - Updated to offload the metrics reporting to futureCompletionExecutor

Testing

Added unit tests

Screenshots (if appropriate)

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 install 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. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
• My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

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

[cenedhryn]

Curious, did the tests fail consistently before adding the fix?

[zoewangg]

Curious, did the tests fail consistently before adding the fix?

No, I think the test started to fail because I made the change to offload metrics reporting to the thread completion executor. I'll either revert the change or fix the test

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication