Skip to content

Commit

Permalink
Fix bug when S3Request is GC'd before download completes (#506)
Browse files Browse the repository at this point in the history
**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.
  • Loading branch information
graebm committed Oct 9, 2023
1 parent cf9b73f commit 203ba9e
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 5 deletions.
12 changes: 7 additions & 5 deletions source/s3_meta_request.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions test/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 203ba9e

Please sign in to comment.