From 203ba9ef9c438e56e5f374785e5608ea87e7637b Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Mon, 9 Oct 2023 09:16:27 -0700 Subject: [PATCH] Fix bug when S3Request is GC'd before download completes (#506) **Issue:** I discovered that, if you drop the reference to an S3Request before the operation completes and it's downloading using `recv_filepath`, then everything looks like it succeeds but you end up with a 0 byte file on disk. **Description of changes:** Ensure the file isn't closed until the operation is complete. Most of our code was careful to keep things alive until the operation was complete. But for whatever reason we put the file-closing code in the wrong place. --- source/s3_meta_request.c | 12 +++++++----- test/test_s3.py | 7 +++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/source/s3_meta_request.c b/source/s3_meta_request.c index 13ace451a..e471c64e5 100644 --- a/source/s3_meta_request.c +++ b/source/s3_meta_request.c @@ -44,6 +44,9 @@ struct aws_s3_meta_request *aws_py_get_s3_meta_request(PyObject *meta_request) { } static void s_destroy(struct s3_meta_request_binding *meta_request) { + if (meta_request->recv_file) { + fclose(meta_request->recv_file); + } if (meta_request->copied_message) { aws_http_message_release(meta_request->copied_message); } @@ -297,14 +300,13 @@ static void s_s3_request_on_finish( /*************** GIL RELEASE ***************/ } -/* Invoked when the python object get cleaned up */ +/* Invoked when S3Request._binding gets cleaned up. + * DO NOT destroy the C binding struct or anything inside it yet. + * The user might have let S3Request get GC'd, + * but the s3_meta_request_binding* must outlive the native aws_s3_meta_request* */ static void s_s3_meta_request_capsule_destructor(PyObject *capsule) { struct s3_meta_request_binding *meta_request = PyCapsule_GetPointer(capsule, s_capsule_name_s3_meta_request); - if (meta_request->recv_file) { - fclose(meta_request->recv_file); - meta_request->recv_file = NULL; - } if (meta_request->native) { aws_s3_meta_request_release(meta_request->native); } else { diff --git a/test/test_s3.py b/test/test_s3.py index 5de4a8cd6..0d1a0c0ef 100644 --- a/test/test_s3.py +++ b/test/test_s3.py @@ -270,6 +270,13 @@ def test_get_object_file_object(self): on_headers=self._on_request_headers, on_progress=self._on_progress) finished_future = s3_request.finished_future + + # Regression test: Let S3Request get GC'd early. + # The download should continue without problems. + # We once had a bug where the file would get closed too early: + # https://github.com/awslabs/aws-crt-python/pull/506 + del s3_request + finished_future.result(self.timeout) # Result check