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 CopyObject in place deadlock for Suspended buckets #10882

Merged
merged 1 commit into from
May 24, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented May 23, 2024

Motivation

As reported in #10881, we hit a deadlock in S3 when copying in place from a never versioned bucket into a Suspended bucket. This was due to a missing check in the copy in-place detection. We only ever checked if the object did not have a version_id, but version_id = None give the same result as version_id = "null".

This part of the code was then responsible for the deadlock:

if should_copy_in_place(src_bucket, src_object, dest_bucket, dest_object):
    return self.open(dest_bucket, dest_object, mode="r")

with self.open(src_bucket, src_object, mode="r") as src_stored_object:
    dest_stored_object = self.open(dest_bucket, dest_object, mode="w")
    dest_stored_object.write(src_stored_object)

As the copy was not flagged as "in-place" when it should, we would not return early and try to open the same underlying object both in write and read mode 😅
(We had an S3 object with version_id = None and the other was version_id = "null", effectively being the same thing).

Changes

  • added more tests around copy in place behavior in AWS regarding Suspended buckets
  • fixed the condition by adding a small helper method to verify that the object is non-versioned

fixes #10881

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels May 23, 2024
@bentsku bentsku added this to the 3.5 milestone May 23, 2024
@bentsku bentsku self-assigned this May 23, 2024
Copy link

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   3m 10s ⏱️
400 tests 348 ✅  52 💤 0 ❌
800 runs  696 ✅ 104 💤 0 ❌

Results for commit 9601927.

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.

Great catch, thanks for fixing 🚀 🙌

Copy link

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 37m 31s ⏱️ -32s
2 986 tests +1  2 676 ✅ +1  310 💤 ±0  0 ❌ ±0 
2 988 runs  +1  2 676 ✅ +1  312 💤 ±0  0 ❌ ±0 

Results for commit 9601927. ± Comparison against base commit 4da50ab.

@bentsku bentsku merged commit ec7927e into master May 24, 2024
52 checks passed
@bentsku bentsku deleted the fix-s3-copy-in-place-versioning branch May 24, 2024 10:06
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
2 participants