pickle and pickletools cli interface doesn't close input and output file.

[tirkarthi]

BPO	41395
Nosy	@avassalotti, @serhiy-storchaka, @corona10, @tirkarthi, @amiremohamadi, @Foxyblue, @achhina
PRs	bpo-41395: Close file objects in pickle and pickletools #21676 bpo-41395: use context manager to close filetype objects #21702 gh-85567: Register a cleanup function to close files for FileType objects in argparse #32257

^{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 = None
created_at = <Date 2020-07-25.14:12:10.580>
labels = ['easy', 'type-bug', '3.8', '3.9', '3.10', 'library']
title = "pickle and pickletools cli interface doesn't close input and output file."
updated_at = <Date 2022-04-05.13:05:47.595>
user = 'https://github.com/tirkarthi'

bugs.python.org fields:

activity = <Date 2022-04-05.13:05:47.595>
actor = 'achhina'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2020-07-25.14:12:10.580>
creator = 'xtreak'
dependencies = []
files = []
hgrepos = []
issue_num = 41395
keywords = ['patch', 'easy']
message_count = 2.0
messages = ['374269', '416776']
nosy_count = 8.0
nosy_names = ['alexandre.vassalotti', 'python-dev', 'serhiy.storchaka', 'corona10', 'xtreak', 'Amir', 's.williams-wynn', 'achhina']
pr_nums = ['21676', '21702', '32257']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue41395'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

Linked PRs

• gh-85567: Fix resouce warnings in pickle and pickletools CLIs #113618
• [3.12] gh-85567: Fix resouce warnings in pickle and pickletools CLIs (GH-113618) #113758
• [3.11] gh-85567: Fix resouce warnings in pickle and pickletools CLIs (GH-113618) #113759

[tirkarthi]

pickle and pickletools use argparse with FileType which is not automatically closed. Other cli interfaces like json [0], ast [1] use context manager to close filetype objects.

pickle :

cpython/Lib/pickle.py

Line 1799 in af08db7

'pickle_file', type=argparse.FileType('br'),

mypickle

>> import pickle
>> with open("mypickle", "wb") as f: pickle.dump({"a": 1}, f)

./python -Wall -m pickle mypickle
{'a': 1}
sys:1: ResourceWarning: unclosed file <_io.BufferedReader name='mypickle'>

pickletools :

cpython/Lib/pickletools.py

Lines 2850 to 2855 in af08db7

	parser.add_argument(
	'pickle_file', type=argparse.FileType('br'),
	nargs='*', help='the pickle file')
	parser.add_argument(
	'-o', '--output', default=sys.stdout, type=argparse.FileType('w'),
	help='the file where the output should be written')

./python -Wall -m pickletools mypickle -o mypickle.py
sys:1: ResourceWarning: unclosed file <_io.BufferedReader name='mypickle'>
sys:1: ResourceWarning: unclosed file <_io.TextIOWrapper name='mypickle.py' mode='w' encoding='UTF-8'>

[0]

cpython/Lib/json/tool.py

Line 61 in af08db7

with options.infile as infile, options.outfile as outfile:

[1]

cpython/Lib/ast.py

Line 1510 in af08db7

with args.infile as infile:

[achhina]

Hi,

First-time contributor here, I've made a patch in follow-up to the discussions that happened in Amir's patch in regards to this. I'd appreciate it if someone would be able to take a look and review it!

#32257

[vstinner]

The commit 328dbc0 of #85567 introduced a reference leak: I created #91757

[serhiy-storchaka]

test_argparse uses the argparse in a not common way, but we cannot guarantee that some third-party code does not use argparse repeatedly.

There is a problem with #32257 -- the change prevent closing file object by the garbage collector.

More complex solution could work:

1. All open files should be added to a global WeakSet.
2. Register only one cleanup function which closes files in that set.

[vstinner]

I'm not a fan of closing files magically at exit. I prefer to close them explicitly. Why not fixing the issue in pickletools, rather than changing the argparse module?

Since the Refleaks buildbots are broken for 6 days, I just revert the PR until someone can investigate the issue and fix it: #91771

[ncoghlan]

The note about using FileType in https://docs.python.org/3/library/argparse.html#type conveys the value of making argparse clean up by default:

Even FileType has its limitations for use with the type keyword. If one argument uses FileType and then a subsequent argument fails, an error is reported but the file is not automatically closed. In this case, it would be better to wait until after the parser has run and then use the with-statement to manage the files.

An enhanced cleanup check that works the way @serhiy-storchaka suggests (closing all files in a WeakSet, rather than holding a strong reference to all opened files until the program ends) would mean that the files would always be closed at the Python level (even if some subsequent error prevents the affected app from reaching its "normal" file handling code) without preventing the normal GC cleanup.

[serhiy-storchaka]

I am not completely satisfied with closing files at exit, even if use a WeakSet. It makes us relying on the garbage collector to prevent file descriptor exhausting when argparse is used repeatedly in one process. Was not ResourceWarning introduced to catch such kind of errors?

The https://github.com/pallets/click library has several FileType-like types and multiple options to control the behavior (it perhaps even overengineered). I see two general ways to solve the problem:

1. A new FileType-like type which returns a factory, not an open file. You need to open it before use (preferably with the with statement).
2. Make ArgumentParser or a new parser class owning all open files. It should have a method for closing them all and support the context manager protocol. It will require significant rewriting the user code, because you need to keep the parser open while work with files.

And the problem is not limited to open files. We need a way to specify how to clean up resources with user-defined types (for example a type similar to FileType, but supporting compression, or opening a URL instead of a file). The above two options could be generalized.

For now, I think it is easier to revert #32257.

[serhiy-storchaka]

There is an older and more specific issue for FileType flaws: #58032. Let continue there.

[furkanonder]

@serhiy-storchaka The issue seems to be resolved in this PR.

[serhiy-storchaka]

The pickletools CLI is especially treacherous. It leaks the files if you specify the -t argument. It leaks the output file if you only specify the output file argument. And of course it leaks the files in case of argument errors, for example specifying non-integer indent level or non-writeable output file.

The only reliable way to handle this is to get rid of FileType and open and close files explicitly. PR #113618 does this.