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

fix(lib-storage): call AbortMultipartUpload when failing to CompleteMultipartUpload #6112

Merged
merged 3 commits into from
May 21, 2024

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented May 20, 2024

Issue

related to #4414

Description

This PR modifies the behavior of @aws-sdk/lib-storage's Upload class' leavePartsOnError: boolean to do what it sounds like it does instead of something meaningless.

Previous to this PR:

  • when true, any error encountered during MPU would be thrown
  • when false, any error encountered during MPU would be ignored
  • default was false

As of this PR:

  • when true, AbortMultipartUpload is not called in the event of a failed MPU
  • when false, AbortMultipartUpload is called in the event of a failed MPU to mark the parts for cleanup as recommended by S3
  • default is still false

Testing

new e2e test

Additional context

Portion of docs from AWS SDK JSv2:

 * ## Handling Multipart Cleanup
 *
 * By default, this class will automatically clean up any multipart uploads
 * when an individual part upload fails. This behavior can be disabled in order
 * to manually handle failures by setting the `leavePartsOnError` configuration
 * option to `true` when initializing the upload object.

behavior (JSv2 source code):

    if (self.service.config.params.UploadId && !self.leavePartsOnError) {
      self.service.abortMultipartUpload().send();
    } else if (self.leavePartsOnError) {
      self.isDoneChunking = false;
    }

This behavior appears to have been lost in #2039

Checklist

  • If you wrote E2E tests, are they resilient to concurrent I/O?

@@ -207,17 +208,17 @@ export class Upload extends EventEmitter {

private async __doConcurrentUpload(dataFeeder: AsyncGenerator<RawDataPart, void, undefined>): Promise<void> {
for await (const dataPart of dataFeeder) {
if (this.uploadedParts.length > this.MAX_PARTS) {
if (this.uploadEnqueuedPartsCount > this.MAX_PARTS) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reason for this change: this.uploadedParts.length is updated after an async operation (UploadPart), which causes a delay.

Due to request concurrency, this.uploadedParts.length can be far behind the actual number of parts that have been iterated and submitted.

@kuhe kuhe merged commit b5288e6 into aws:main May 21, 2024
5 checks passed
@kuhe kuhe deleted the fix/lib-storage branch May 21, 2024 19:21
Copy link

github-actions bot commented Jun 6, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants