classification
Title: Opening a file holds the GIL when it calls "isatty()"
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: lukasz.langa, miss-islington, smurfix, vstinner, vxgmichel
Priority: normal Keywords: patch

Created on 2021-05-23 17:18 by smurfix, last changed 2021-09-10 16:22 by lukasz.langa. This issue is now closed.

Files
File name Uploaded Description Edit
bpo-44219.patch vxgmichel, 2021-09-08 16:33
Pull Requests
URL Status Linked Edit
PR 28250 merged vxgmichel, 2021-09-09 08:41
PR 28255 merged miss-islington, 2021-09-09 13:12
PR 28256 merged miss-islington, 2021-09-09 13:12
PR 28261 merged lukasz.langa, 2021-09-09 16:44
PR 28274 merged miss-islington, 2021-09-10 15:51
PR 28275 merged miss-islington, 2021-09-10 15:51
Messages (12)
msg394208 - (view) Author: Matthias Urlichs (smurfix) * Date: 2021-05-23 17:18
Opening a file calls `isatty` which calls an ioctl with the GIL held.

GDB:
```
#0  __GI___tcgetattr (fd=18, termios_p=termios_p@entry=0x7f618a5df920)
    at ../sysdeps/unix/sysv/linux/tcgetattr.c:38
#1  0x00007f618bd1ca0c in __isatty (fd=<optimized out>) at ../sysdeps/posix/isatty.c:27
#2  0x000000000062b746 in _Py_device_encoding (fd=<optimized out>) at ../Python/fileutils.c:62
#3  0x000000000060bf90 in _io_TextIOWrapper___init___impl (write_through=0, line_buffering=0, 
    newline=0x0, errors='strict', encoding=<optimized out>, 
    buffer=<_io.BufferedWriter at remote 0x7f618986aeb0>, self=0x7f618985a860)
    at ../Modules/_io/textio.c:1149
...
```

Please don't do that.

In my case, the file in question is implemented as a FUSE mount which is served by the same process (different thread of course). Thus holding the GIL at this point causes a rather interesting deadlock.

Tested with 3.9.
msg401261 - (view) Author: Vincent Michel (vxgmichel) * Date: 2021-09-07 13:19
My team ran into this issue while developing a fuse application too.

In an effort to help this issue move forward, I tried to list all occurrences of the `isatty` C function in the cpython code base. I found 14 of them.

9 of them are directly related to stdin/stdout/stderr, so it's probably not crucial to release the GIL for those occurrences:
- `main.c:stdin_is_interactive`
- `main.c:pymain_import_readline`
- `readline.c:setup_readline`
- `bltinmodule.c:builtin_input_impl` (x2)
- `frozenmain.c:Py_FrozenMain`
- `pylifecycle.c:Py_FdIsInteractive` (x2)
- `fileobject.c:stdprinter_isatty` (GIL is actually released for this one)

Out of the remaining 4, only 1 releases the GIL:
- `fileio.c:_io_FileIO_isatty_impl`: used for `FileIO.isatty`

Which gives 3 occurrences of non-stdstream specific usage of `isatty` that do not release the GIL:
- `posixmodule.c:os_isatty_impl`: used by `os.isatty`
- `fileutils.c:_Py_device_encoding`: used `TextIOWrapper.__init__`
- `fileutils.c:_Py_write_impl`: windows specific, issue #11395

The first one is used by `os.isatty` which means this call can also deadlock. I did manage to reproduce it with a simple fuse loopback file system: https://github.com/fusepy/fusepy/blob/master/examples/loopback.py

The second one is the one found by @smurfix and gets triggered when `io.open()` is used in text mode.

The third one only triggers on windows when writing more than 32767 bytes to a file descriptor. A comment points to issue #11395 (https://bugs.python.org/issue11395). Also, it seems from the function signature that this function might be called with or without the GIL held, which might cause the fix to be a bit more complicated than the first two use cases.

I hope this helps.
msg401402 - (view) Author: Vincent Michel (vxgmichel) * Date: 2021-09-08 16:33
Here's a possible patch that fixes the 3 unprotected calls to `isatty` mentioned above. It successfully passes the test suite. I can submit a PR with this patch if necessary.
msg401450 - (view) Author: Matthias Urlichs (smurfix) * Date: 2021-09-09 09:18
Please do. However, I do think that changing the stdstream related ioctl calls also is a good idea, if only for code regularity/completeness sake. 

(Besides, nothing prevents somebody from starting a FUSE file system and then redirecting stdout to it …)
msg401458 - (view) Author: Vincent Michel (vxgmichel) * Date: 2021-09-09 10:05
There are a couple of reasons why I did not make changes to the stdstream related functions. 

The first one is that a PR with many changes is less likely to get reviewed and merged than a PR with fewer changes. The second one is that it's hard for me to make sure that those functions are always called with the GIL already held. Maybe some of them never hold the GIL in the first place, and I'm not familiar enough with the code base to tell.

So in the end it will probably be up to the coredev reviewing the PR, but better start small.

> Besides, nothing prevents somebody from starting a FUSE file system and then redirecting stdout to it …

I ran some checks and `python -c 'input()' < my-fuse-mountpoint/data_in` does indeed trigger an ioctl call to the corresponding fuse file system. But how would `my-fuse-mountpoint/data_in` be the stdin for the process that itself starts the fuse filesystem? I don't see how to generate this deadlock, apart from setting up interprocess communication between the fuse process and the `my-fuse-mountpoint/data_in`-as-stdin process.
msg401470 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-09-09 13:12
New changeset 06148b1870fceb1a21738761b8e1ac3bf654319b by Vincent Michel in branch 'main':
bpo-44219: Release the GIL during isatty syscalls (GH-28250)
https://github.com/python/cpython/commit/06148b1870fceb1a21738761b8e1ac3bf654319b
msg401478 - (view) Author: miss-islington (miss-islington) Date: 2021-09-09 13:40
New changeset 5c65834d801d6b4313eef0684a30e12c22ccfedd by Miss Islington (bot) in branch '3.9':
bpo-44219: Release the GIL during isatty syscalls (GH-28250)
https://github.com/python/cpython/commit/5c65834d801d6b4313eef0684a30e12c22ccfedd
msg401495 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-09-09 16:35
New changeset 23c46778d65870784cb6d4de30f43aac62d71e73 by Miss Islington (bot) in branch '3.10':
bpo-44219: Release the GIL during isatty syscalls (GH-28250) (GH-28255)
https://github.com/python/cpython/commit/23c46778d65870784cb6d4de30f43aac62d71e73
msg401497 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-09-09 16:42
> In my case, the file in question is implemented as a FUSE mount which is served by the same process (different thread of course). Thus holding the GIL at this point causes a rather interesting deadlock.

Since the change fixes a deadlock, I agree to backport it to 3.9 and 3.10.

Thanks for the fix!
msg401584 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-09-10 15:51
New changeset 296b7100705ef52aece3378b0ae42c33a58526e1 by Łukasz Langa in branch 'main':
bpo-44219: Mention GH-28250 is a deadlock bugfix (GH-28261)
https://github.com/python/cpython/commit/296b7100705ef52aece3378b0ae42c33a58526e1
msg401588 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-09-10 16:11
New changeset 60ddf499e14cc7daba3804e5a3460e4224dacc5c by Miss Islington (bot) in branch '3.10':
bpo-44219: Mention GH-28250 is a deadlock bugfix (GH-28261) (GH-28274)
https://github.com/python/cpython/commit/60ddf499e14cc7daba3804e5a3460e4224dacc5c
msg401591 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-09-10 16:22
New changeset 314de5326f8b687a3fa7b19ea8faaa449f59b8fe by Miss Islington (bot) in branch '3.9':
bpo-44219: Mention GH-28250 is a deadlock bugfix (GH-28261) (GH-28275)
https://github.com/python/cpython/commit/314de5326f8b687a3fa7b19ea8faaa449f59b8fe
History
Date User Action Args
2021-09-10 16:22:31lukasz.langasetmessages: + msg401591
2021-09-10 16:11:48lukasz.langasetmessages: + msg401588
2021-09-10 15:51:55miss-islingtonsetpull_requests: + pull_request26693
2021-09-10 15:51:53lukasz.langasetmessages: + msg401584
2021-09-10 15:51:50miss-islingtonsetpull_requests: + pull_request26692
2021-09-09 16:44:27lukasz.langasetpull_requests: + pull_request26681
2021-09-09 16:42:10vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg401497

stage: patch review -> resolved
2021-09-09 16:35:53lukasz.langasetnosy: + lukasz.langa
messages: + msg401495
2021-09-09 13:40:50miss-islingtonsetmessages: + msg401478
2021-09-09 13:12:17miss-islingtonsetpull_requests: + pull_request26675
2021-09-09 13:12:13miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request26674
2021-09-09 13:12:08vstinnersetnosy: + vstinner
messages: + msg401470
2021-09-09 10:05:25vxgmichelsetmessages: + msg401458
2021-09-09 09:18:04smurfixsetmessages: + msg401450
2021-09-09 08:41:47vxgmichelsetstage: patch review
pull_requests: + pull_request26671
2021-09-08 16:33:15vxgmichelsetfiles: + bpo-44219.patch
keywords: + patch
messages: + msg401402
2021-09-07 13:19:02vxgmichelsetnosy: + vxgmichel
messages: + msg401261
2021-05-23 17:18:13smurfixcreate