Use "with" to avoid possible fd leaks

[serhiy-storchaka]

BPO	22831
PRs	bpo-22831: Use "with" to avoid possible fd leaks in distutils. #10921 bpo-22831: Use "with" to avoid possible fd leaks in tools (part 1). #10926 bpo-22831: Use "with" to avoid possible fd leaks in tools (part 2). #10927 bpo-22831: Use "with" to avoid possible fd leaks in tests (part 1). #10928 bpo-22831: Use "with" to avoid possible fd leaks in tests (part 2). #10929
Files	fd_leaks.diff fd_leaks.ignore-all-space.diff fd_leaks_distutils.patch fd_leaks_lib.patch fd_leaks_tests1.patch fd_leaks_tools2.patch fd_leaks_tests2.patch fd_leaks_tools1.patch fd_leaks_tests1_2.patch fd_leaks_tools1_2.patch fd_leaks_tools2_2.patch fd_leaks_tests2_2.patch

^{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:

assignee = None
closed_at = <Date 2019-03-30.06:34:13.702>
created_at = <Date 2014-11-09.19:22:27.681>
labels = ['type-security', '3.8', '3.9', 'library', 'build', 'tests']
title = 'Use "with" to avoid possible fd leaks'
updated_at = <Date 2020-01-28.07:18:39.356>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2020-01-28.07:18:39.356>
actor = 'Hinsonlcrystal'
assignee = 'none'
closed = True
closed_date = <Date 2019-03-30.06:34:13.702>
closer = 'serhiy.storchaka'
components = ['Build', 'Demos and Tools', 'Library (Lib)', 'Tests']
creation = <Date 2014-11-09.19:22:27.681>
creator = 'serhiy.storchaka'
dependencies = []
files = ['37158', '38589', '38592', '38593', '38594', '38595', '38596', '38597', '38601', '38602', '38603', '41480']
hgrepos = []
issue_num = 22831
keywords = ['patch']
message_count = 23.0
messages = ['230901', '238602', '238625', '238627', '238643', '238655', '238670', '238671', '238898', '238901', '240050', '257400', '257402', '257587', '331130', '331134', '331137', '332193', '332240', '337169', '337170', '339179', '339180']
nosy_count = 1.0
nosy_names = ['Hinsonlcrystal']
pr_nums = ['10921', '10926', '10927', '10928', '10929']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue22831'
versions = ['Python 3.8', 'Python 3.9']

[serhiy-storchaka]

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.

[berkerpeksag]

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.

[vstinner]

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.

[vadmium]

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.

[serhiy-storchaka]

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.

[vadmium]

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.

[serhiy-storchaka]

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.

[serhiy-storchaka]

п'ятниця, 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\>

---

[vadmium]

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.

[serhiy-storchaka]

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.

[python-dev]

New changeset ea94f6c87f5d by Serhiy Storchaka in branch 'default':
Issue bpo-22831: Use "with" to avoid possible fd leaks.
https://hg.python.org/cpython/rev/ea94f6c87f5d

[ezio-melotti]

Serhiy, what's the status of this?