Repository: getmoto/moto

Issue:
S3 backend throwing 500 on versioned bucket
This is an injection that happened between 4.0.5 + 4.0.8.  The issue is due to line https://github.com/spulec/moto/blob/master/moto/s3/models.py#L1616 which is doing a deep copy of something it should not be deep copying as the structure is complicated with locks and what not.  In certain scenarios the file will be closed causing it to throw an exception.  See https://github.com/spulec/moto/issues/5341#issuecomment-1287429197


Discussion:
This is the code I had which was triggering this issue:
```python
            for bucket in self._buckets:
                paginator = self._s3_client.get_paginator('list_object_versions')
                async for page in paginator.paginate(Bucket=bucket):
                    for s3_obj in itertools.chain(page.get('Versions', empty_tuple), page.get('DeleteMarkers', empty_tuple)):
                        await self._s3_client.delete_object(Bucket=bucket, Key=s3_obj['Key'], VersionId=s3_obj['VersionId'])
```

just the pagination itself caused it to return a 500 and caused aiobotocore to keep retrying the request, and causing an apparent hang.

I created the bucket as followed:
```
        for bucket in self._buckets:
            await self._s3_client.create_bucket(
                Bucket=bucket,
                CreateBucketConfiguration={"LocationConstraint": os.environ['AWS_DEFAULT_REGION']}
            )
            await self._s3_client.put_bucket_versioning(Bucket=bucket, VersioningConfiguration={'Status': 'Enabled'})
```

and added a few files to it before attempting to paginate it and delete what was left in the bucket.
Hi @thehesiod, I can't reproduce this, I'm afraid. This is my attempt:
```
import asyncio
import aiobotocore
import boto3
import botocore
import itertools
from aiobotocore.session import get_session
from moto import mock_s3


class PatchedAWSResponse:
    def __init__(self, response: botocore.awsrequest.AWSResponse):
        self._response = response
        self.status_code = response.status_code
        self.raw = response.raw
        self.raw.raw_headers = {}

    @property
    async def content(self):
        return self._response.content

def factory(original):
    def patched_convert_to_response_dict(http_response, operation_model):
        return original(PatchedAWSResponse(http_response), operation_model)

    return patched_convert_to_response_dict

aiobotocore.endpoint.convert_to_response_dict = factory(aiobotocore.endpoint.convert_to_response_dict)
empty_tuple = ()

@mock_s3
def test():
    loop = asyncio.get_event_loop()
    loop.run_until_complete(deleteall())
    loop.close()


async def deleteall():
    session = get_session()

    async with session.create_client('s3', region_name='us-east-1') as client:
        bucket = "bucketx"
        await client.create_bucket(Bucket=bucket)
        await client.put_bucket_versioning(Bucket=bucket, VersioningConfiguration={'Status': 'Enabled'})

        for i in range(100):
            await client.put_object(Bucket=bucket, Key=f"key{i}", Body="sth")

        paginator = client.get_paginator('list_object_versions')
        async for page in paginator.paginate(Bucket=bucket):
            for s3_obj in itertools.chain(page.get('Versions', empty_tuple), page.get('DeleteMarkers', empty_tuple)):
                await client.delete_object(Bucket=bucket, Key=s3_obj['Key'], VersionId=s3_obj['VersionId'])
```
That's on master, with werkzeug 2.2.2 and the latest versions of aiobotocore etc.
btw I'm using the server and not the patch.  Lemme c if I can get a reproable use-case
hah tricky, simple test case doesn't work
I just saw the stacktrace that you posted - looks like the GC PR's that we merged are the rootcause: https://github.com/spulec/moto/pull/5556/files

TLDR: S3 opens filehandles automatically when creating a `FakeKey()`-object, but we never close them (until now). These PR's were raised to close them explicitly everytime a FakeKey is deleted.

Running an async delete probably looks like this:
 - thread 1: call delete_file
 - thread 1: close file handle on the file
 - thread 2: deep copy that exact file
 - thread 1: remove the file from the bucket
 - thread 2: boom

So Moto should ensure that either 1) we close/delete in a single operation, or 2) we delete first to ensure that no-one can reach it - and then close the filehandle.
i just saw that it's related to multipart
boom https://gist.github.com/thehesiod/31504dd47505f38b59b3e7a238370201 :)
Awesome - thanks for putting that together @thehesiod!

Patch:
diff --git a/moto/s3/models.py b/moto/s3/models.py
--- a/moto/s3/models.py
+++ b/moto/s3/models.py
@@ -305,7 +305,12 @@ def expiry_date(self):
     # https://docs.python.org/3/library/pickle.html#handling-stateful-objects
     def __getstate__(self):
         state = self.__dict__.copy()
-        state["value"] = self.value
+        try:
+            state["value"] = self.value
+        except ValueError:
+            # Buffer is already closed, so we can't reach the data
+            # Only happens if the key was deleted
+            state["value"] = ""
         del state["_value_buffer"]
         del state["lock"]
         return state


Test patch:
diff --git a/tests/test_s3/test_s3_file_handles.py b/tests/test_s3/test_s3_file_handles.py
--- a/tests/test_s3/test_s3_file_handles.py
+++ b/tests/test_s3/test_s3_file_handles.py
@@ -1,3 +1,4 @@
+import copy
 import gc
 import warnings
 from functools import wraps
@@ -186,3 +187,21 @@ def test_delete_specific_version(self):
         self.s3.delete_object(
             bucket_name="my-bucket", key_name="my-key", version_id=key._version_id
         )
+
+
+def test_verify_key_can_be_copied_after_disposing():
+    # https://github.com/spulec/moto/issues/5588
+    # Multithreaded bug where:
+    #  - User: calls list_object_versions
+    #  - Moto creates a list of all keys
+    #  - User: deletes a key
+    #  - Moto iterates over the previously created list, that contains a now-deleted key, and creates a copy of it
+    #
+    # This test verifies the copy-operation succeeds, it will just not have any data
+    key = s3model.FakeKey(name="test", bucket_name="bucket", value="sth")
+    assert not key._value_buffer.closed
+    key.dispose()
+    assert key._value_buffer.closed
+
+    copied_key = copy.copy(key)
+    copied_key.value.should.equal(b"")