Skip to content

bpo-28867: Warn when NamedTemporaryFile is not explicitly closed #1936

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

bpo-28867: Warn when NamedTemporaryFile is not explicitly closed #1936

wants to merge 1 commit into from

Conversation

jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Jun 4, 2017 •
edited by bedevere-bot
Loading

@mention-bot
Copy link

@jdufresne, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @tim-one and @orsenthil to be potential reviewers.

@jdufresne
Copy link
Contributor Author

Added the news entry.

@matrixise matrixise self-assigned this Sep 12, 2019
berkerpeksag
berkerpeksag requested changes Jan 23, 2020
View reviewed changes
Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

I don't think the following pattern is acceptable:

try:
    ...
except urllib.error.HTTPError as exc:
    exc.close()

Dealing with HTTPError's fp attribute shouldn't be the responsibility of the user. It would be better to deal with it in addbase:

cpython/Lib/urllib/response.py

Line 14 in 99e6c26

class addbase(tempfile._TemporaryFileWrapper):

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@jdufresne
Copy link
Contributor Author

@berkerpeksag Thanks for the review. I'm happy to adjust the PR to meet the request, but I could use a bit of guidance and more detail.

The reason I took this approach is due to the comment here:

cpython/Lib/urllib/request.py

Lines 742 to 745 in 65ecc39

# Don't close the fp until we are sure that we won't use it
# with HTTPError.
fp.read()
fp.close() 

        # Don't close the fp until we are sure that we won't use it
        # with HTTPError.

Are you suggesting that the error handler should eagerly close the file? Would doing so be acceptable?

@berkerpeksag
Copy link
Member

Ah, good point, thanks. I need to think about it more carefully. While I don't have objections on the idea discussed in the issue, I think we need to find a solution for exc.close() before merging this PR.

@jdufresne jdufresne closed this Aug 26, 2021
@jdufresne jdufresne deleted the named-temporary-warning branch August 26, 2021 17:13
@jdufresne jdufresne mannequin mentioned this pull request Apr 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkerpeksag berkerpeksag berkerpeksag requested changes

Assignees

@matrixise matrixise

Labels
awaiting changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jdufresne @mention-bot @bedevere-bot @berkerpeksag @matrixise @brettcannon @the-knights-who-say-ni