Author paul_ollis
Recipients Albert.Zeyer, Seth.Troisi, eryksun, hitbox, joernheissler, nedbat, nneonneo, paul_ollis, sanjioh, serhiy.storchaka
Date 2020-01-15.19:58:22
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1579118302.65.0.470463967541.issue39318@roundup.psfhosted.org>
In-reply-to
Content
I think it is worth pointing out that the semantics of 

    f = ``open(fd, closefd=True)`` 

are broken (IMHO) because an exception can result in an unreferenced file
object that has taken over reponsibility for closing the fd, but it can
also fail without creating the file object.

Therefore it should be considered a bad idea to use open in this way. So
NamedTemporaryFile should take responsibility for closing the fd; i.e. it
should used closefd=False.

I would suggest that NamedTemporaryFile's code should be:

    try:
        file = _io.open(fd, mode, buffering=buffering, closefd=False,
                        newline=newline, encoding=encoding, errors=errors)
        return _TemporaryFileWrapper(file, name, delete)
    except BaseException:
        _os.close(fd)
        try:
            _os.unlink(name)
        except OSError:
            pass  # On windows the file will already be deleted.
        raise

And '_os.close(self.file.fileno())' should be added after each of the two calls
to 'self.file.close()' in _TemporaryFileCloser.

Some notes on the design choices here.

1. The exception handling performs the close *before* the unlink because:

   - That is the normal order that people expect.
   - It is most important that the file is closed. We cannot rule out the 
     possibility of the unlink raising an exception, which could leak the fd.

2. BaseException is caught because we should not leak a file descriptor for
   KeyboardInterrupt or any other exception.

3. It is generally good practice to protect os.unlink with a try ... except
   OSError. So I think this is an appropriate way to prevent the Windows
   specific error. 

   It will also suppress some other, rare but possible, errors. I think that
   this is legitimate behaviour, but it should be documented.
History
Date User Action Args
2020-01-15 19:58:22paul_ollissetrecipients: + paul_ollis, nneonneo, nedbat, Seth.Troisi, Albert.Zeyer, serhiy.storchaka, eryksun, sanjioh, joernheissler, hitbox
2020-01-15 19:58:22paul_ollissetmessageid: <1579118302.65.0.470463967541.issue39318@roundup.psfhosted.org>
2020-01-15 19:58:22paul_ollislinkissue39318 messages
2020-01-15 19:58:22paul_olliscreate