When calling the tempfile.NamedTemporaryFile with mode='wr' or with any other wrong value for "mode", it raises the ValueError and silently leaves an unknown, just created file on the file system.

Here is a naïve fix including a test.

This looks like an extension of bpo-21058. Does the unlink() work on Windows? It seems to me that the file is configured to remove itself on close(), therefore I expect unlink() will raise an exception of its own. Also made some suggestions in the code review.

This problem also affects Python 2, if you fudge the right wrong parameters:

>>> NamedTemporaryFile((), prefix="blaua.")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/proj/python/cpython/Lib/tempfile.py", line 477, in NamedTemporaryFile
    file = _os.fdopen(fd, mode, bufsize)
TypeError: argument 2 must be string, not tuple
[59140 refs]
>>> glob("/tmp/blaua.*")
['/tmp/blaua.AFtEqx']

Does the unlink() work on Windows?

Yes. O_TEMPORARY opens the file with FILE_SHARE_DELETE, so unlink won't raise an error.

Opening a file creates and returns a handle for a kernel File object that references the underlying file/link/stream control block in the file system. There may be multiple open File objects from separate NtCreateFile and NtOpenFile system calls, but they all reference a common file control block. Deleting a file requires a File handle, which is used to set the delete disposition in the control block. When all references (handle and pointer) to all File objects that reference the file are closed, the file is unlinked if the delete disposition is set.

The way delete-on-close works is to set a flag in the File object that causes the delete disposition to be automatically set when the File object is closed. However, any File handle that references the file can be used to set or unset this disposition if it has DELETE access. So it's harmless to manually call DeleteFile on the file (i.e. NtOpenFile and NtSetInformationFile to set the delete disposition) beforehand.

I wonder if Victor could clarify why bare except wasn't used in the python3 version.

Anyway, here is the updated patch testing for TypeError as well.

I wonder if Victor could clarify why bare except wasn't used in the python3 version.

What do you call a "bare except"?

I wrote "except Exception: <cleanup code>; raise", it doesn't ignore the error. I want to always call the cleanup code on error.

Victor: You changed “except Exception:” to bare “except:” in revision 182f08c0dd45 in Python 2, but the Python 3 code never got such a change.

Victor: You changed “except Exception:” to bare “except:” in revision 182f08c0dd45 in Python 2, but the Python 3 code never got such a change.

Ah strange :-) Python 3 must also be fixed.

`except:` is equivalent to `except BaseException:`. When I introduced the former into idlelib, a couple of years ago, I was told to do the latter in a follow-up patch, so that there would be no doubt as to the intent. The same should be done here.

I prefer "except:" over "except BaseException:". What is the benefit
of passing explicitly BaseException?

Eryk Sun: The patch proposes to add an unlink() call after the file has been closed:

except Exception:
    _os.close(fd)  # This automatically deletes the file right?
    _os.unlink(name)  # Won’t this raise FileNotFoundError?
    raise

By your explanation, it sounds like it would be better to call unlink() before close().

Terry & Victor: Writing the explicit “except BaseException:” makes it clear you weren’t being lazy in figuring out what exceptions you want to catch. But in this case the “raise” at the end of the the exception handler make it clear enough for me. I would be happy with either option.

It makes it clearer that you know that it will 'everything' and intend to catch do so and that you did not casually toss in 'except:', as used to be the habit of many. There are over 20 bare excepts in idlelib and I suspect many or most are unintentionally over-broad.

By your explanation, it sounds like it would be better
to call unlink() before close().

Sorry, I was responding in general, because I thought you meant unlink would fail like it would for most open files on Windows, because the CRT normally doesn't open files with delete sharing. But I see what you meant now. Yes, the order needs to be reversed as unlink() and then close() for this to work. Doing the close first does raise a FileNotFoundError.

Here is the updated patch including fixes for except and order of deletion.

I think patch 3 is good for Python 3, thankyou. For Python 2, the test will have to be adjusted. From memory, mode='wr' is accepted without an exception, and mode=2 triggers an early error (so we never trigger the bug).

Then, I think the TypeError check could be dropped and 'wr' replaced by an obviously wrong value, both seem fairly trivial. I don't have a working 2.7 checkout, so if anyone wants to extend the fix to that branch, they're more than welcome.

Here is my proposed version for Python 2.

New changeset 5bfb4147405e by Martin Panter in branch '2.7':
Issue bpo-26385: Cleanup NamedTemporaryFile if fdopen() fails, by SilentGhost
https://hg.python.org/cpython/rev/5bfb4147405e

New changeset a1c125f21db4 by Martin Panter in branch '3.5':
Issue bpo-26385: Cleanup NamedTemporaryFile if open() fails, by SilentGhost
https://hg.python.org/cpython/rev/a1c125f21db4

New changeset 865cf8eba51a by Martin Panter in branch 'default':
Issue bpo-26385: Merge NamedTemporaryFile fix from 3.5
https://hg.python.org/cpython/rev/865cf8eba51a