GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-58451: Add optional delete_on_close parameter to NamedTemporaryFile by Ev2geny · Pull Request #97015 · python/cpython

Conversation

@Ev2geny

@Ev2geny Ev2geny commented Sep 22, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This is a reincarnation of the already marked as awaiting merge PR 22431, which was subsequently closed due to some git mistake.

This PR fixes issue 58451: tempfile.NamedTemporaryFile not particularly useful on Windows

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:
  1. create and open temporary file
  2. write data to it
  3. pass name of the temporary file to the operational code
  • In operational code, being tested
  1. open file, using name of the temporary file
  2. 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 the deletion behaviour.

If delete is true (the default) and delete_on_close is true (the default), the file is deleted as soon as it is closed. If delete is true and delete_on_close is false, the file is deleted on context manager exit only, if no context manager was used, then the file is deleted only when the file-like object is finalized. If delete is false, the value of delete_on_close is ignored.

So, the change shall be fully backwards compatible.

  • Issue: tempfile.NamedTemporaryFile not particularly useful on Windows #58451

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dlech reacted with hooray emoji
All reactions
  • 🎉 1 reaction
Ev2geny added 4 commits September 22, 2022 00:30
@Ev2geny
Brought the same changes as in now closed PR 22431, but to the latest…
3df957c
… version of main
@Ev2geny
Based on the review from @eryksun the code and the documentation is u…
c4cf39f
…pdated to handle the situation, when delete = True, delete_on_close = Flase and no context manager was used
@Ev2geny
documentation is slightly corrected
ac0b241
@Ev2geny
minor documentation update
384614f
@bedevere-bot bedevere-bot added the awaiting review label Sep 22, 2022
@Ev2geny

Ev2geny commented Sep 22, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

The changes done to the code since it left the PR 22431

  • Merge conflicts are resolved

  • A new functionality is added based on the review feedback from @eryksun to cover the case if delete is true and delete_on_close is false, but the file is never used in a with context. The new functionality is that in this case the file is deleted when the instance is finalized. This is implemented by adding a __del__() method to the _TemporaryFileCloser. This required documentation update as well.
    @eryksun , please have a look at the implementation as well as documentation update.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Ev2geny Ev2geny mentioned this pull request Sep 22, 2022
bpo-14243: Optionally delete NamedTemporaryFile on content manager exit but not on file close #22431
Closed
merwok
merwok reviewed Sep 22, 2022
View reviewed changes
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

merwok
merwok reviewed Sep 22, 2022
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2020-09-28-04-56-04.bpo-14243.YECnxv.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Ev2geny @merwok
Update Doc/whatsnew/3.12.rst
0e0b6ba
Co-authored-by: Éric <[email protected]>
@Ev2geny

Ev2geny commented Sep 22, 2022

Copy link
Copy Markdown
Contributor Author

Ok, looks like there are some Ubuntu errors, which I will need to look at

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eryksun
eryksun reviewed Sep 22, 2022
View reviewed changes
Comment thread Lib/tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Ev2geny
Deletion of file upon finalization is moved from _TemporaryFileWrappe…
cc81ab5
…r to _TemporaryFileCloser

Test test_del_by_finalizer_if_no_with is changed from simulating finalizer by calling __del__() to making
sure finalizer really runs
eryksun
eryksun reviewed Sep 23, 2022
View reviewed changes
Comment thread Lib/tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Ev2geny added 3 commits September 25, 2022 00:07
@Ev2geny
Some additions to Lib/test/test_tempfile.py
5af6150
@Ev2geny
clearing of the files in the situation when delete and not delete_on_…
239354e
…close is moved to _TemporaryFileWrapper

based on the advice from @eryksun

There are errors in the test (at least on Linux)
@Ev2geny
if statement is added to avoid errors, when testing. Not sure 100% is…
5de68c0
… this is a solution or workaround
eryksun
eryksun reviewed Sep 25, 2022
View reviewed changes
Comment thread Lib/tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

eryksun
eryksun reviewed Sep 25, 2022
View reviewed changes
Comment thread Lib/tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

eryksun
eryksun reviewed Sep 25, 2022
View reviewed changes
Comment thread Lib/tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

ezio-melotti
ezio-melotti reviewed Sep 25, 2022
View reviewed changes
Comment thread Doc/library/tempfile.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

eryksun
eryksun reviewed Sep 25, 2022
View reviewed changes
Comment thread Lib/tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Ev2geny and others added 3 commits September 25, 2022 09:59
@Ev2geny @ezio-melotti
@eryksun @merwok
Apply suggestions from code review
eef3439
Implemented review from @eryksun and @merwok

Co-authored-by: Ezio Melotti <[email protected]>
Co-authored-by: Eryk Sun <[email protected]>
Co-authored-by: Éric <[email protected]>
@Ev2geny
__del__ was defined twice in _TemporaryFileWrapper. Fixed now
c9801ee
@Ev2geny
_TemporaryFileCloser is returned to original state and the logic, whi…
6944ab2
…ch takes into account delete_on_close moved to _TemporaryFileWrapper (suggestion of @ekisun)
@Ev2geny

Ev2geny commented Sep 25, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@eryksun , I have implemented all suggestions, but it now fails the unit test for the issue18879

    def test_method_lookup(self):
        # Issue #18879: Looking up a temporary file method should keep it
        # alive long enough.
        f = self.do_create()
        wr = weakref.ref(f)
        write = f.write
        write2 = f.write
        del f
        write(b'foo')
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@eryksun

eryksun commented Sep 25, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I have implemented all suggestions, but it now fails the unit test for the issue18879

The delegated attribute cache is a problem. It leads to a reference cycle if the function wrapper (bpo-18879) is changed to keep a reference to the file wrapper (i.e. self) instead of the file closer (i.e. self._closer). Apparently avoiding this reference cycle was the reason the closer was added.

So, sorry to have misled you, but it looks like we're going to have to implement all delete operations in the closer. The wrapper just needs to change __init__() to instantiate the closer with delete_on_close, and change __exit__() to call the closer's cleanup() method. The latter includes a call to self.file.close() if needed. Since all deleting is handled by the closer, there's no need to implement __del__() in the wrapper. For example:

class _TemporaryFileCloser:
    """A separate object allowing proper closing of a temporary file's
    underlying file object, without adding a __del__ method to the
    temporary file."""

    cleanup_called = False
    close_called = False

    def __init__(self, file, name, delete=True, delete_on_close=True):
        self.file = file
        self.name = name
        self.delete = delete
        self.delete_on_close = delete_on_close

    def cleanup(self, windows=(_os.name == 'nt'), unlink=_os.unlink):
        if not self.cleanup_called:
            self.cleanup_called = True
            try:
                if not self.close_called:
                    self.close_called = True
                    self.file.close()
            finally:
                if self.delete and not (self.delete_on_close and windows):
                    try:
                        unlink(self.name)
                    except FileNotFoundError:
                        pass

    def close(self):
        if not self.close_called:
            self.close_called = True
            try:
                self.file.close()
            finally:
                if self.delete and self.delete_on_close:
                    self.cleanup()

    def __del__(self):
        self.cleanup()
class _TemporaryFileWrapper:
    """Temporary file wrapper

    This class provides a wrapper around files opened for
    temporary use.  In particular, it seeks to automatically
    remove the file when it is no longer needed.
    """

    def __init__(self, file, name, delete=True, delete_on_close=True):
        self.file = file
        self.name = name
        self._closer = _TemporaryFileCloser(file, name, delete,
                                            delete_on_close)

    def __getattr__(self, name):
        # Attribute lookups are delegated to the underlying file
        # and cached for non-numeric results
        # (i.e. methods are cached, closed and friends are not)
        file = self.__dict__['file']
        a = getattr(file, name)
        if hasattr(a, '__call__'):
            func = a
            @_functools.wraps(func)
            def func_wrapper(*args, **kwargs):
                return func(*args, **kwargs)
            # Avoid closing the file as long as the wrapper is alive,
            # see issue #18879.
            func_wrapper._closer = self._closer
            a = func_wrapper
        if not isinstance(a, int):
            setattr(self, name, a)
        return a

    # The underlying __enter__ method returns the wrong object
    # (self.file) so override it to return the wrapper
    def __enter__(self):
        self.file.__enter__()
        return self

    # Need to trap __exit__ as well to ensure the file gets
    # deleted when used in a with statement
    def __exit__(self, exc, value, tb):
        result = self.file.__exit__(exc, value, tb)
        self._closer.cleanup()
        return result

    def close(self):
        """
        Close the temporary file, possibly deleting it.
        """
        self._closer.close()

    # iter() doesn't use __getattr__ to find the __iter__ method
    def __iter__(self):
        # Don't return iter(self.file), but yield from it to avoid closing
        # file as long as it's being used as iterator (see issue #23700).  We
        # can't use 'yield from' here because iter(file) returns the file
        # object itself, which has a close method, and thus the file would get
        # closed when the generator is finalized, due to PEP380 semantics.
        for line in self.file:
            yield line

This passes all tests for me in both Linux and Windows.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@eryksun

eryksun commented Sep 25, 2022

Copy link
Copy Markdown
Contributor

As I mentioned previously, the exception handler in NamedTemporaryFile() also needs to be changed to check delete_on_close:

    except:
        if name is not None and not (
          _os.name == 'nt' and delete and delete_on_close):
            _os.unlink(name)
        raise
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ev2geny added 2 commits September 25, 2022 19:28
@Ev2geny
All delete operations are moved to the class _TemporaryFileCloser
44f0664
	modified:   Lib/tempfile.py
@Ev2geny
Minor update to comments
1dfaf72
eryksun
eryksun reviewed Oct 1, 2022
View reviewed changes
Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

eryksun
eryksun reviewed Oct 1, 2022
View reviewed changes
Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

eryksun
eryksun reviewed Oct 1, 2022
View reviewed changes
Comment thread Lib/test/test_tempfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Ev2geny @eryksun
Improving comments in the init test file, based on the feedback from @…
3eceb45
…eryksun


Files changed:
      Lib/test/test_tempfile.py

Co-authored-by: Eryk Sun <[email protected]>
@Ev2geny

Ev2geny commented Oct 2, 2022

Copy link
Copy Markdown
Contributor Author

@eryksun , can you please have a look at this discussion about the fact, that the directory entry is not unlinked on Unix. I think you a correct person to have an idea there.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ev2geny added 2 commits October 2, 2022 19:55
@Ev2geny
Updating of documentation
892e87d
	modified:   library/tempfile.rst
@Ev2geny
Merge branch 'fix-issue-14243' of https://github.com/Ev2geny/cpython …
67996b2
…into fix-issue-14243
@Ev2geny

Ev2geny commented Oct 4, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@zooba , I have done my best to implement your comments and also got a help of eryksun.
Can you look at it again please?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba
zooba approved these changes Oct 4, 2022
View reviewed changes
Comment thread Doc/library/tempfile.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/tempfile.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Oct 4, 2022
@zooba
Apply suggestions from code review
07b963e
@zooba

zooba commented Oct 4, 2022

Copy link
Copy Markdown
Member

Thanks for all your hard work on this! I tweaked some of the docs slightly, and will merge once CI finishes again.

Ev2geny reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zooba
zooba merged commit 743453a into python:main Oct 4, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 4, 2022
@pfmoore pfmoore mentioned this pull request Oct 5, 2022
tempfile.NamedTemporaryFile not particularly useful on Windows #58451
Closed
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@Ev2geny @mpage
pythongh-58451: Add optional delete_on_close parameter to NamedTempor…
75af114
…aryFile (pythonGH-97015)
@felixxm felixxm mentioned this pull request Mar 2, 2023
Refs #34118 -- Used delete_on_close argument of tempfile.NamedTemporaryFile on Windows and Python 3.12. django/django#16611
Closed
felixxm added a commit to felixxm/django that referenced this pull request Mar 2, 2023
@felixxm
Refs #34118 -- Used delete_on_close argument of tempfile.NamedTempora…
a95a8da
…ryFile on Windows and Python 3.12.

delete_on_close is available in Python 3.12:
- python/cpython#58451
- python/cpython#97015
so we don't need a custom NamedTemporaryFile implementation anymore.
@c00kiemon5ter c00kiemon5ter mentioned this pull request Jun 8, 2023
Fixing temporary file creation for Windows IdentityPython/pysaml2#665
Open
Eta0 added a commit to coreweave/tensorizer that referenced this pull request Jan 30, 2024
@Eta0
fix(stream_io): Finalize temporary files correctly on __exit__
8849642
The temporary file finalizer's check that the file is not already closed
was causing the upload to be skipped if the stream was used as a context
manager without an explicit call to .close(), because CPython's
implementation of .__exit__() for NamedTemporaryFile objects closes
the underlying file before calling the wrapper's own .close() method.
After changing to `weakref.finalize` in commit 3fd7f1e, uploading
temporary files became inherently idempotent, so the check that the file
is not already closed is no longer necessary anyway.
This change deletes that check.

Furthermore, changes to the NamedTemporaryFile implementation with the
addition of its delete_on_close parameter in Python 3.12
(python/cpython#97015) make its .__exit__() method no longer call
its .close() method at all, so this change also embeds the finalizer
as a hook in a wrapper around both .close() and .__exit__() separately.
@Eta0 Eta0 mentioned this pull request Jan 30, 2024
fix(stream_io): Finalize temporary files correctly on __exit__ coreweave/tensorizer#72
Merged
jmartin-tech added a commit to jmartin-tech/garak that referenced this pull request Apr 24, 2024
@jmartin-tech
portable temp file remove for 3.10+
be975ef
Remove temp files manually for OS portablity in python 3.10+
`delete_on_close` for temp files is added in [3.12](python/cpython#97015)

Signed-off-by: Jeffrey Martin <[email protected]>
jmartin-tech added a commit to jmartin-tech/garak that referenced this pull request Apr 24, 2024
@jmartin-tech
portable temp file remove for 3.10+
a370b78
Remove temp files manually for OS portablity in python 3.10+
`delete_on_close` for temp files is added in [3.12](python/cpython#97015)

Signed-off-by: Jeffrey Martin <[email protected]>
jmartin-tech added a commit to jmartin-tech/garak that referenced this pull request Apr 24, 2024
@jmartin-tech
portable temp file remove for 3.10+
a481e1d
Remove temp files manually for OS portablity in python 3.10+
`delete_on_close` for temp files is added in [3.12](python/cpython#97015)

Signed-off-by: Jeffrey Martin <[email protected]>
leondz pushed a commit to NVIDIA/garak that referenced this pull request Apr 26, 2024
@jmartin-tech
Enable windows tests as github action (#626)
79c7649
* align path access for OS agnostic support

* rely on os.sep when call in `split()` on path string
* lean in on Path `/` operator when combining with a Path object

Signed-off-by: Jeffrey Martin <[email protected]>

* retrieve executable from runtime

* avoid hard coded executbale path by asking runtime path
* guard file removal during cleanup

Signed-off-by: Jeffrey Martin <[email protected]>

* force report digest output as `utf-8`

Signed-off-by: Jeffrey Martin <[email protected]>

* test updates for windows temp files

Signed-off-by: Jeffrey Martin <[email protected]>

* set encoding for all file operations without `binary` flag

Signed-off-by: Jeffrey Martin <[email protected]>

* portable temp file remove for 3.10+

Remove temp files manually for OS portablity in python 3.10+
`delete_on_close` for temp files is added in [3.12](python/cpython#97015)

Signed-off-by: Jeffrey Martin <[email protected]>

* add windows pytest

Signed-off-by: Jeffrey Martin <[email protected]>

* update ggml temp file

Signed-off-by: Jeffrey Martin <[email protected]>

* variable name precision

Signed-off-by: Jeffrey Martin <[email protected]>

* manual windows testing only

Signed-off-by: Jeffrey Martin <[email protected]>

---------

Signed-off-by: Jeffrey Martin <[email protected]>
@krebslw krebslw mentioned this pull request Aug 16, 2024
Manglende grafikvisning Klimadatastyrelsen/FIRE#682
Closed
@Avasam Avasam mentioned this pull request Mar 11, 2025
Fix mypy test temporary config file creation python/typeshed#13620
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@eryksun eryksun eryksun left review comments
@ezio-melotti ezio-melotti ezio-melotti left review comments
@zooba zooba zooba approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@Ev2geny @eryksun @zooba @merwok @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.