bpo-41395: use context manager to close filetype objects

[amiremohamadi]

Use context manager to close filetype objects and avoid ResourceWarnings.

https://bugs.python.org/issue41395

[kk456852]

Your code is impressive!

keep going!

[amiremohamadi]

I think Azure Pipeline fail is not related to my changes. is it possible to run tests again?

[gvanrossum]

Close and reopen to rerun tests.

[remilapeyre]

I think this could be improved as if there is an error while processing some files, the files that have not been already processed will not be closed. By using contextlib.ExitStack() here we could make sure that the files would always be closed even if some error occurs as we would register them all before starting processing them.

[amiremohamadi]

Sounds good!
Just for making sure, you mean sth like this?

with contextlib.ExitStack() as stack:                            
    output = stack.enter_context(args.output)                    
    infiles = [stack.enter_context(f) for f in args.pickle_file] 
    for f in infiles:                                            
        preamble = args.preamble.format(name=f.name)
        output.write(preamble + '\n')
        dis(f, output, memo, args.indentlevel, annotate)

[facundobatista]

What about just using try/finally? Looks simpler to me:

            try:
                for f in args.pickle_file:
                    preamble = args.preamble.format(name=f.name)
                    args.output.write(preamble + '\n')
                    dis(f, args.output, memo, args.indentlevel, annotate)
            finally:
                args.output.close()
                for f in args.pickle_file:
                    f.close()

[amiremohamadi]

@facundobatista ,@remilapeyre , @serhiy-storchaka

as you know, argparse module tries to open the FileType object and if any error occurs it raises an exception. here's the code from Lib/argparse.py:

try:                                                                 
    return open(string, self._mode, self._bufsize, self._encoding, self._errors)
except OSError as e:
    args = {'filename': string, 'error': e}
    message = _("can't open '%(filename)s': %(error)s")
    raise ArgumentTypeError(message % args)

So when we run pickletools like this:

python -Wall Lib/pickletools.py file1 chertfile file2 

If chertfile doesn't exist, it raises the exception and we get ResourceWarning because file1 opened before and never closed.

Any idea? Should it be handled in argparse module?

[facundobatista]

Oh, mmm... so there are multiple exit/crashing points after argparse opened the file. It's impossible/too hard to cover them all outside argparse.

Probably the best way to fix this is in argparse itself, maybe it's as easy as using atexit.register(just_opened_file.close) there (which will behave correctly even if the file is manually closed before).

That way, also, this issue will not be solved only for pickle, but for everybody using argparse.

+1 from me to fix this in argparse, then.

[serhiy-storchaka]

It goes in the right direction, but it cannot overcome the main innate defect of FileType. It leaves the files open if Python quits or fails before it has a chance to handle the opened files. For example, when pass option -t in the pickletools CLI. Or pass only the output file argument without input file arguments.

#113618 manages files more explicitly and does not have such issues.