classification
Title: codecs.open leaks file descriptor when invalid encoding is passed
Type: resource usage Stage: patch review
Components: IO, Library (Lib) Versions: Python 3.9, Python 3.8, Python 3.7, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Brock Mendel, caporta, josh.r, serhiy.storchaka, xtreak
Priority: normal Keywords: easy, patch

Created on 2019-12-04 16:51 by Brock Mendel, last changed 2020-01-26 18:33 by caporta.

Pull Requests
URL Status Linked Edit
PR 17666 open python-dev, 2019-12-19 22:31
Messages (8)
msg357811 - (view) Author: Brock Mendel (Brock Mendel) Date: 2019-12-04 16:51
xref https://github.com/pandas-dev/pandas/pull/30034

codecs.open does `file = open(...)` before validating the encoding kwarg, leaving the open file behind if that validation raises.
msg357812 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2019-12-04 17:01
Does using with block similar to https://bugs.python.org/issue22831 solve this problem?
msg357814 - (view) Author: Brock Mendel (Brock Mendel) Date: 2019-12-04 17:19
> Does using with block similar to https://bugs.python.org/issue22831 solve this problem?

The motivating use case uses `with codecs.open(buf, "w", encoding=encoding) as f:`

https://github.com/pandas-dev/pandas/blob/master/pandas/io/formats/format.py#L498
msg357815 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2019-12-04 18:01
Ah okay, thanks for the detail. Forgot there should be an open file handle to be returned by codecs.open
msg357818 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-12-04 19:34
Add

    try:
        ...
    except:
        file.close()
        raise
msg357834 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-12-04 22:35
Any reason not to just defer opening the file until after the codec has been validated, so the resource acquisition comes last?
msg357838 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-12-05 05:26
Many reasons.

1. It is simpler.
2. We will need a try/except in any case to prevent a leak if an exception be raised by other code following open().
3. It matches the behavior of io.open().
msg360731 - (view) Author: Chris Aporta (caporta) * Date: 2020-01-26 18:33
Just quickly pinging the thread as a friendly reminder that PR 17666 is open and potentially close to mergeable, as it's been through two review cycles already (thanks Serhiy). If someone has the bandwidth to take another look, it would be greatly appreciated. Thanks!
History
Date User Action Args
2020-01-26 18:33:16caportasetnosy: + caporta
messages: + msg360731
2019-12-19 22:31:26python-devsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request17132
2019-12-05 05:26:50serhiy.storchakasetmessages: + msg357838
2019-12-04 22:35:40josh.rsetnosy: + josh.r
messages: + msg357834
2019-12-04 19:34:41serhiy.storchakasetversions: + Python 2.7, Python 3.7, Python 3.8, Python 3.9
messages: + msg357818

components: + Library (Lib), IO
keywords: + easy
type: resource usage
stage: needs patch
2019-12-04 18:01:56xtreaksetmessages: + msg357815
2019-12-04 17:19:31Brock Mendelsetmessages: + msg357814
2019-12-04 17:01:30xtreaksetnosy: + serhiy.storchaka
messages: + msg357812
2019-12-04 16:54:51xtreaksetnosy: + xtreak
2019-12-04 16:51:47Brock Mendelcreate