Fix shutil.get_terminal_size() to catch AttributeError

[Vgr255]

BPO	26801
Nosy	@vstinner, @larryhastings, @ned-deily, @vadmium, @serhiy-storchaka, @1st1, @Vgr255
Files	get_terminal_size.diff get_term_size_with_test.patch get_term_size_with_test2.patch get_terminal_size_valueerror.patch get_terminal_size_valueerror2.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 2016-11-08.22:22:45.538>
created_at = <Date 2016-04-18.23:43:57.093>
labels = ['type-bug', 'library']
title = 'Fix shutil.get_terminal_size() to catch AttributeError'
updated_at = <Date 2016-11-08.22:22:45.526>
user = 'https://github.com/Vgr255'

bugs.python.org fields:

activity = <Date 2016-11-08.22:22:45.526>
actor = 'yselivanov'
assignee = 'none'
closed = True
closed_date = <Date 2016-11-08.22:22:45.538>
closer = 'yselivanov'
components = ['Library (Lib)']
creation = <Date 2016-04-18.23:43:57.093>
creator = 'abarry'
dependencies = []
files = ['42511', '42522', '42523', '42525', '42529']
hgrepos = []
issue_num = 26801
keywords = ['patch']
message_count = 26.0
messages = ['263701', '263705', '263706', '263710', '263748', '263749', '263755', '263756', '263760', '263762', '263764', '263766', '263767', '263768', '263773', '263783', '263804', '264093', '264095', '264096', '264097', '264099', '264100', '278347', '280357', '280358']
nosy_count = 8.0
nosy_names = ['vstinner', 'larry', 'ned.deily', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'yselivanov', 'abarry']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26801'
versions = ['Python 3.5', 'Python 3.6']

[Vgr255]

shutil.get_terminal_size() will sometimes propagate AttributeError: module <os> has not attribute 'get_terminal_size' to the caller. The call checks for NameError, which makes no sense, so I guess it must be an oversight. Attached patch fixes it.

(diff was generated with git, I don't know if it works with Mercurial, but it's a trivial change anyway)

[vadmium]

The patch looks good to me. The function was originally written to be included in the “os” module, hence the NameError. The patch should probably be fine with Mercurial, but it looks like the Reitveld review system doesn’t like it :)

What platform do you get the AttributeError with? Perhaps the function is not well covered in the test suite.

[Vgr255]

I think Rietveld doesn't like me because I made it a .diff file, and not a .patch file, but who knows.

It's a bit of a shot in the dark though, because I can't reproduce an environment where os.get_terminal_size() doesn't exist. I'm on Windows and sometimes compile the latest trunk to play around and find bugs, so it could be in one of those times (even though I just tried and it didn't fail).

I think that if NameError was to be caught before, it means the function was to be "maybe there, maybe not", which could very well still be the case, so it makes sense to use the proper exception in that case. I don't see any significant drawback to catching AttributeError, anyway.

[serhiy-storchaka]

What platform do you get the AttributeError with? Perhaps the function is not well covered in the test suite.

I guess os.get_terminal_size() didn't exist on ancient OSes like DOS, OS/2, ancient UNIXes. On all supported platforms (including such exotic as old AIX, QNX or Minix) it should be defined.

Tests should fail if os.get_terminal_size() doesn't exist.

I think Rietveld doesn't like me because I made it a .diff file, and not a .patch file, but who knows.

I think this is because the patch doesn't contain a revision number.

On Windows os.get_terminal_size() can raise ValueError if sys.__stdout__.fileno() is not one of 0, 1 or 2. It is worth to catch it too. An AttributeError is also raised if sys.stdout has no the "fileno" attribute (StringIO or None).

[vstinner]

get_terminal_size.diff LGTM.

Would you like to try to write an unit test? Maybe using unittest.mock.patch?

[vstinner]

On all supported platforms (including such exotic as old AIX, QNX or Minix) it should be defined.

test_shutil always call shutil.get_terminal_size() and I didn't see any failure on the unit suite on our buildbots, even less common platforms like OpenBSD, AIX, OpenIndiana, etc.

So yes, os.get_terminal_size() looks to be available on all supported platforms.

[Vgr255]

On posix-based OSes, os.get_terminal_size might not exist ( https://hg.python.org/cpython/file/default/Modules/posixmodule.c#l12462 ), so this is needed.

New patch file with tests included.

[Vgr255]

Hmm, if sys.__stdout__ was deleted (or set to None), this would also raise an AttributeError when calling shutil.get_terminal_size, so I think that even if os.get_terminal_size is guaranteed to be always present (which it's not, IIUC), catching AttributeError would prevent that bug, too.

Should I write a unit test for that too? Even though this one theorically covers it as well, I wouldn't want people in the future to think they can safely remove except AttributeError from the code.

[vstinner]

Hmm, if sys.__stdout__ was deleted (or set to None), this would also raise an AttributeError when calling shutil.get_terminal_size, so I think that even if os.get_terminal_size is guaranteed to be always present (which it's not, IIUC), catching AttributeError would prevent that bug, too.

It would be nice to have an unit test too for this case. You can use something like:

with unittest.mock.patch('shutil.sys') as mock_sys:
    del mock_sys.__stdout__
    print(shutil.get_terminal_size((3, 3)))

(and mock also os.envion, as shown in the review.)

[serhiy-storchaka]

Honest, I don't think that we need such complex test for the case that isn't occurred in wild. If delete os.get_terminal_size, all TermsizeTests tests fail, so we will know if encounter a platform without os.get_terminal_size.

Instead I suggest to add ValueError in exceptions list and add tests for changed sys.__stdout__: None, StringIO(), open(TESTFN, "w"). Some of these tests fail without AttributeError in the exceptions list.

[vstinner]

Honest, I don't think that we need such complex test for the case that isn't occurred in wild.

Right. I'm also fine if you test manually this corner case.

The Lib/shutil.py change LGTM in get_term_size_with_test2.patch (I ignored the unit test).

[Vgr255]

To be fair, I don't think we actually need a unit test to check if os.get_terminal_size exists, as we catch any AttributeError at all. I'd want to keep the except clause there to properly handle sys.__stdout__ being None (or simply absent). I also don't consider that I'm fixing a bug here, but more like an oversight. The except clause with NameError is obviously an oversight from when the function was ported from os to shutil, so I'd rather fix it.

[python-dev]

New changeset e3763b5964b6 by Victor Stinner in branch '3.5':
Fix shutil.get_terminal_size() error handling
https://hg.python.org/cpython/rev/e3763b5964b6

New changeset 75f40345d784 by Victor Stinner in branch 'default':
Merge 3.5: issue bpo-26801
https://hg.python.org/cpython/rev/75f40345d784