Ensure files are always closed after use#5346
Conversation
aakallam
force-pushed
the
push-rktqpxtnxyxo
branch
5 times, most recently
from
dd606d1 to
79d5a0e
Compare
aakallam
force-pushed
the
push-rktqpxtnxyxo
branch
from
79d5a0e to
46a8e8c
Compare
| finally: | ||
| # Don't need the archive after writing testcase to blobstore. | ||
| if zip_path: | ||
| shell.remove_file(zip_path) |
There was a problem hiding this comment.
Is there a similar way we can handle file clean up?
There was a problem hiding this comment.
Do you mean for the testcase files or something else?
There was a problem hiding this comment.
I thought it was strange that we're manually calling remove_file(zip_path) in a couple places but I'm not sure of a way around that.
either way cleaning up the file usage LGTM
| finally: | ||
| # Don't need the archive after writing testcase to blobstore. | ||
| if zip_path: | ||
| shell.remove_file(zip_path) |
There was a problem hiding this comment.
I thought it was strange that we're manually calling remove_file(zip_path) in a couple places but I'm not sure of a way around that.
either way cleaning up the file usage LGTM
ViniciustCosta
left a comment
ViniciustCosta
left a comment
There was a problem hiding this comment.
Looks good!
Since it touches multiple files, could you get some evidence running on dev that the change is not adding unexpected regressions?
aakallam
force-pushed
the
push-rktqpxtnxyxo
branch
from
46a8e8c to
707f9df
Compare
|
I deployed to dev yesterday and have confirmed that no regressions have been found |
| logs.error(f'Unable to open testcase archive {zip_path}.') | ||
| return None, None, None | ||
| finally: | ||
| if zip_path: |
There was a problem hiding this comment.
Wouldn't this remove the zip_path file before it is used in with open(upload_file_path), since upload_file_path is just a reference to zip_path?
| file_path = file_list[0] | ||
| file_handle = open(file_path, 'wb') | ||
| file_handle.close() | ||
| testcase_filepath = file_list[0] |
There was a problem hiding this comment.
In the original code, if we minimized everything, opening the file in wb mode and immediately closing it effectively truncated the file to 0 bytes. I think this step was removed here, which mean instead of uploading an empty testcase, it will now upload the original, un-minimized file contents, right?
| storage.upload_signed_url(data, minimize_task_input.testcase_upload_url) | ||
| minimized_keys = minimize_task_input.testcase_blob_name | ||
| file_handle.close() | ||
| with open(testcase_filepath, 'rb') as file_handle: |
There was a problem hiding this comment.
Should we wrap this in a try-catch similar to what we had in the original code with the logs.error('Unable to open archive for blobstore write.')?
3 participants
This is a follow up to #5342 that refactors a bunch of file I/O to use
withstatements to ensure that the file handles are always closed. Closing file handles is especially important on Windows because only one file object can point at a file at one time.To make a few complex file opening cases work, this PR adds a couple new path variables to simplify the refactoring but otherwise does not introduce or change any logic.