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.

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.

The point of ResourceWarning it tell you when you are depending on destructors for cleanup, so this does look like a bug.

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.

__del__ is a destructor. If the file is open when it is called, it should emit a ResourceWarning.

The ResourceWarning that you've introduced need to be "fixed" in a few place in test_urllib2

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.

Hmm. That e.close() looks like there may be a deeper problem here. I don't have time to investigate myself, unfortunately.

Just for some context, the e.close() is handling this bit of code:

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.

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.

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.

Just to bump this because it's been inactive for 7 years ... similar to #103070, NamedTemporaryFile really should be emitting a proper ResourceWarning as this ticket notes, and it should point at user code to help folks track down the call sites. The changes in #1936 seemed to be generally correct, but were lost because the PR was focused on urllib instead of tempfile.