Avoid calling isatty() for most open() calls

[collinanderson]

BPO	45944
Nosy	@pitrou, @benjaminp, @serhiy-storchaka, @eryksun, @collinanderson
PRs	bpo-45944: Avoid calling isatty() for most open() calls #29870

^{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 = None
created_at = <Date 2021-12-01.05:08:15.763>
labels = ['3.11', 'expert-IO', 'performance']
title = 'Avoid calling isatty() for most open() calls'
updated_at = <Date 2021-12-01.11:53:13.388>
user = 'https://github.com/collinanderson'

bugs.python.org fields:

activity = <Date 2021-12-01.11:53:13.388>
actor = 'eryksun'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['IO']
creation = <Date 2021-12-01.05:08:15.763>
creator = 'collinanderson'
dependencies = []
files = []
hgrepos = []
issue_num = 45944
keywords = ['patch']
message_count = 3.0
messages = ['407427', '407434', '407444']
nosy_count = 6.0
nosy_names = ['pitrou', 'benjamin.peterson', 'stutzbach', 'serhiy.storchaka', 'eryksun', 'collinanderson']
pr_nums = ['29870']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue45944'
versions = ['Python 3.11']

Linked PRs

• gh-90102: Optimize io.FileIO.isatty() #112495
• gh-90102: Remove isatty call during regular open #124922
• gh-90102: Fix pyio return value #125089

[collinanderson]

isatty() is a system call on linux. Most open()s are files, and we're already getting the size of the file. If it has a size, then we know it's not a atty, and can avoid calling it.

[serhiy-storchaka]

What if change FileIO.isatty() instead? If make it returning False without invoking a system call if the file size is non-zero it will eliminate the need to expose _size.

[eryksun]

make it returning False without invoking a system call if the file
size is non-zero it will eliminate the need to expose _size.

I suggest using the file type instead of the size. There's no reason to call isatty() if it's not an S_IFCHR file. This will avoid calling isatty() on regular files that happen to be empty.

In Windows, isatty(fd) is based solely on the file type, which is flagged in the fd record when a file descriptor is opened for a native file handle. It's not a system call, but it's also nearly worthless for how isatty() is typically used, since it's true for any S_IFCHR file (e.g. con, nul, com1).

[serhiy-storchaka]

#112495 is a simpler PR that implements the idea proposed by @eryksun. It also caches the isatty() result for subsequent calls.

[serhiy-storchaka]

One test fails with #112495 if the stdin is not a terminal. For example:

$ ./python -m test -v test_builtin -m test_input_no_stdout_fileno <python
...
test test_builtin failed -- Traceback (most recent call last):
  File "/home/serhiy/py/cpython/Lib/test/test_builtin.py", line 2393, in test_input_no_stdout_fileno
    self.assertSequenceEqual(lines, expected)
    ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
AssertionError: Sequences differ: ['stdin.isatty(): False', "captured: 'prompt'"] != ('stdin.isatty(): True', "captured: 'prompt'")

First differing element 0:
'stdin.isatty(): False'
'stdin.isatty(): True'

- ['stdin.isatty(): False', "captured: 'prompt'"]
? ^                 ^^^^                        ^

+ ('stdin.isatty(): True', "captured: 'prompt'")
? ^                 ^^^                        ^

This is because the result of isatty() is not constant during the lifetime of the file object. The file descriptor can be reassigned to other file or device, so it should not be cached.

#29870 uses the cached value only in open(), immediately after creating the FileIO object, so it is not affected by this. But it is more complex, and making it more portable will make it even more complex. How much time it saves?

[vstinner]

PR gh-112495 was closed. @serhiy-storchaka wrote:

It does not work, because the file descriptor can be reassigned from tty to non-tty and vice versa. The result of isatty() can not be cached.

Should we close the issue?

[cmaloney]

I think it's okay to rely on the fd not being modified inside of open() which is what #121593 does. It doesn't cache isatty generally (any future call still does a isatty libc call), just says "we can skip this call/check during open if we already have enough information from stat that the fd isn't a TTY".

[vstinner]

Optimization implemented by change cc9b9be.