-
Notifications
You must be signed in to change notification settings - Fork 960
Fix bug in FileAsyncRequestBody where inflight parts were negative #6564
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
base: master
Are you sure you want to change the base?
Conversation
| // will never be invoked, and if the current buffer is full, the publisher will stop | ||
| // sending new FileAsyncRequestBody, leading to uncompleted future. | ||
| .doAfterOnCancel(() -> startNextRequestBody(simplePublisher)) | ||
| .doAfterOnCancel(() -> startNextRequestBody(simplePublisher, position)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another possible fix, to avoid keep track of inflight in a collection, would be to have a flag in the FileAsyncRequestBodyWrapper that tracks if the next request was started or not, and call startNextRequestBody only when it was not.
But both feels a little bit like a bandaid solution, is there a way to completely avoid the multiple calls to the startNextRequestBody?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - that was my fix - which seems to be working.
I'm not sure there is a way for us to avoid multiple onCancel being called - and I'm not sure that we'd ever want to make the assumption either - the Reactive Stream Spec (rule 3.7) requires canancls after the first to be nops:
After the Subscription is cancelled, additional Subscription.cancel() MUST be NOPs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on reactive specs, yeah you are correct we should make sure anyways it is NOOP for multiple calls
|
|
s3-regression-tests-upload-async keeps failing with timeouts on MRAP tests: These failures happen for all body types (see above example where body type is BUFFERS_REMAINING) - these non file type bodies don't use the FileAsycRequestBody that is being changed. Possibly an MRAP related issue - will retry later. |



Fix bug in FileAsyncRequestBody where inflight parts were negative
Motivation and Context
Fix bug in S3 Multipart uploads with FileAsyncRequestBody - ensure that concurrency is limited correctly by bufferSizeInBytes.
Fixes: #6539
Modifications
The ContentLengthAwareSubscriber calls
cancelon the subscription (see code linked) which can result in multiple calls to thestartNextRequestBodyfor the same part - causing the old numAsyncRequestBodiesInFlight to be decremented multiple times and leading to it being negative.Testing
Updated unit tests (added additional condition) + manual test.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License