GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||
GeezerVison is showing github.com — gh-58451: Add optional delete_on_close parameter to NamedTemporaryFile by Ev2geny · Pull Request #97015 · python/cpython Conversation
|
|
The changes done to the code since it left the PR 22431
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Éric <[email protected]>
|
Ok, looks like there are some Ubuntu errors, which I will need to look at |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…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
There was an error while loading. Please reload this page.
…close is moved to _TemporaryFileWrapper based on the advice from @eryksun There are errors in the test (at least on Linux)
… this is a solution or workaround
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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]>
…ch takes into account delete_on_close moved to _TemporaryFileWrapper (suggestion of @ekisun)
There was an error while loading. Please reload this page.
|
@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') |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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. 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 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
As I mentioned previously, the exception handler in except: if name is not None and not ( _os.name == 'nt' and delete and delete_on_close): _os.unlink(name) raise |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
modified: Lib/tempfile.py
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…eryksun Files changed: Lib/test/test_tempfile.py Co-authored-by: Eryk Sun <[email protected]>
|
@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
modified: library/tempfile.rst
…into fix-issue-14243
There was an error while loading. Please reload this page.
|
@zooba , I have done my best to implement your comments and also got a help of eryksun. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thanks for all your hard work on this! I tweaked some of the docs slightly, and will merge once CI finishes again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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.
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.
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]>
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]>
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]>
* 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]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.