bpo-41395: use context manager to close filetype objects - #21702
bpo-41395: use context manager to close filetype objects#21702amiremohamadi wants to merge 8 commits into
Conversation
Sorry, something went wrong.
kk456852
left a comment
There was a problem hiding this comment.
Your code is impressive!
keep going!
Sorry, something went wrong.
|
I think Azure Pipeline fail is not related to my changes. is it possible to run tests again? |
Sorry, something went wrong.
|
Close and reopen to rerun tests. |
Sorry, something went wrong.
| dis(f, args.output, memo, args.indentlevel, annotate) | ||
| with args.output: | ||
| for f in args.pickle_file: | ||
| with f: |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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)
Sorry, something went wrong.
There was a problem hiding this comment.
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()
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Use context manager to close filetype objects and avoid ResourceWarnings.
https://bugs.python.org/issue41395