classification
Title: Use "with" to avoid possible fd leaks
Type: resource usage Stage: patch review
Components: Demos and Tools, Library (Lib), Tests Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, berker.peksag, dstufft, eric.araujo, ezio.melotti, martin.panter, python-dev, rhettinger, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2014-11-09 19:22 by serhiy.storchaka, last changed 2018-12-05 17:12 by serhiy.storchaka.

Files
File name Uploaded Description Edit
fd_leaks.diff serhiy.storchaka, 2014-11-09 19:22
fd_leaks.ignore-all-space.diff martin.panter, 2015-03-20 09:20 review
fd_leaks_distutils.patch serhiy.storchaka, 2015-03-20 11:03 review
fd_leaks_lib.patch serhiy.storchaka, 2015-03-20 11:03 review
fd_leaks_tests1.patch serhiy.storchaka, 2015-03-20 11:03 review
fd_leaks_tools2.patch serhiy.storchaka, 2015-03-20 11:03 review
fd_leaks_tests2.patch serhiy.storchaka, 2015-03-20 11:03 review
fd_leaks_tools1.patch serhiy.storchaka, 2015-03-20 11:03 review
fd_leaks_tests1_2.patch serhiy.storchaka, 2015-03-20 13:39 review
fd_leaks_tools1_2.patch serhiy.storchaka, 2015-03-20 13:39 review
fd_leaks_tools2_2.patch serhiy.storchaka, 2015-03-20 13:39 review
fd_leaks_tests2_2.patch serhiy.storchaka, 2016-01-03 07:00 review
Pull Requests
URL Status Linked Edit
PR 10921 open serhiy.storchaka, 2018-12-05 15:13
PR 10926 open serhiy.storchaka, 2018-12-05 16:04
PR 10927 open serhiy.storchaka, 2018-12-05 16:04
PR 10928 open serhiy.storchaka, 2018-12-05 16:04
PR 10929 open serhiy.storchaka, 2018-12-05 16:05
Messages (17)
msg230901 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-09 19:22
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.
msg238602 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-03-20 03:30
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.
msg238625 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-20 09:12
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.
msg238627 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-20 09:20
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.
msg238643 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-20 11:03
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.
msg238655 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-20 12:36
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.
msg238670 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-20 13:39
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.
msg238671 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-20 13:39
п'ятниця, 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>
> _______________________________________
msg238898 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-22 13:55
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.
msg238901 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-22 14:23
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.
msg240050 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-04-04 08:03
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
msg257400 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2016-01-03 06:37
Serhiy, what's the status of this?
msg257402 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-01-03 07:00
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
msg257587 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-06 05:43
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.
msg331130 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-05 15:14
PR 10921 is based on fd_leaks_distutils.patch.
msg331134 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-05 16:07
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.
msg331137 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-05 17:12
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().
History
Date User Action Args
2018-12-05 17:12:22serhiy.storchakasetmessages: + msg331137
2018-12-05 16:52:06serhiy.storchakasetpull_requests: - pull_request10179
2018-12-05 16:52:02serhiy.storchakasetpull_requests: - pull_request10177
2018-12-05 16:51:57serhiy.storchakasetpull_requests: - pull_request10176
2018-12-05 16:51:54serhiy.storchakasetpull_requests: - pull_request10180
2018-12-05 16:51:51serhiy.storchakasetpull_requests: - pull_request10173
2018-12-05 16:51:43serhiy.storchakasetpull_requests: - pull_request10174
2018-12-05 16:51:36serhiy.storchakasetpull_requests: - pull_request10171
2018-12-05 16:51:27serhiy.storchakasetpull_requests: - pull_request10170
2018-12-05 16:08:11serhiy.storchakasetpull_requests: - pull_request10160
2018-12-05 16:07:44serhiy.storchakasetmessages: + msg331134
2018-12-05 16:05:28serhiy.storchakasetpull_requests: + pull_request10180
2018-12-05 16:05:19serhiy.storchakasetpull_requests: + pull_request10179
2018-12-05 16:05:11serhiy.storchakasetpull_requests: + pull_request10178
2018-12-05 16:05:03serhiy.storchakasetpull_requests: + pull_request10177
2018-12-05 16:04:56serhiy.storchakasetpull_requests: + pull_request10176
2018-12-05 16:04:51serhiy.storchakasetpull_requests: + pull_request10175
2018-12-05 16:04:42serhiy.storchakasetpull_requests: + pull_request10173
2018-12-05 16:04:36serhiy.storchakasetpull_requests: + pull_request10174
2018-12-05 16:04:27serhiy.storchakasetpull_requests: + pull_request10172
2018-12-05 16:04:22serhiy.storchakasetpull_requests: + pull_request10171
2018-12-05 16:04:15serhiy.storchakasetpull_requests: + pull_request10170
2018-12-05 16:04:07serhiy.storchakasetpull_requests: + pull_request10169
2018-12-05 15:14:36serhiy.storchakasetnosy: + eric.araujo, dstufft
2018-12-05 15:14:15serhiy.storchakasetmessages: + msg331130
versions: + Python 3.8, - Python 3.5
2018-12-05 15:13:22serhiy.storchakasetpull_requests: + pull_request10160
2018-12-05 15:13:18serhiy.storchakasetpull_requests: + pull_request10159
2016-01-06 05:43:33martin.pantersetmessages: + msg257587
2016-01-03 07:00:39serhiy.storchakasetfiles: + fd_leaks_tests2_2.patch

messages: + msg257402
2016-01-03 06:37:29ezio.melottisetmessages: + msg257400
2015-04-04 08:03:49python-devsetnosy: + python-dev
messages: + msg240050
2015-03-22 14:23:28serhiy.storchakasetmessages: + msg238901
2015-03-22 13:55:35martin.pantersetmessages: + msg238898
2015-03-20 13:39:52serhiy.storchakasetfiles: + fd_leaks_tests1_2.patch, fd_leaks_tools1_2.patch, fd_leaks_tools2_2.patch

messages: + msg238671
2015-03-20 13:39:19serhiy.storchakasetmessages: + msg238670
2015-03-20 12:36:25martin.pantersetmessages: + msg238655
2015-03-20 11:03:56serhiy.storchakasetfiles: + fd_leaks_distutils.patch, fd_leaks_lib.patch, fd_leaks_tests1.patch, fd_leaks_tools2.patch, fd_leaks_tests2.patch, fd_leaks_tools1.patch

messages: + msg238643
2015-03-20 09:20:28martin.pantersetfiles: + fd_leaks.ignore-all-space.diff

messages: + msg238627
2015-03-20 09:12:06vstinnersetnosy: + vstinner
messages: + msg238625
2015-03-20 03:30:02berker.peksagsetnosy: + berker.peksag
messages: + msg238602
2015-03-20 03:00:09martin.pantersetnosy: + martin.panter
2015-03-02 08:54:50ezio.melottisetnosy: + ezio.melotti
2014-11-14 02:11:49Arfreversetnosy: + Arfrever
2014-11-09 19:22:27serhiy.storchakacreate