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 S3 UploadPart logic when cancelling/error a request #9901

Merged
merged 2 commits into from Dec 19, 2023
Merged

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Dec 18, 2023

Motivation

As reported with #9851, we would face an issue when cancelling/error an UploadPart request and retrying on the same PartNumber.
This was due because of our ephemeral MultipartUpload object is not fully stateless, and keeps the EphemeralS3StoredObject in a dict to allow easier "fusion" of all underlying parts when completing (it does not have to recreate the EphemeralS3StoredObject on top of the LockedSpooledTemporaryFile).
Code here:

def open(self, s3_part: S3Part) -> EphemeralS3StoredObject:
"""
Returns an EphemeralS3StoredObject for an S3Part, allowing direct access to the object. This will add a part
into the Multipart collection. We can directly store the EphemeralS3StoredObject in the collection, as S3Part
cannot be accessed/read directly from the API.
:param s3_part: S3Part object
:return: EphemeralS3StoredObject, most often to directly `write` into it.
"""
if not (stored_part := self.parts.get(s3_part.part_number)):
file = LockedSpooledTemporaryFile(dir=self.upload_dir, max_size=S3_MAX_FILE_SIZE_BYTES)
stored_part = EphemeralS3StoredObject(s3_part, file)
self.parts[s3_part.part_number] = stored_part
return stored_part

However, this created an issue when the request would error/cancel, because the EphemeralS3StoredObject kept the reference to the S3Part that errored (created in the provider operation which raised an Exception, with localstack.services.s3.v3.storage.ephemeral.EphemeralS3StoredMultipart.open). When creating a new S3Part in a non-failing provider operation, the asynchronous calculation of the ETag due to the aws-chunked encoding would not set the ETag to the new S3Part but to the previously created one, hence the returned ETag set to None.

Changes

The simple fix is to simply remove the part entry for the underlying storage if the writing of that part fails. It could maybe be wrapped in a context manager which would clean up only if the operation fails, but this is simple and clear what it does.

I've also added a test to verify the fix, ran the external soto test suite and validated that it now works.

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Dec 18, 2023
@bentsku bentsku self-assigned this Dec 18, 2023
Copy link

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0      2 suites  ±0   3m 15s ⏱️ +3s
382 tests +1  332 ✔️ +1    50 💤 ±0  0 ±0 
764 runs  +2  664 ✔️ +2  100 💤 ±0  0 ±0 

Results for commit 5dc1173. ± Comparison against base commit dd9b2b0.

@bentsku bentsku linked an issue Dec 18, 2023 that may be closed by this pull request
1 task
@coveralls
Copy link

Coverage Status

coverage: 84.036% (-0.006%) from 84.042%
when pulling 5dc1173 on fix-s3-cancel
into dd9b2b0 on master.

Copy link

LocalStack Community integration with Pro

       2 files  ±0         2 suites  ±0   1h 14m 16s ⏱️ + 2m 36s
2 409 tests +1  2 181 ✔️ +1  228 💤 ±0  0 ±0 
2 410 runs  +1  2 181 ✔️ +1  229 💤 ±0  0 ±0 

Results for commit 5dc1173. ± Comparison against base commit dd9b2b0.

@bentsku bentsku requested a review from steffyP December 18, 2023 17:12
Copy link
Member

@steffyP steffyP left a comment

Choose a reason for hiding this comment

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

LGTM 👍 😄
Thanks for the detailed explanation and adding a new test, which will help detect any regression in the future 🚀

@bentsku bentsku merged commit 1b66f9e into master Dec 19, 2023
43 checks passed
@bentsku bentsku deleted the fix-s3-cancel branch December 19, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Second call to S3.UploadPart returns invalid response if first call is cancelled
3 participants