NamedTemporaryFile with delete=True should not fail if file already deleted

[richardxia]

BPO	29573
Nosy	@rhettinger, @tiran, @jwilk, @bitdancer, @vadmium, @andrewnester, @richardxia
PRs	bpo-29573: Fixed unlinking already removed NamedTemporaryFile. #134

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-11-25.18:38:39.961>
created_at = <Date 2017-02-15.21:21:19.117>
labels = ['type-bug', 'expert-IO']
title = 'NamedTemporaryFile with delete=True should not fail if file already deleted'
updated_at = <Date 2020-11-25.18:38:39.960>
user = 'https://github.com/richardxia'

bugs.python.org fields:

activity = <Date 2020-11-25.18:38:39.960>
actor = 'iritkatriel'
assignee = 'none'
closed = True
closed_date = <Date 2020-11-25.18:38:39.961>
closer = 'iritkatriel'
components = ['IO']
creation = <Date 2017-02-15.21:21:19.117>
creator = 'richardxia'
dependencies = []
files = []
hgrepos = []
issue_num = 29573
keywords = []
message_count = 16.0
messages = ['287888', '287946', '288156', '288612', '288686', '290083', '290144', '290642', '290650', '290651', '290656', '290658', '290661', '290665', '293559', '293562']
nosy_count = 7.0
nosy_names = ['rhettinger', 'christian.heimes', 'jwilk', 'r.david.murray', 'martin.panter', 'andrewnester', 'richardxia']
pr_nums = ['134']
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29573'
versions = ['Python 3.5', 'Python 3.6']

[richardxia]

Here is a very short program to demonstrate what I'm seeing:

>>> import tempfile
>>> import os
>>> with tempfile.NamedTemporaryFile(delete=True) as fp:
...     print(fp.name)
...     os.system('rm {}'.format(fp.name))
/tmp/tmpomw0udc6
0
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "/usr/local/lib/python3.6/tempfile.py", line 502, in __exit__
    self.close()
  File "/usr/local/lib/python3.6/tempfile.py", line 509, in close
    self._closer.close()
  File "/usr/local/lib/python3.6/tempfile.py", line 446, in close
    unlink(self.name)
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpomw0udc6'

In my specific use case, I am shelling out to another program (binutils' objcopy) and passing the path of the NamedTemporaryFile as the output file. objcopy apparently deletes the output file if it encounters an error, which causes NamedTemporaryFile's exit method to fail when it tries to delete the file.

While I can work around this by using delete=False and manually doing the cleanup on my own, it's less elegant than being able to rely on the normal context manager exit.

[andrewnester]

I've just added PR fixing this issue.

[andrewnester]

any updates on this? :)

[andrewnester]

some updates?

[jwilk]

Deleting non-existent files in /tmp is an indicator of a security hole.
This kind of error must never pass silently.

[andrewnester]

any updates?

[rhettinger]

Deleting non-existent files in /tmp is an indicator
of a security hole.

Christian, what is your take on this?

[tiran]

Raymond, I think Richard's approach is problematic at best.

Richard, you cannot use a NamedTempFile with an external process like that. At least you have to flush the file to disk. Even flushing is not safely portable. The safest and most portable approach is to close the file, call the other process and then unlink the file manually:

with tempfile.NamedTemporaryFile(delete=False) as f:
    try:    
        f.write(b'data')
        f.close()
        subprocess.Popen(['binutil', f.name, ...])
    finally:
        os.unlink(f.name)

It's too bad that close() on a NamedTemporaryFile(delete=True) deletes the files. For your problem it would be beneficial to have __exit__() perform the unlink operation.

[bitdancer]

He's still going to get an error using your code, Christian. But if he knows that the file being gone is OK, he can catch and ignore the error. Having exit do the unlink wouldn't help him; in that case he'd have to wrap the whole 'with' clause in a try/except.

So I think delete=False and handling the error "manually" is the correct solution here, since I doubt we want to add a "already_deleted_ok=False" flag to the API for this limited use case that can be handled with delete=False.

I vote for closing this as rejected.

[tiran]

RDM, I don't see how my example is going to fail. It uses delete=False with an explicit unlink. Please educate me. :)

[bitdancer]

His problem is that the file has already been deleted by the time the subprocess returns, so the unlink is going to raise an exception.

[richardxia]

Thanks for the discussion. I ended up doing something similar to the code snippet Christian posted, except I also had a second try/except FileNotFoundError within the original finally block to catch the case that David pointed out.

In retrospect, I probably should have used TemporaryDirectory since I am using Python 3.5 and because the file I was creating with NamedTemporaryFile was only being used as an output file, not an input file, to the subprocess command.

[tiran]

Ah! I missed the fact that the subprocess removes the file. It's easily fixable, wrap the unlink and catch FileNotFoundError / ENOENT.