New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wave.Wave_read.close() doesn't release file #55064
Comments
Calling wave.close() fails to release all references to the file passed in via wave.open(filename_or_obj, "rb"). As a result, processing many wave files produces an IOError of too many files open. This bug is often masked because this dangling reference is collected if the wave object is collected. However, if the wave object is retained, calling wave_obj.close() won't release the reference, and so the file will never be closed. There are two solutions:
Trunk code (and 2.7.1, and older): def close(self):
if self._i_opened_the_file:
self._i_opened_the_file.close()
self._i_opened_the_file = None
self._file = None but note initfp(self, file): therefore close needs to add: self._data_chunk = None |
Thanks for the report and analysis. Would you care to submit a patch to fix it? |
(Presumably this is also a problem for Python 3, as well). |
This is not a bug in the implementation: the file object is only closed when you passed a file name to open(). Like other APIs that allow file names or objects to be passed in, it is the caller's responsibility to close the file object if an object was passed. However, this was not documented. I've fixed that with r87859. |
Thank you for clarifying the documentation. However, I don't think that fully resolves the issue. I'm not complaining about a failure to close the file. As you observe, it doesn't need to (and shouldn't) close a file object, but it should release the reference. The code already tries to release the reference ("self._file = None"). It just fails to release it correctly, missing the other reference to the file object (self._data_chunk). That's the bug. Your clarification of the documentation is appreciated nonetheless. I've attached a patch as Ned requested. The same patch can currently be applied to release27-maint, release31-maint, and py3k. (The line numbers and surrounding context are identical.) |
I don't really see the bug here. Either you openened the file object, then you have to close it. Or wave.py opened it, then it will close it, no matter if it still has a reference or not. Of course we could set _data_chunk to None, but I'm unsure what behavior change you would expect from that. |
Agreed with Georg. No OS resource is leaking if the file is explicitly closed (since it releases the underlying file descriptor). That the Python "file object" is still attached somewhere is of secondary importance. |
A point of clarification on the original report: Georg is completely right when he points out that this is only an issue when passing in a file object. If passed a filename, wave.py both opens and closes the file explicitly, and the dangling reference isn't important, as Antoine observes. However, a retained reference in the file-object case is still a leak. Georg writes: "Of course we could set _data_chunk to None, but I'm unsure what behavior change you would expect from that." It allows garbage collection to close the file object if there are no more references to it. It seems reasonable for a client of wave.py to assume that close() will release all references to the object, and indeed the code seems to support that assumption -- it sets _file to None. If releasing references were truly of no importance, then I would argue that the line setting _file to None should be removed. It serves no purpose after wave.py has explicitly closed the file (if it opened it) other than to release a reference to the file object. Therefore, I suggest that _data_chunk should also be set to None in order to release the reference completely, thereby allowing the file object to be garbage collected. |
This is a very bad policy to begin with. Garbage collection can be delayed for a number of reasons:
So the good thing to do is to close your file explicitly. Luckily, Python 2.6 and upwards makes it easier by using the "with" statement. IMO this issue should be closed. |
I am not sure, should this issue be closed as "rejected" because the suggested patch for Lib/wave.py was rejected, or as "fixed" because Georg was committed the documentation patch (changeset 8239ec6f39e6) for this issue? |
I say "fixed": there was a bug (undocumented, but correct behavior) and that was fixed. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: