A file is not properly closed by webbrowser._invoke

[swarmer]

BPO	15447
Nosy	@bitdancer, @swarmer
Files	fileclose.patch: Close the file explicitly fileclose_devnull.patch: Use DEVNULL constant instead of manually opening a file fileclose_devnull_v2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-09-03.16:55:23.255>
created_at = <Date 2012-07-25.13:15:13.088>
labels = ['type-bug', 'library']
title = 'A file is not properly closed by webbrowser._invoke'
updated_at = <Date 2012-09-03.16:55:23.254>
user = 'https://github.com/swarmer'

bugs.python.org fields:

activity = <Date 2012-09-03.16:55:23.254>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2012-09-03.16:55:23.255>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2012-07-25.13:15:13.088>
creator = 'anton.barkovsky'
dependencies = []
files = ['26511', '26513', '26602']
hgrepos = []
issue_num = 15447
keywords = ['patch']
message_count = 12.0
messages = ['166392', '166394', '166395', '166396', '166397', '166399', '166401', '166404', '166900', '167424', '169774', '169779']
nosy_count = 4.0
nosy_names = ['r.david.murray', 'rosslagerwall', 'python-dev', 'anton.barkovsky']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue15447'
versions = ['Python 3.4']

[swarmer]

webbrowser._invoke opens /dev/null, never closes it and a warning is
printed.

I'm attaching a patch.
The diff looks messy, but I'm just wrapping the code in a try-finally
block, the rest is just indented.

[bitdancer]

Thanks. Is this warning printed by the webbrowser unit tests? If not can you see a way to add one that does?

[swarmer]

The warning is printed by the file object when it closes itself in __del__:

ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='r+' encoding='UTF-8'>

There isn't much to test, or is there?

[swarmer]

To clarify, I discovered this when I was simply running webbrowser.open
in REPL.

[rosslagerwall]

Are there any webbrowser unit tests?

(this could probably use the new subprocess.DEVNULL constant in 3.3)

[bitdancer]

@anton: That's what I was guessing. If we had a unit test in test_webbrowser that did the same thing, we'd have seen the resource warning when running the tests and fixed it. However, it looks like there aren't *any* tests for webbrowser, not even in test_sundry (which just makes sure modules without tests are importable).

So adding a test that will trigger this resource warning requires setting up a test_webbrowser file first, even before we get to the problem of how to test something that wants to start up a web browser...(but that should be solvable with unittest.mock, I think).

[swarmer]

Adding a patch that uses subprocess.DEVNULL instead.

Writing tests for webbrowser should be a separate issue, right?

[bitdancer]

You could do it either way. Normally we prefer to have a test along with any fix; in this case adding a test involves adding the test module as well, but it is not different in principle. If you want to work on it and prefer to have it as a separate issue that's fine, we'll just make the test issue dependent on this one.

[swarmer]

An updated patch with the same issue fixed in Konqueror class.

[swarmer]

Added tests in bpo-15557.

[python-dev]

New changeset 901c790e4417 by R David Murray in branch 'default':
bpo-15447: Use subprocess.DEVNULL in webbrowser, instead of opening
http://hg.python.org/cpython/rev/901c790e4417

New changeset 5da3b2df38b3 by R David Murray in branch 'default':
bpo-15557,bpo-15447,bpo-15509: webbrowser test suite added.
http://hg.python.org/cpython/rev/5da3b2df38b3

[bitdancer]

Thanks, Anton.