Fixing Java S3 Client Transfer Manager issue with transferComplete() not called for AsyncRequestBody.fromFile

[joviegas]

Fixing Java S3 Client Transfer Manager issue with transferComplete() not called for AsyncRequestBody.fromFile

Motivation and Context

• Fix for issue S3TransferManager: The TransferListener.transferComplete() is not invoked when AsyncRequestBody.fromFile() is used #3839

Root Cause

• NettyRequestExecutor prematurely cancels the subscription when the last chunk is sent.
• The logic of shouldContinuePublishing after DoneCheck-OnNext checks < instead of <= in shouldContinuePublishing().

Modifications

• The actual fix would be to move shouldContinuePublishing before the isDone check and then update < to <= in shouldContinuePublishing().
• However, we had an internal discussion, and the outcome was not to modify this but to update the TransferManager Code to ensure that we send the transferComplete once all the bytes are received.
• Added a done flag to ensure that transferComplete is not called more than once.

Testing

• Added JUnit tests.
• Included integration tests for the Java client since this issue is primarily observed in the Java S3 client Transfer Manager.

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

[cenedhryn]

Can we add a comment to the NettyRequestExecutor.java shouldContinuePublishing() to indicate that the check can cause issues and what the mitigation is?

[joviegas]

Can we add a comment to the NettyRequestExecutor.java shouldContinuePublishing() to indicate that the check can cause issues and what the mitigation is?

I doubt a comment would help much. We should conduct a Failure Mode Analysis to see what happens if we make the suggested fixe by invoking shouldContinuePublishing before done and modifying written < cl to written <= cl . Just adding a comment might make it seem like we've fully evaluated NettyRequestExecutor's behavior with no room for improvement

[cenedhryn]

A deep dive would be ideal, but likely not happening at this point in time. My concern is that after some time the discussion in the PR will be forgotten and we'll lose context and time figuring out the same thing again, for a different issue and that a comment would let us connect the two parts of the code.
Personally I don't care how people would interpret a comment (if they even saw it). As developers we make trade-off decisions every day, and it's not a bad thing to be prudent. That being said, will leave it up to you.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

82.4% Coverage
0.0% Duplication