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 :

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 :

./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]

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

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

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.

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

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.

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.

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

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

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.