Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

bpo-41395: use context manager to close filetype objects#21702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

amiremohamadi
Copy link
Contributor

@amiremohamadiamiremohamadi commentedAug 2, 2020
edited by bedevere-bot
Loading

Use context manager to close filetype objects and avoid ResourceWarnings.

https://bugs.python.org/issue41395

@amiremohamadiamiremohamadi changed the titlebpo 41395: use context manager to close filetype objectsbpo-41395: use context manager to close filetype objectsAug 2, 2020
Copy link

@kk456852kk456852 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Your code is impressive!

keep going!

@amiremohamadi
Copy link
ContributorAuthor

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

@gvanrossum
Copy link
Member

Close and reopen to rerun tests.

dis(f, args.output, memo, args.indentlevel, annotate)
with args.output:
for f in args.pickle_file:
with f:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 usingcontextlib.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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What about just usingtry/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 reacted with thumbs up emoji
@amiremohamadi
Copy link
ContributorAuthor

@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 fromLib/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

Ifchertfile doesn't exist, it raises the exception and we getResourceWarning because file1 opened before and never closed.

Any idea? Should it be handled in argparse module?

@facundobatista
Copy link
Member

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 usingatexit.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.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It goes in the right direction, but it cannot overcome the main innate defect ofFileType. 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 thepickletools CLI. Or pass only the output file argument without input file arguments.

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@facundobatistafacundobatistafacundobatista left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@remilapeyreremilapeyreremilapeyre requested changes

@kk456852kk456852kk456852 approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

9 participants
@amiremohamadi@gvanrossum@facundobatista@serhiy-storchaka@remilapeyre@kk456852@the-knights-who-say-ni@ezio-melotti@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp