Skip to content
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

Add stability test case to test issue #4608 #4757

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Dec 8, 2023

Motivation and Context

Test #4608

Root cause of #4608

The issue was fixed with revert #4621 and actual fix #4620
Following was the root cause

  1. This issue is specific when we have S3 API calls with and without checksum back to back.
  2. The Race condition occurs in This code

       return headerChecksumSpecs != null &&
              headerChecksumSpecs.algorithm() != null &&
              !HttpChecksumUtils.isHttpChecksumPresent(interceptorContext.httpRequest(), headerChecksumSpecs) &&
              HttpChecksumUtils.isUnsignedPayload(
                  context.executionAttributes().getAttribute(SIGNING_METHOD), interceptorContext.httpRequest().protocol(),
                  isContentStreaming) &&
              !headerChecksumSpecs.isRequestStreaming();
  1. ChecksumThread Has a checksum because Its CHECKSUM_SPECS was updated with non null values here thus its headerChecksumSpecs !=null passes and it goes to HttpChecksumUtils.isHttpChecksumPresent(interceptorContext.httpRequest(), headerChecksumSpecs)
  2. Now when a Non Checksum API in another NonChecksumThread acccess the RESOLVED_CHECKSUM_SPECS its header and other attributes are reset to null because no checksum for that API
  3. When the API in ChecksumThread resumes , its in call where it does DefaultSdkHttpFullRequest.firstMatchingHeader where it expects the string to be nonNull for a String compare to find the header in a Tree Map, thus ends up with NPE

Modifications

  • Added a test case in stability test to send Request with and without checksum back to back

Testing

  • Made sure that test is valid by checking on release which had the issue
  • git checkout d9cd9d3 and tested issue is recreated with the stabilty test added

License

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

@joviegas joviegas requested a review from a team as a code owner December 8, 2023 17:47
@cenedhryn
Copy link
Contributor

There are dependency issues with module stability-tests. Did you import something from a different module that's not in the pom?

IntFunction<CompletableFuture<?>> future = i -> {
String keyName = computeKeyName(i);

return testClient.putObject(b -> b.bucket(getTestBucketName())
Copy link
Contributor

@zoewangg zoewangg Dec 8, 2023

Choose a reason for hiding this comment

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

There's a chance that all concurrent requests are using the same checksum algorithm, can we pick a checksum algorithm based on index, like index % 4 and each one correlates to one of the supported checksum algorithms? This way, we can remove thenRunAsync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@joviegas
Copy link
Contributor Author

joviegas commented Dec 8, 2023

There are dependency issues with module stability-tests. Did you import something from a different module that's not in the pom?

Yeap , it was caused by StaticCredentialProvider

@cenedhryn
Copy link
Contributor

Just to confirm - the test fails on the code that has the bug, and it passes using master?

Copy link

sonarcloud bot commented Dec 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@joviegas
Copy link
Contributor Author

joviegas commented Dec 11, 2023

Just to confirm - the test fails on the code that has the bug, and it passes using master?

Yeap , (mentioned in the the testing section). Done stability test in PR checks

@joviegas joviegas merged commit 399a5c0 into master Dec 12, 2023
15 checks passed
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.

3 participants