homepage

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:
* Doc/includes/email-* changes have already been committed.
* There is a commented-out line in Tools/scripts/nm2def.py:
 - f.close()
 + # f.close()
* The patch is huge, I'd commit Tools/ and Lib/test/ parts separately.
* Most of the open(..., 'r') usages can be changed to open(...)
* It would be good to add context management protocol support to http.client.HTTPConnection.

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.
> There is a commented-out line in Tools/scripts/nm2def.py:
It is in a pair to commented out open() two lines above.
> Most of the open(..., 'r') usages can be changed to open(...)
Done.
> * It would be good to add context management protocol support to
> http.client.HTTPConnection.
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.
> 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.
Yes, sad, but Tools/scripts/ is full of Python 2-isms.

п'ятниця, 20-бер-2015 12:36:25 ви написали:
> Martin Panter added the comment:
> 
> 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.
> 
> ----------
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22831>
> _______________________________________

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':
Issue #22831: Use "with" to avoid possible fd leaks.
https://hg.python.org/cpython/rev/ea94f6c87f5d 

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
fd_leaks_tools1_2.patch
fd_leaks_tools2_2.patch
fd_leaks_tests1_2.patch
fd_leaks_tests2_2.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.

PR 10926 is based on fd_leaks_tools1_2.patch.
PR 10927 is based on fd_leaks_tools2_2.patch.
PR 10928 is based on fd_leaks_tests1_2.patch.
PR 10929 is based on fd_leaks_tests2_2.patch.
Some of changes in these patches were already applied in other issues.

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?

New changeset c5d5dfdb223efb0e668e3f317d31b8b70ae96aa6 by Serhiy Storchaka in branch 'master':
bpo-22831: Use "with" to avoid possible fd leaks in distutils. (GH-10921)
https://github.com/python/cpython/commit/c5d5dfdb223efb0e668e3f317d31b8b70ae96aa6

New changeset 9e4861f52349011cd5916eef8e8344575e8ac426 by Serhiy Storchaka in branch 'master':
bpo-22831: Use "with" to avoid possible fd leaks in tests (part 1). (GH-10928)
https://github.com/python/cpython/commit/9e4861f52349011cd5916eef8e8344575e8ac426

New changeset 5b10b9824780b2181158902067912ee9e7b04657 by Serhiy Storchaka in branch 'master':
bpo-22831: Use "with" to avoid possible fd leaks in tests (part 2). (GH-10929)
https://github.com/python/cpython/commit/5b10b9824780b2181158902067912ee9e7b04657

New changeset afbb7a371fb44edc731344eab5b474ad8f7b57d7 by Serhiy Storchaka in branch 'master':
bpo-22831: Use "with" to avoid possible fd leaks in tools (part 1). (GH-10926)
https://github.com/python/cpython/commit/afbb7a371fb44edc731344eab5b474ad8f7b57d7

New changeset 172bb39452ae8b3ccdf5d1f23ead46f44200cd49 by Serhiy Storchaka in branch 'master':
bpo-22831: Use "with" to avoid possible fd leaks in tools (part 2). (GH-10927)
https://github.com/python/cpython/commit/172bb39452ae8b3ccdf5d1f23ead46f44200cd49