Automatus: close hanging tempfiles descriptors #9199
Conversation
We don't need those descriptors only their filenames, similar as in ace8f69.
|
Start a new ephemeral environment with changes proposed in this pull request: Fedora Environment Oracle Linux 8 Environment |
|
Code Climate has analyzed commit 14d46cd and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 42.7% (0.0% change). View more on Code Climate. |
| @@ -493,7 +493,8 @@ def copy_of_datastream(self, new_filename=None): | |||
| os.unlink(new_filename) | |||
|
|
|||
| def _change_variable_value(self, varname, value): | |||
| _, xslt_filename = tempfile.mkstemp(prefix="xslt-change-value", dir="/tmp") | |||
| descriptor, xslt_filename = tempfile.mkstemp(prefix="xslt-change-value", dir="/tmp") | |||
| os.close(descriptor) | |||
There was a problem hiding this comment.
How about using the already open descriptor to write the template into it, instead of opening the file again in lines 499~500.
There was a problem hiding this comment.
We can, but mkstemp returns a low-level file descriptor (not file object which is expected by write() function), so we will need to use os.write and convert the string which we will be writing into the file to bytes.
There was a problem hiding this comment.
Ahh, okay, overlooked that detail.
There was a problem hiding this comment.
@matusmarhefka So, the file is opened with open() and in line 500 we do fp.write().
And the docs say that the descriptor should be the same as if an open() was done:
https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp
I have not tried, but I think it should work, no?
There was a problem hiding this comment.
mkstemp() returns a tuple containing an OS-level handle to an open file (as would be returned by os.open()) and the absolute pathname of that file, in that order.
So not open, but os.open, which means only write from the os module will work (the last in example), but we would need to convert string to bytes:
>>> import os
>>> from tempfile import mkstemp
>>> fd, name = mkstemp()
>>> str = """test"""
>>> fd.write(str)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'int' object has no attribute 'write'
>>> os.write(fd, str)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: a bytes-like object is required, not 'str'
>>> os.write(fd, str.encode())
4
I don't have strong opinion, but I would rather stick to the original code as it doesn't require conversion to bytes.
There was a problem hiding this comment.
I wasn't paying attention to difference between open() and os.open()
There was a problem hiding this comment.
No problem, thanks for the review.
We don't need those descriptors only their filenames,
similar as in ace8f69.