classification
Title: Fix shutil.get_terminal_size() to catch AttributeError
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: abarry, larry, martin.panter, ned.deily, python-dev, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2016-04-18 23:43 by abarry, last changed 2016-11-08 22:22 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
get_terminal_size.diff abarry, 2016-04-18 23:43
get_term_size_with_test.patch abarry, 2016-04-19 17:35
get_term_size_with_test2.patch abarry, 2016-04-19 18:24 review
get_terminal_size_valueerror.patch serhiy.storchaka, 2016-04-19 21:15 review
get_terminal_size_valueerror2.patch serhiy.storchaka, 2016-04-20 07:17 review
Messages (26)
msg263701 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-04-18 23:43
`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)
msg263705 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-19 02:53
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.
msg263706 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-04-19 03:00
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.
msg263710 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-19 05:45
> 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).
msg263748 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-19 15:09
get_terminal_size.diff LGTM.

Would you like to try to write an unit test? Maybe using unittest.mock.patch?
msg263749 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-19 15:11
> 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.
msg263755 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-04-19 17:35
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.
msg263756 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-04-19 18:07
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.
msg263760 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-19 19:39
> 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.)
msg263762 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-19 20:12
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.
msg263764 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-19 20:16
> 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).
msg263766 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-04-19 20:20
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.
msg263767 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-19 20:29
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 #26801
https://hg.python.org/cpython/rev/75f40345d784
msg263768 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-19 20:30
Well, since Serhiy, Emmanuel and me agree that unit tests are overkill, I pushed the obivous and trivial fix. Thank you Emmanual for your contribution! I added your name to Misc/ACKS ;-)
msg263773 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-19 21:15
Here is a patch that adds ValueError in the exceptions list and adds tests.
msg263783 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-19 22:33
I doubt it is worth spending much effort supporting sys.__stdout__ being overwritten with StringIO or deleted. That seems an abuse of the “sys” module. Idle doesn’t even seem to alter this attribute.

But if you call stdout.close() or detach(), I think that is a more valid way to trigger ValueError, so Serhiy’s patch is worthwhile for that case.
msg263804 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-20 07:17
Left testing only the most common cases: sys.__stdout__ is None or is non a terminal.
msg264093 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-24 04:40
Serhiy’s patch looks worthwhile to me, though I still think a comment would help. There are lots of different cases being handled by those few lines:

try:
   size = os.get_terminal_size(sys.__stdout__.fileno())
except (AttributeError, ValueError, OSError)
msg264095 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-24 05:09
Could you suggest concrete wording?
msg264096 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-24 06:22
How about:

try:
    size = os.get_terminal_size(sys.__stdout__.fileno())
except (AttributeError, ValueError, OSError):
    # stdout is None, closed, detached, or not a terminal, or
    # os.get_terminal_size() is unsupported
    size = os.terminal_size(fallback)
msg264097 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-24 06:34
Martin's comment is helpful and LGTM.
msg264099 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-24 06:59
New changeset df8652452d25 by Serhiy Storchaka in branch '3.5':
Issue #26801: shutil.get_terminal_size() now handles the case of stdout is
https://hg.python.org/cpython/rev/df8652452d25

New changeset d6e6dcef674f by Serhiy Storchaka in branch 'default':
Issue #26801: shutil.get_terminal_size() now handles the case of stdout is
https://hg.python.org/cpython/rev/d6e6dcef674f
msg264100 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-24 07:00
Thank you Martin, your comment is helpful.
msg278347 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-10-09 05:51
New changeset 678424183b38 by INADA Naoki in branch '3.6':
Issue #26801: Added C implementation of asyncio.Future.
https://hg.python.org/cpython/rev/678424183b38

New changeset f8815001a390 by INADA Naoki in branch 'default':
Issue #26801: Added C implementation of asyncio.Future.
https://hg.python.org/cpython/rev/f8815001a390
msg280357 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-11-08 22:20
This patch introduced multiple refleaks in test_asyncgen.
msg280358 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-11-08 22:22
Ah, never mind, the commit message has a wrong issue number:

    Issue #26801: Added C implementation of asyncio.Future.

Closing this one, will re-open #26081.
History
Date User Action Args
2017-10-26 08:18:54serhiy.storchakalinkissue24920 superseder
2016-11-08 22:22:45yselivanovsetpriority: release blocker -> normal
status: open -> closed
resolution: fixed
messages: + msg280358
2016-11-08 22:20:59yselivanovsetpriority: normal -> release blocker
nosy: + larry, ned.deily
2016-11-08 22:20:50yselivanovsetstatus: closed -> open

nosy: + yselivanov
messages: + msg280357

resolution: fixed -> (no value)
2016-10-09 05:51:49python-devsetmessages: + msg278347
2016-04-24 07:00:54serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg264100

stage: patch review -> resolved
2016-04-24 06:59:42python-devsetmessages: + msg264099
2016-04-24 06:34:07vstinnersetmessages: + msg264097
2016-04-24 06:22:13martin.pantersetmessages: + msg264096
2016-04-24 05:09:19serhiy.storchakasetmessages: + msg264095
2016-04-24 04:40:42martin.pantersetmessages: + msg264093
2016-04-20 07:17:06serhiy.storchakasetfiles: + get_terminal_size_valueerror2.patch

messages: + msg263804
2016-04-19 22:33:07martin.pantersetmessages: + msg263783
2016-04-19 21:15:55serhiy.storchakasetstatus: closed -> open
files: + get_terminal_size_valueerror.patch
resolution: fixed -> (no value)
messages: + msg263773
2016-04-19 20:31:31vstinnersetstatus: open -> closed
resolution: fixed
2016-04-19 20:30:23vstinnersetmessages: + msg263768
2016-04-19 20:29:30python-devsetnosy: + python-dev
messages: + msg263767
2016-04-19 20:20:17abarrysetmessages: + msg263766
2016-04-19 20:16:23vstinnersetmessages: + msg263764
2016-04-19 20:12:24serhiy.storchakasetmessages: + msg263762
2016-04-19 19:39:08vstinnersetmessages: + msg263760
2016-04-19 18:24:28abarrysetfiles: + get_term_size_with_test2.patch
2016-04-19 18:07:28abarrysetmessages: + msg263756
2016-04-19 17:35:33abarrysetfiles: + get_term_size_with_test.patch

messages: + msg263755
2016-04-19 15:11:03vstinnersetmessages: + msg263749
2016-04-19 15:09:27vstinnersetnosy: + vstinner
messages: + msg263748
2016-04-19 05:45:45serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg263710
2016-04-19 03:00:57abarrysetmessages: + msg263706
2016-04-19 02:53:43martin.pantersetnosy: + martin.panter

messages: + msg263705
versions: + Python 3.5, Python 3.6
2016-04-18 23:43:57abarrycreate