bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close #22431

Ev2geny · 2020-09-27T21:12:33Z

This fixes this issue: https://bugs.python.org/issue14243

tempfile.NamedTemporaryFile is too hard to use portably when you need to open the file by name after writing it. To do that, you need to close the file first (on Windows), which means you have to pass delete=False, which in turn means that you get no help in cleaning up the actual file resource,

Hence at the moment there is no out of the box solution to use tempfile.NamedTemporaryFile on Windows in such scenario (which is often used in unit testing):

in test module:

create and open temporary file
write data to it
pass name of the temporary file to the operational code

In operational code, being tested

open file, using name of the temporary file
read data from this temporary file

In this Pull Request the issue is solved by adding an additional optional argument to NamedTemporaryFile() 'delete_on_close' (default is True). It works in combination with already existing argument 'delete', and determines whether created temporary file gets deleted on close (which until this change was the only functionality available and it remains default now).

If 'delete' = True and 'delete_on_close' = False then temporary file gets deleted on context manager exit only (which is a new functionality).

If 'delete' is not true, then value of 'delete_on_close' is ignored.

So, the change shall be fully backwards compatible.

In the bug discussion such option was discussed by r.david.murray but mainly by eryksun

I have also added several unit tests to test new functionality in different combinations

https://bugs.python.org/issue14243

the-knights-who-say-ni · 2020-09-27T21:12:36Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@Ev2geny

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Ev2geny · 2020-09-28T10:49:03Z

I just can't figure out why bedevere/issue-number check is failing. Where do I have to mention it?

blais · 2020-10-11T04:23:12Z

Hmm, I'm not sure I like the API in this solution. If I want to write portable code (Linux and Windows), I would then have to use delete=True and delete_on_close=False in every since call, right? This is going to look at bit convoluted. I'd much rather this was a distinct type of NamedTemporaryFile.

Ev2geny · 2020-10-12T10:54:39Z

Hmm, I'm not sure I like the API in this solution. If I want to write portable code (Linux and Windows), I would then have to use delete=True and delete_on_close=False in every since call, right? This is going to look at bit convoluted. I'd much rather this was a distinct type of NamedTemporaryFile.

@blais , yes, this PR assumes, you would need to use delete_on_close=False (delete=True is already a default), if you want to write a portable code (Linux and Windows), but only if you need to refer to a created temporary file by its name, not by a file object. And this is a new functionality all together, as current functionality of the NamedTemporaryFile implicitly assumes, that you need to refer to a file by its file object (at least on Windows). Though I agree with you, that in test modules, one needs to often refer to temporary file by its name (hence the PR).

There were a lot of suggestions on how to fix this in the original issue14243 discussion with no clear conclusion on what is the best approach. I have chosen this one as it is backwards compatible and does not change philosophy of existing code. I will wait for code reviews to provide their feedback. There must be someone, who takes a final decision on this.

blais · 2020-10-13T01:39:54Z

It would be wiser to create an alternative interface for this that is safe to use between platforms by default, even if it is more constrained, even if that means providing a completely different context manager.

bozhinov · 2020-10-18T13:41:57Z

OK. So I read the comments in the bug and the workaround here.
Here is what I did though (on Windows) - Created a temporary directory - put my temp file in there - script ends - all is gone
Hope that helps

willingc · 2020-12-15T07:54:46Z

@zooba Do you have time to take a look at this PR?

Ev2geny · 2021-01-13T21:57:22Z

@zooba , just checking whether you managed to look at this

MaxwellDupre · 2022-05-21T09:28:09Z

Can you re-target for 3.12 (Doc/whatsnew/3.10.rst) please?
Also, in NEWS I don't think you need double back ticks (``).

AlexWaygood · 2022-05-21T09:52:38Z

Also, in NEWS I don't think you need double back ticks (``).

@MaxwellDupre this is incorrect. Double backticks are usually preferred over single backticks in .rst files, unless a specific ReST role is being used.

Please read https://devguide.python.org/documenting/ for a full guide on ReST syntax.

zooba

Code change looks good, but we need to get the docs in order.

Lib/test/test_tempfile.py

zooba · 2022-05-23T16:24:51Z

Misc/NEWS.d/next/Library/2020-09-28-04-56-04.bpo-14243.YECnxv.rst

@@ -0,0 +1,16 @@
+At the moment  ``NamedTemporaryFile`` is too hard to use portably when you


This is way more details than we want here - this will end up in a list with 1000 other changes.

Most of this should move to specific documentation, and then this entry can link to it with :class:`NamedTemporaryFile`

This is fixed. The spesific documentation is updated and linked to with :class:tempfile.NamedTemporaryFile``

zooba · 2022-05-23T16:24:51Z

Doc/whatsnew/3.10.rst

@@ -93,6 +93,8 @@ Other Language Changes
  :meth:`~object.__index__` method).
  (Contributed by Serhiy Storchaka in :issue:`37999`.)

+* XXX Describe changes to NamedTemporaryFile functionality. Re 2020-09-28-04-56-04.bpo-14243.YECnxv.rst


Needs to move to 3.12.rst, and can be about half the length of what's currently in the NEWS file

Implemented. Whats new is moved to 3.12.rst and only 2 lines now

bedevere-bot · 2022-05-23T16:24:55Z

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.

…t manager complaining

@zooba

Commiting changes, suggested by @zooba Co-authored-by: Steve Dower <steve.dower@microsoft.com>

…to 3.12 rst

Ev2geny · 2022-05-29T21:57:19Z

I have made the requested changes; please review again

bedevere-bot · 2022-05-29T21:57:22Z

Thanks for making the requested changes!

@zooba: please review the changes made to this pull request.

Doc/library/tempfile.rst

Doc/whatsnew/3.12.rst

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Doc/whatsnew/3.12.rst

Doc/library/tempfile.rst

changing issue number to github issue number in documentation Co-authored-by: Steve Dower <steve.dower@microsoft.com>

…eryksun

…eryksun

…into fix-issue-14243

…d on the feedback from AA-Turner

Ev2geny · 2022-06-07T06:45:51Z

@eryksun , @AA-Turner , I have implemented your comments, thank you! Can you please review the final version once again?

Doc/whatsnew/3.12.rst

Wrap lines at 70-80 chars in the Doc/whatsnew/3.12.rst Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

the-knights-who-say-ni added the CLA not signed label Sep 27, 2020

bedevere-bot added the awaiting review label Sep 27, 2020

Ev2geny changed the title ~~bro-14243: Optionally close NamedTemporaryFile on content manager exit and not on close~~ bro-14243: Optionally close NamedTemporaryFile on content manager exit and not on file close Sep 27, 2020

Ev2geny changed the title ~~bro-14243: Optionally close NamedTemporaryFile on content manager exit and not on file close~~ bro-14243: Optionally delete NamedTemporaryFile on content manager exit and not on file close Sep 28, 2020

Ev2geny changed the title ~~bro-14243: Optionally delete NamedTemporaryFile on content manager exit and not on file close~~ bro-14243: tempfile.NamedTemporaryFile not particularly useful on Windows Sep 28, 2020

Ev2geny changed the title ~~bro-14243: tempfile.NamedTemporaryFile not particularly useful on Windows~~ bro-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close Sep 28, 2020

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 29, 2020

Chary314 mentioned this pull request Oct 5, 2020

A lot of unittests tests fail under Windows due to usage of NamedTemporaryFile beancount/beancount#550

Open

encukou changed the title ~~bro-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close~~ bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close Dec 14, 2020

dabrahams mannequin mentioned this pull request Apr 10, 2022

tempfile.NamedTemporaryFile not particularly useful on Windows #58451

Open

kumaraditya303 requested review from zooba and eryksun May 21, 2022

zooba requested changes May 23, 2022

View changes

bedevere-bot added awaiting changes and removed awaiting review labels May 23, 2022

Ev2geny added 3 commits May 27, 2022

inial update. Tests do not work yet

144db7f

delete_on_close flag is added to NamedTemporaryFile functionality

21f7df0

possibility for user to delete temporary file is added without conten…

2d7927b

…t manager complaining

Update Lib/test/test_tempfile.py

589d0b8

Commiting changes, suggested by @zooba Co-authored-by: Steve Dower <steve.dower@microsoft.com>

Ev2geny force-pushed the fix-issue-14243 branch from 1b79266 to 589d0b8 Compare May 29, 2022

Ev2geny added 3 commits May 29, 2022

Doc/whatsnew/3.10.rst is restored from the main. changes will no go …

e22a3db

…to 3.12 rst

Documentation update

3be2f47

Minor documentation update

39e8b50

bedevere-bot added awaiting change review and removed awaiting changes labels May 29, 2022

bedevere-bot requested a review from zooba May 29, 2022

Ev2geny added 2 commits May 30, 2022

Training whitespaces removed in Doc/library/tempfile.rst

4d8f152

New line is added to the end of the news file

9ec55f5

AA-Turner suggested changes May 30, 2022

View changes

Doc/library/tempfile.rst Outdated Show resolved Hide resolved

Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved

Ev2geny and others added 2 commits May 30, 2022

Update Doc/library/tempfile.rst

d6f382e

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

Update Doc/whatsnew/3.12.rst

f9dc427

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

zooba approved these changes May 30, 2022

View changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 30, 2022

zooba reviewed May 30, 2022

View changes

Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved

eryksun reviewed May 31, 2022

View changes

Doc/library/tempfile.rst Outdated Show resolved Hide resolved

eryksun reviewed May 31, 2022

View changes

Doc/library/tempfile.rst Show resolved Hide resolved

Ev2geny and others added 4 commits May 31, 2022

Update Doc/whatsnew/3.12.rst

e29b7d2

changing issue number to github issue number in documentation Co-authored-by: Steve Dower <steve.dower@microsoft.com>

Documentation in tempfile.rst is updated based on the feedback from @…

d003a39

…eryksun

Merge branch 'fix-issue-14243' of https://github.com/Ev2geny/cpython …

e2fc6db

…into fix-issue-14243

Information in whatsnew is moved to the section Improved modules base…

3ac4e20

…d on the feedback from AA-Turner

Ev2geny requested review from eryksun and AA-Turner May 31, 2022

AA-Turner reviewed Jun 7, 2022

View changes

Doc/whatsnew/3.12.rst Outdated Show resolved Hide resolved

Update Doc/whatsnew/3.12.rst

653cf27

Wrap lines at 70-80 chars in the Doc/whatsnew/3.12.rst Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close #22431

bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close #22431

Ev2geny commented Sep 27, 2020 •

edited by bedevere-bot

the-knights-who-say-ni commented Sep 27, 2020

Ev2geny commented Sep 28, 2020 •

edited

blais commented Oct 11, 2020

Ev2geny commented Oct 12, 2020 •

edited

blais commented Oct 13, 2020

bozhinov commented Oct 18, 2020

willingc commented Dec 15, 2020

Ev2geny commented Jan 13, 2021

MaxwellDupre commented May 21, 2022

AlexWaygood commented May 21, 2022

zooba left a comment

zooba May 23, 2022

Ev2geny May 29, 2022

zooba May 23, 2022

Ev2geny May 29, 2022

bedevere-bot commented May 23, 2022

Ev2geny commented May 29, 2022

bedevere-bot commented May 29, 2022

Ev2geny commented Jun 7, 2022

		@@ -0,0 +1,16 @@
		At the moment ``NamedTemporaryFile`` is too hard to use portably when you

bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close #22431

Are you sure you want to change the base?

bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close #22431

Conversation

Ev2geny commented Sep 27, 2020 • edited by bedevere-bot

the-knights-who-say-ni commented Sep 27, 2020

Recognized GitHub username

Ev2geny commented Sep 28, 2020 • edited

blais commented Oct 11, 2020

Ev2geny commented Oct 12, 2020 • edited

blais commented Oct 13, 2020

bozhinov commented Oct 18, 2020

willingc commented Dec 15, 2020

Ev2geny commented Jan 13, 2021

MaxwellDupre commented May 21, 2022

AlexWaygood commented May 21, 2022

zooba left a comment

zooba May 23, 2022

Choose a reason for hiding this comment

Ev2geny May 29, 2022

Choose a reason for hiding this comment

zooba May 23, 2022

Choose a reason for hiding this comment

Ev2geny May 29, 2022

Choose a reason for hiding this comment

bedevere-bot commented May 23, 2022

Ev2geny commented May 29, 2022

bedevere-bot commented May 29, 2022

Ev2geny commented Jun 7, 2022

Ev2geny commented Sep 27, 2020 •

edited by bedevere-bot

Ev2geny commented Sep 28, 2020 •

edited

Ev2geny commented Oct 12, 2020 •

edited