classification
Title: NamedTemporaryFile does not generate a ResourceWarning for unclosed files (unlike TemporaryFile)
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: SilentGhost, jdufresne, r.david.murray, vstinner
Priority: normal Keywords: patch

Created on 2016-12-04 17:47 by jdufresne, last changed 2017-06-04 03:13 by jdufresne.

Files
File name Uploaded Description Edit
namedtemporaryfile-resourcewarning.patch jdufresne, 2016-12-07 05:29 Patch to add ResourceWarning to NamedTemporaryFile review
namedtemporaryfile-resourcewarning-2.patch jdufresne, 2016-12-07 15:52 Updated patch review
namedtemporaryfile-resourcewarning-3.patch jdufresne, 2016-12-11 16:53 Patch take 3 review
namedtemporaryfile-resourcewarning-4.patch jdufresne, 2016-12-12 00:46 Patch take 4 review
Pull Requests
URL Status Linked Edit
PR 1936 open jdufresne, 2017-06-04 03:13
Messages (11)
msg282352 - (view) Author: Jon Dufresne (jdufresne) * Date: 2016-12-04 17:47
When using unittest, I'll frequently enable -Wall to help catch code smells and potential bugs.

One feature of this, I'm told when files aren't explicitly closed with an error like: "ResourceWarning: unclosed file <...>". I've noticed this warning is generated for an unclosed tempfile.TemporaryFile (good), but not for an unclosed tempfile.NamedTemporaryFile (possible bug).

I've verified this on Python 3.5 and 3.6.

Example test:

```
import tempfile
import unittest


class MyTest(unittest.TestCase):
    def test_temporary_file(self):
        tempfile.TemporaryFile()

    def test_named_temporary_file(self):
        tempfile.NamedTemporaryFile()
```

Actual output:

```
$ Python-3.6.0b4/python --version
Python 3.6.0b4
$ Python-3.6.0b4/python -Wall -m unittest -v test
test_named_temporary_file (test.MyTest) ... ok
test_temporary_file (test.MyTest) ... /home/jon/test.py:7: ResourceWarning: unclosed file <_io.BufferedRandom name=3>
  tempfile.TemporaryFile()
ok

----------------------------------------------------------------------
Ran 2 tests in 0.004s

OK

```

Expected:

I expect both tests to generate a ResourceWarning, as neither test explicitly closes the file.
msg282355 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-12-04 18:49
Looking at the code it seems NamedTemporaryFile is closed on destruction of the object, so there is no need for ResourceWarning, because the file descriptor is not leaking.

You also don't need to involve unittest here. Just running python with -Wall would generate a warning if there is a need for one.
msg282360 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-12-04 20:45
The point of ResourceWarning it tell you when you are depending on destructors for cleanup, so this does look like a bug.
msg282366 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-12-04 21:12
> The point of ResourceWarning it tell you when you are depending on destructors for cleanup
But there is an explicit call in __del__ to .close(), it's looks like intended behaviour and is ultimately the difference that Jon was seeing between NamedTemporaryFile and TemporaryFile.
msg282372 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-12-04 22:29
__del__ is a destructor.  If the file is open when it is called, it should emit a ResourceWarning.
msg282599 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-12-07 07:49
The ResourceWarning that you've introduced need to be "fixed" in a few place in test_urllib2
msg282629 - (view) Author: Jon Dufresne (jdufresne) * Date: 2016-12-07 15:52
Thanks for the review. I have updated the patch. Now all warnings during tests handled. Please let me know if there are any other concerns with the changes.
msg282630 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-12-07 15:58
Hmm.  That e.close() looks like there may be a deeper problem here.  I don't have time to investigate myself, unfortunately.
msg282631 - (view) Author: Jon Dufresne (jdufresne) * Date: 2016-12-07 16:25
Just for some context, the e.close() is handling this bit of code:

https://github.com/python/cpython/blob/d8132c4da7c46587221c5a244224b770d03860b6/Lib/urllib/request.py#L739-L754

When there is no error, http_error_302() will close the passed fp, on error, it will not. The following comment indicates this is intentional:

# Don't close the fp until we are sure that we won't use it
# with HTTPError.

But I agree, this may pose a problem when enforcing deterministic resource handling.
msg282933 - (view) Author: Jon Dufresne (jdufresne) * Date: 2016-12-11 16:53
I've taken a new approach to resolve the urllib issues.

I believe HTTPError _should not_ warn when __del__ is called as HTTPError wraps an existing resource instead of generating its own. IIUC, in this case, I believe it falls to the responsibility of code generating the resource to properly close and manage the resource (or pass that responsibility along). If I misunderstood, please let me know.

All feedback on the patch is welcome, thanks.
msg282955 - (view) Author: Jon Dufresne (jdufresne) * Date: 2016-12-12 00:46
I decided to try a new direction.

Instead of modifying _TemporaryFileCloser to handle urllib, I've changed urllib classes to not inherit from _TemporaryFileCloser. The urllib classes are not temporary files as built by tempfile, so I believe this makes more sense. There is a small amount of code copied to the existing class, but I believe this cleans up the inheritance in a way that makes more sense for the given objects and requires fewer workarounds.

All feedback on this alternative approach is welcome. Thanks.
History
Date User Action Args
2017-06-04 03:13:36jdufresnesetpull_requests: + pull_request2015
2016-12-12 00:46:07jdufresnesetfiles: + namedtemporaryfile-resourcewarning-4.patch

messages: + msg282955
2016-12-11 16:53:55jdufresnesetfiles: + namedtemporaryfile-resourcewarning-3.patch

messages: + msg282933
2016-12-08 22:44:37vstinnersetnosy: + vstinner
2016-12-07 16:25:45jdufresnesetmessages: + msg282631
2016-12-07 15:58:08r.david.murraysetmessages: + msg282630
2016-12-07 15:52:09jdufresnesetfiles: + namedtemporaryfile-resourcewarning-2.patch

messages: + msg282629
2016-12-07 07:49:49SilentGhostsetstage: needs patch
messages: + msg282599
versions: + Python 3.7
2016-12-07 05:29:42jdufresnesetfiles: + namedtemporaryfile-resourcewarning.patch
keywords: + patch
2016-12-04 22:29:53r.david.murraysetmessages: + msg282372
2016-12-04 21:12:17SilentGhostsetmessages: + msg282366
2016-12-04 20:45:28r.david.murraysetnosy: + r.david.murray
messages: + msg282360
2016-12-04 18:49:41SilentGhostsetnosy: + SilentGhost
messages: + msg282355
2016-12-04 17:47:27jdufresnecreate