Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-58451: Add optional delete_on_close parameter to NamedTemporaryFile #97015

Merged
merged 26 commits into from Oct 4, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3df957c
Brought the same changes as in now closed PR 22431, but to the latest…
Ev2geny Sep 21, 2022
c4cf39f
Based on the review from @eryksun the code and the documentation is u…
Ev2geny Sep 22, 2022
ac0b241
documentation is slightly corrected
Ev2geny Sep 22, 2022
384614f
minor documentation update
Ev2geny Sep 22, 2022
0e0b6ba
Update Doc/whatsnew/3.12.rst
Ev2geny Sep 22, 2022
cc81ab5
Deletion of file upon finalization is moved from _TemporaryFileWrappe…
Ev2geny Sep 23, 2022
5af6150
Some additions to Lib/test/test_tempfile.py
Ev2geny Sep 24, 2022
239354e
clearing of the files in the situation when delete and not delete_on_…
Ev2geny Sep 24, 2022
5de68c0
if statement is added to avoid errors, when testing. Not sure 100% is…
Ev2geny Sep 24, 2022
eef3439
Apply suggestions from code review
Ev2geny Sep 25, 2022
c9801ee
__del__ was defined twice in _TemporaryFileWrapper. Fixed now
Ev2geny Sep 25, 2022
6944ab2
_TemporaryFileCloser is returned to original state and the logic, whi…
Ev2geny Sep 25, 2022
44f0664
All delete operations are moved to the class _TemporaryFileCloser
Ev2geny Sep 25, 2022
1dfaf72
Minor update to comments
Ev2geny Sep 25, 2022
e54feb5
Whitespec fixes by patchcheck.py
Ev2geny Sep 25, 2022
d2d1119
Update Lib/tempfile.py
Ev2geny Sep 26, 2022
bd4b2dd
Update Lib/tempfile.py
Ev2geny Sep 26, 2022
29eeaff
Modified exception handling in NamedTemporaryFile
Ev2geny Sep 26, 2022
5306b6d
Wrapping at 80 characters is fixed
Ev2geny Sep 26, 2022
1c99fb3
Update Lib/tempfile.py
Ev2geny Oct 1, 2022
29b89d3
Documentation update, based on the feedback from @zooba
Ev2geny Oct 1, 2022
e586028
Minor PEP8 update based on the feedback from @zooba
Ev2geny Oct 1, 2022
3eceb45
Improving comments in the init test file, based on the feedback from …
Ev2geny Oct 1, 2022
892e87d
Updating of documentation
Ev2geny Oct 2, 2022
67996b2
Merge branch 'fix-issue-14243' of https://github.com/Ev2geny/cpython …
Ev2geny Oct 2, 2022
07b963e
Apply suggestions from code review
zooba Oct 4, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
80 changes: 66 additions & 14 deletions Doc/library/tempfile.rst
Expand Up @@ -75,20 +75,53 @@ The module defines the following user-callable items:
Added *errors* parameter.


.. function:: NamedTemporaryFile(mode='w+b', buffering=-1, encoding=None, newline=None, suffix=None, prefix=None, dir=None, delete=True, *, errors=None)

This function operates exactly as :func:`TemporaryFile` does, except that
the file is guaranteed to have a visible name in the file system (on
Unix, the directory entry is not unlinked). That name can be retrieved
from the :attr:`name` attribute of the returned
file-like object. Whether the name can be
used to open the file a second time, while the named temporary file is
still open, varies across platforms (it can be so used on Unix; it cannot
on Windows). If *delete* is true (the default), the file is
deleted as soon as it is closed.
The returned object is always a file-like object whose :attr:`!file`
attribute is the underlying true file object. This file-like object can
be used in a :keyword:`with` statement, just like a normal file.
.. function:: NamedTemporaryFile(mode='w+b', buffering=-1, encoding=None, newline=None, suffix=None, prefix=None, dir=None, delete=True, *, errors=None, delete_on_close=True)

This function operates exactly as :func:`TemporaryFile` does, except the
following differences:

* The file is guaranteed to have a visible name in the file system (on Unix,
the directory entry is not unlinked).

* There is more granularity in the deletion behaviour of the file (see
``delete_on_close`` below)
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved

The returned object is always a :term:`file-like object` whose :attr:`!file`
attribute is the underlying true file object. This :term:`file-like object` can be
used in a :keyword:`with` statement, just like a normal file. The name of the
temporary file can be retrieved from the :attr:`name` attribute of the
returned file-like object.

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
:term:`file-like object` is finalized and hence deletion is not always
guaranteed in this case (see :meth:`object.__del__`). If *delete* is false,
the value of *delete_on_close* is ignored.
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved

While the named temporary file is open, the file can always be opened again
on POSIX. On Windows, it can be opened again if *delete* is false, or if
*delete_on_close* is false, or if the additional open shares delete access
(e.g. by calling :func:`os.open` with the flag ``O_TEMPORARY``). On
Windows, if *delete* is true and *delete_on_close* is false, additional
opens that do not share delete access (e.g. via builtin :func:`open`) must
be closed before exiting the context manager, else the :func:`os.unlink`
call on context manager exit will fail with a :exc:`PermissionError`.
Copy link
Member

Choose a reason for hiding this comment

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

Can we reframe this as:

  • the file can be opened again using its name
  • on Windows, subsequent opens should share delete access (e.g.)
  • however, when delete or delete_on_close are false, sharing delete access is not necessary, but you should ensure the file is closed before leaving the context manager

This sets it up more positively for the user - what should they do, as opposed to what could go wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check how I have written this in the new version. Not sure everybody would agree, but I have now written this information the way I would want it to be explained for me without the risk of twisting my brains.

Additional question: does it make sense to write unit tests to cover all these combinations of opening the file the second time, while it is already open. I have tested this for myself (on Windows) to make sure it is all correct, but I am not sure that it shall go to unit tests.


To use the name of the temporary file to open the closed file second time,
either make sure not to delete the file upon closure (set the *delete*
parameter to be false) or, in case the temporary file is created in a
:keyword:`with` statement, set the *delete_on_close* parameter to be false.
The latter approach is recommended as it provides assistance in automatic
cleaning of the temporary file upon the context manager exit.
zooba marked this conversation as resolved.
Show resolved Hide resolved

On Windows, if *delete_on_close* is false, and the file is created in a
directory for which the user lacks delete access, then the :func:`os.unlink`
call on exit of the context manager will fail with a :exc:`PermissionError`.
This cannot happen when *delete_on_close* is true because delete access is
requested by the open, which fails immediately if the requested access is not
granted.

On POSIX (only), a process that is terminated abruptly with SIGKILL
cannot automatically delete any NamedTemporaryFiles it created.
Expand All @@ -98,6 +131,9 @@ The module defines the following user-callable items:
.. versionchanged:: 3.8
Added *errors* parameter.

.. versionchanged:: 3.12
Added *delete_on_close* parameter.


.. class:: SpooledTemporaryFile(max_size=0, mode='w+b', buffering=-1, encoding=None, newline=None, suffix=None, prefix=None, dir=None, *, errors=None)

Expand Down Expand Up @@ -346,6 +382,22 @@ Here are some examples of typical usage of the :mod:`tempfile` module::
>>>
# file is now closed and removed

# create a temporary file using a context manager, note the name,
# close the file, use the name to open the file again
>>> with tempfile.TemporaryFile(delete_on_close=False) as fp:
... fp.write(b'Hello world!')
... fp_name = fp.name
... fp.close()
# the file is closed, but not removed
# open the file again by using its name
... f = open(fp_name)
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
... f.seek(0)
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
... f.read()
b'Hello world!'
... f.close()
>>>
# file is now removed

# create a temporary directory using the context manager
>>> with tempfile.TemporaryDirectory() as tmpdirname:
... print('created temporary directory', tmpdirname)
Expand Down
5 changes: 5 additions & 0 deletions Doc/whatsnew/3.12.rst
Expand Up @@ -142,6 +142,11 @@ unicodedata
* The Unicode database has been updated to version 15.0.0. (Contributed by
Benjamin Peterson in :gh:`96734`).

tempfile
--------

The :class:`tempfile.NamedTemporaryFile` function has a new optional parameter
*delete_on_close* (Contributed by Evgeny Zorin in :gh:`58451`.)

Optimizations
=============
Expand Down
42 changes: 34 additions & 8 deletions Lib/tempfile.py
Expand Up @@ -421,10 +421,11 @@ class _TemporaryFileCloser:
file = None # Set here since __del__ checks it
close_called = False

def __init__(self, file, name, delete=True):
def __init__(self, file, name, delete=True, delete_on_close=True):
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
self.file = file
self.name = name
self.delete = delete
self.delete_on_close = delete_on_close

# NT provides delete-on-close as a primitive, so we don't need
# the wrapper to do anything special. We still use it so that
Expand All @@ -442,7 +443,7 @@ def close(self, unlink=_os.unlink):
try:
self.file.close()
finally:
if self.delete:
if self.delete and self.delete_on_close:
unlink(self.name)

# Need to ensure the file is deleted on __del__
Expand All @@ -464,11 +465,13 @@ class _TemporaryFileWrapper:
remove the file when it is no longer needed.
"""

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

def __getattr__(self, name):
# Attribute lookups are delegated to the underlying file
Expand Down Expand Up @@ -500,6 +503,15 @@ def __enter__(self):
def __exit__(self, exc, value, tb):
result = self.file.__exit__(exc, value, tb)
self.close()
# If the file is to be deleted, but not on close, delete it now.
if self.delete and not self.delete_on_close:
try:
_os.unlink(self.name)
# It is okay to ignore FileNotFoundError. The file may have
# been deleted already.
except FileNotFoundError:
pass

return result

def close(self):
Expand All @@ -518,18 +530,32 @@ def __iter__(self):
for line in self.file:
yield line

def __del__(self):
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
# This is to delete the temporary file in case delete = True,
# delete_on_close = False and no context manager was used
if self.delete:
try:
_os.unlink(self.name)
# It is okay to ignore FileNotFoundError. The file may have
# been deleted already.
except FileNotFoundError:
pass

Ev2geny marked this conversation as resolved.
Show resolved Hide resolved

def NamedTemporaryFile(mode='w+b', buffering=-1, encoding=None,
newline=None, suffix=None, prefix=None,
dir=None, delete=True, *, errors=None):
dir=None, delete=True, *, errors=None,
delete_on_close=True):
"""Create and return a temporary file.
Arguments:
'prefix', 'suffix', 'dir' -- as for mkstemp.
'mode' -- the mode argument to io.open (default "w+b").
'buffering' -- the buffer size argument to io.open (default -1).
'encoding' -- the encoding argument to io.open (default None)
'newline' -- the newline argument to io.open (default None)
'delete' -- whether the file is deleted on close (default True).
'delete' -- whether the file is automatically deleted (default True).
'delete_on_close' -- if 'delete', whether the file is deleted on close
or on context exit (default True).
'errors' -- the errors argument to io.open (default None)
The file is created as mkstemp() would do it.

Expand All @@ -548,7 +574,7 @@ def NamedTemporaryFile(mode='w+b', buffering=-1, encoding=None,

# Setting O_TEMPORARY in the flags causes the OS to delete
# the file when it is closed. This is only supported by Windows.
if _os.name == 'nt' and delete:
if _os.name == 'nt' and delete and delete_on_close:
flags |= _os.O_TEMPORARY

if "b" not in mode:
Expand All @@ -567,7 +593,7 @@ def opener(*args):
raw = getattr(file, 'buffer', file)
raw = getattr(raw, 'raw', raw)
raw.name = name
return _TemporaryFileWrapper(file, name, delete)
return _TemporaryFileWrapper(file, name, delete, delete_on_close)
except:
file.close()
raise
Expand Down
82 changes: 82 additions & 0 deletions Lib/test/test_tempfile.py
Expand Up @@ -979,6 +979,88 @@ def test_del_on_close(self):
finally:
os.rmdir(dir)

def test_not_del_on_close_if_delete_on_close_false(self):
# A NamedTemporaryFile is NOT deleted when closed if
# delete_on_close= False, but is deleted on content manager exit
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
dir = tempfile.mkdtemp()
try:
with tempfile.NamedTemporaryFile(dir=dir,
delete=True,
delete_on_close=False) as f:
f.write(b'blat')
f_name = f.name
f.close()
with self.subTest():
# Testing that file is not deleted on close
self.assertTrue(os.path.exists(f.name),
"NamedTemporaryFile %s is incorrectly deleted\
on closure when delete_on_close= False" % f_name)
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved

with self.subTest():
# Testing that file is deleted on content manager exit
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
self.assertFalse(os.path.exists(f.name),
"NamedTemporaryFile %s exists\
after content manager exit" % f.name)
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved

finally:
os.rmdir(dir)

def test_ok_to_delete_manually(self):
# A NamedTemporaryFile can be deleted by a user before content manager
# comes to it. This will not generate an error
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved

dir = tempfile.mkdtemp()
try:
with tempfile.NamedTemporaryFile(dir=dir,
delete=True,
delete_on_close=False) as f:
f.write(b'blat')
f.close()
os.unlink(f.name)

finally:
os.rmdir(dir)

def test_not_del_if_delete_false(self):
# A NamedTemporaryFile is not deleted if delete = False
dir = tempfile.mkdtemp()
f_name = ""
try:
# setting delete_on_close = True to test, that this does not have
# an effect, if delete = False
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
with tempfile.NamedTemporaryFile(dir=dir, delete=False,
delete_on_close=True) as f:
f.write(b'blat')
f_name = f.name
self.assertTrue(os.path.exists(f.name),
"NamedTemporaryFile %s exists after close" % f.name)
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
finally:
os.unlink(f_name)
os.rmdir(dir)

def test_del_by_finalizer_if_no_with(self):
# A NamedTemporaryFile is deleted by fanalizer in case delete = True
# delete_on_close = False and no context manager is used
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved
dir = tempfile.mkdtemp()
try:
f = tempfile.NamedTemporaryFile(dir=dir, delete=True,
delete_on_close=False)
tmp = f.name
f.write(b'blat')
f.close()
with self.subTest():
self.assertTrue(os.path.exists(tmp),
"NamedTemporaryFile %s missing after close"\
% tmp)
# Simulating finalizer
f.__del__()
with self.subTest():
self.assertFalse(os.path.exists(tmp),
"NamedTemporaryFile %s exists after finalizer "\
% f.name)
finally:
os.rmdir(dir)

def test_dis_del_on_close(self):
# Tests that delete-on-close can be disabled
dir = tempfile.mkdtemp()
Expand Down
@@ -0,0 +1 @@
The :class:`tempfile.NamedTemporaryFile` has a new optional parameter *delete_on_close*
Ev2geny marked this conversation as resolved.
Show resolved Hide resolved