classification
Title: Got ResourceWarning: unclosed file when using test function from formatter module
Type: resource usage Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, berker.peksag, eric.smith, mjpieters, python-dev, r.david.murray, serhiy.storchaka, vajrasky, vstinner
Priority: normal Keywords: patch

Created on 2013-08-03 13:45 by vajrasky, last changed 2015-01-05 10:16 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
formatter_fix_resource_warning.patch vajrasky, 2013-08-03 13:45 review
formatter_fix_resource_warning_v2.patch vajrasky, 2013-08-03 13:51 review
formatter_fix_resource_warning_v3.patch vajrasky, 2013-08-03 14:04 review
formatter_fix_resource_warning_v4.patch vajrasky, 2013-08-12 07:18 review
formatter_fix_resource_warning_v5.patch vajrasky, 2013-08-12 08:15 review
issue18644.diff berker.peksag, 2015-01-05 09:53 review
Messages (20)
msg194260 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-08-03 13:45
This python is compiled with --with-pydebug option.

[sky@localhost cpython]$ cat /tmp/a.txt
manly man likes cute cat.
[sky@localhost cpython]$ ./python
Python 3.4.0a0 (default:e408e821d6c8, Jul 27 2013, 10:49:54) 
[GCC 4.7.2 20121109 (Red Hat 4.7.2-8)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from formatter import test
>>> test('/tmp/a.txt')
manly man likes cute cat.
__main__:1: ResourceWarning: unclosed file <_io.TextIOWrapper name='/tmp/a.txt' mode='r' encoding='UTF-8'>
>>> 
[sky@localhost cpython]$ ./python Lib/formatter.py /tmp/a.txt
manly man likes cute cat.
Lib/formatter.py:445: ResourceWarning: unclosed file <_io.TextIOWrapper name='/tmp/a.txt' mode='r' encoding='UTF-8'>
  test()
msg194261 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-08-03 13:51
Sorry, I forgot about stdin. Attached the patch to handle stdin gracefully.
msg194262 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-08-03 14:04
I guess I should not close stdin just in case people are using test function in the script.

Attached the patch to only close the open files not stdin.
msg194265 - (view) Author: Martijn Pieters (mjpieters) * Date: 2013-08-03 14:44
Why is the `formatter` module still part of Python 3? This was a dependency for the `htmllib` module in Python 2 only, and that module was deprecated and removed from Python 3.
msg194771 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-08-09 20:35
"Why is the `formatter` module still part of Python 3? This was a dependency for the `htmllib` module in Python 2 only, and that module was deprecated and removed from Python 3."

The formatter module was deprecated? When?
msg194925 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-08-12 04:53
Please wrap the "businesss" part of the test function in a try... finally. Otherwise the patch looks good.
msg194929 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-08-12 07:18
Thanks, Benjamin, for reviewing my patch.

Attached the fourth patch to wrap the "business" part inside the try ... finally.
msg194930 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-08-12 07:32
That will fail if fp is not assigned before an exception is raised. I mostly mean the part starting with the for loop.
msg194931 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-08-12 08:15
Ah, sorry about that.

Attached the fifth patch to only wrap "for loop" section inside the try... finally.
msg194946 - (view) Author: Martijn Pieters (mjpieters) * Date: 2013-08-12 11:37
> The formatter module was deprecated? When?

It wasn't, that's the point I am raising. The `formatter` module was exclusively used by the `htmllib` module, I am surprised the `formatter` module wasn't part of that deprecation.
msg194947 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-08-12 11:42
Pydoc uses DumbWriter.
msg215278 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2014-03-31 22:37
Version 5 of the patch looks fine; Vajrasky, I think you can go ahead and commit it.

(I didn't understand RDM's comment about pydoc using DumbWriter; in 3.4, it doesn't seem to me that pydoc does.  What am I missing?)
msg215316 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-04-01 13:37
There were a bunch of changes to pydoc in 3.4, so I'm not surprised that it doesn't use DumbWriter any more (possibly as part of the formatter pending deprecation).  I think I was answering why it wasn't deprecated as part of the htmllib deprecation.
msg233442 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-01-05 07:18
New changeset f859a61f5853 by Berker Peksag in branch '3.4':
Issue #18644: Fix a ResourceWarning in formatter.test().
https://hg.python.org/cpython/rev/f859a61f5853

New changeset f374e4e6d04b by Berker Peksag in branch 'default':
Issue #18644: Fix a ResourceWarning in formatter.test().
https://hg.python.org/cpython/rev/f374e4e6d04b
msg233443 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-01-05 07:20
Thanks for the patch, Vajrasky.
msg233446 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2015-01-05 08:29
Not that I think it's worth changing for this case, but I find code like this better written as:

if some_test:
    fl = contextlib.closing(open(sys.argv[1]))
else:
    fl = sys.stdin
with fl as fl:
    do_stuff(fl)

This way you don't need another test, the close isn't far away from the open, and you save some lines of boilerplate.


Or, if "with fl as fl" bothers you:

with sys.stdout if some_test else contextlib.closing(open(sys.argv[1])) as fl:
    do_stuff(fl)

I don't recommend that, though.

In any event, thanks for the fix!
msg233451 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-01-05 09:53
Thanks for the suggestion, Eric. contextlib.closing(open(...)) looks unnecessary to me. Here is an alternative patch.
msg233452 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-05 09:57
> fl = contextlib.closing(open(sys.argv[1]))

I prefer the current code with the explicit close() and "is not sys.stdin", it's less magic :-)
msg233455 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2015-01-05 10:13
Good point on contextlib.closing not being needed. I usually use this pattern on things that aren't files!

On second thought, the with statement will close sys.stdin, so this isn't a valid pattern here. Sorry for the noise.
msg233457 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-01-05 10:16
I prefer the current code (i.e. formatter_fix_resource_warning_v5.patch).

In more complex case ExitStack can be used, but here it looks redundant.

    with contextlib.ExitStack() as stack:
        if some_test:
            fl = open(sys.argv[1])
            stack.enter_context(fl)
        else:
            fl = sys.stdin
        do_stuff(fl)

or

    if some_test:
        cm = fl = open(sys.argv[1])
    else:
        fl = sys.stdin
        cm = contextlib.ExitStack()
    with cm:
        do_stuff(fl)
History
Date User Action Args
2015-01-05 10:16:52serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg233457
2015-01-05 10:13:30eric.smithsetmessages: + msg233455
2015-01-05 09:57:35vstinnersetmessages: + msg233452
2015-01-05 09:53:57berker.peksagsetfiles: + issue18644.diff

messages: + msg233451
2015-01-05 08:29:14eric.smithsetnosy: + eric.smith
messages: + msg233446
2015-01-05 07:20:21berker.peksagsetstatus: open -> closed

components: + Library (Lib), - Tests
versions: + Python 3.5
nosy: + berker.peksag

messages: + msg233443
resolution: fixed
stage: resolved
2015-01-05 07:18:22python-devsetnosy: + python-dev
messages: + msg233442
2014-12-31 16:25:14akuchlingsetnosy: - akuchling
2014-04-01 13:37:46r.david.murraysetmessages: + msg215316
2014-03-31 22:37:56akuchlingsetnosy: + akuchling
messages: + msg215278
2013-08-12 11:42:31r.david.murraysetnosy: + r.david.murray
messages: + msg194947
2013-08-12 11:37:39mjpieterssetmessages: + msg194946
2013-08-12 08:15:54vajraskysetfiles: + formatter_fix_resource_warning_v5.patch

messages: + msg194931
2013-08-12 07:32:11benjamin.petersonsetmessages: + msg194930
2013-08-12 07:18:27vajraskysetfiles: + formatter_fix_resource_warning_v4.patch

messages: + msg194929
2013-08-12 04:53:22benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg194925
2013-08-09 20:35:53vstinnersetnosy: + vstinner
messages: + msg194771
2013-08-03 14:44:26mjpieterssetnosy: + mjpieters
messages: + msg194265
2013-08-03 14:04:02vajraskysetfiles: + formatter_fix_resource_warning_v3.patch

messages: + msg194262
2013-08-03 13:51:15vajraskysetfiles: + formatter_fix_resource_warning_v2.patch

messages: + msg194261
2013-08-03 13:45:19vajraskycreate