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
Use "with" to avoid possible fd leaks #67020
Comments
Here is large patch which convert a code which potentially can leak file descriptor to use the with statement so files are always closed. This can make effect mainly on alternative Python implementation without reference counting. But even on CPython this will get rid from resource leaking warnings. |
Looks good. A couple of comments:
|
fd_leaks.diff is impossible to review, even Rietveld gave up, it's too big :-p Could you please your patch into smaller patches? Split by directory for example. |
Posting a version of Serhiy’s patch made with “hg diff -p --ignore-all-space”. It is a bit shorter, and should not include all the re-indented lines, which may help reviewing. |
The patch is synchronized with the tip and divided on smaller parts.
It is in a pair to commented out open() two lines above.
Done.
In separate issue. |
There were a couple mistakes I found; see Reitveld. I was going to say to leave the open(mode="r") parameter as it is, since an explicit "r" is more readable, but since you already took some out, I’m not going to complain. While many of the changes look worthwhile, I do worry that some of them will never be tested (at least until say Python 3.14 or something). E.g. I just tried running Tools/scripts/dutree.py cos it looked interesting, but was confronted with the “unorderable types: int() < NoneType()” Python 2-ism. |
Here are updated patches with fixed mistakes found by Martin.
Yes, sad, but Tools/scripts/ is full of Python 2-isms. |
п'ятниця, 20-бер-2015 12:36:25 ви написали:
|
The new fd_leaks_tools1_2.patch seems to have dropped the changes for Tools/scripts/treesync.py, compared to the previous fd_leaks_tools1.patch. |
Oh, I forgot to publish my comments. I dropped the changes for treesync.py because treesync.py is not work with Python 3. I have written separate patch for it and will open separate issue after writing tests. |
New changeset ea94f6c87f5d by Serhiy Storchaka in branch 'default': |
Serhiy, what's the status of this? |
Only the most important patch for the stdlib was committed. Patches for distutils, tests and tools are not committed still. fd_leaks_distutils.patch |
I had another look at the five patches you mentioned. I made a couple review comments about expanding the scope of some “with” statements. There are a couple changes that add explicit file closing, where it was previously up to the garbage collector. I.e. code like open(...).read(). I think those changes are the most important, although they are scattered over the various patches. On the other hand, some of the changes in the test suite, particularly test_dbm_dumb.py and test_xmlrpc.py, hardly seem worth it. The main benefit of the “with” statement would be if the test code fails, which hopefully won’t happen that often. :) In the test suite, perhaps it would be better to call self.addCleanup(f.close) or similar in many cases. That way you wouldn’t need contextlib.closing() as much, and there would be less file history clutter and “cavern code”, due to the extra indentation. |
PR 10921 is based on fd_leaks_distutils.patch. |
self.addCleanup(f.close) can not be used if the same file should be opened several times in the test. It can not be used also if the file is deleted in the test or in tearDown(). |
Éric, could you please take a look at PR 10921? |
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: