Open cagney opened 5 years ago
Thanks, this is interesting. I have a couple of concerns, though. Most significantly, the functions to fork a process into a pty all have some extra code to make the new tty the controlling tty of the new process. Here's ptyprocess' fallback:
Here it is in Python's stdlib:
https://github.com/python/cpython/blob/9e4f2f3a6b8ee995c365e86d976937c141d867f8/Lib/pty.py#L110-L112
And here it is in glibc's forkpty function:
https://github.com/lattera/glibc/blob/895ef79e04a953cac1493863bcae29ad85657ee1/login/login_tty.c
I don't think the posix_spawn
implementation does anything equivalent to this, and I suspect it's important.
Secondly, the posix_spawn
call ultimately seems to use fork()
inside glibc:
Presumably the glibc implementation is pretty good, but I'd still have some concern that we're trading one set of problems for another. The possibility that the whole block needs wrapping in a Python-level threading lock doesn't inspire confidence - even if it ultimately turns out not to need that, the fact that it looks like it might isn't great.
Thanks, this is interesting. I have a couple of concerns, though. Most significantly, the functions to fork a process into a pty all have some extra code to make the new tty the controlling tty of the new process. Here's ptyprocess' fallback:
https://github.com/lattera/glibc/blob/895ef79e04a953cac1493863bcae29ad85657ee1/login/login_tty.c
I don't think the
posix_spawn
implementation does anything equivalent to this, and I suspect it's important.
Yea. Below are extracts from a BSD implementation of login_tty() (its the same as glibc, but simpler):
(void) setsid();
For screwing around with the process group, posix_spawn() seems to have two mechanisms: setpgroup (setpgrp(2)) setsid (setsid(2)) - I pass setsid=True However, looking more carefully I see that telling posix_spawn() to call setsid() is a GNU extension. I'll change things to deal with this.
if (ioctl(fd, TIOCSCTTY, NULL) == -1)
return (-1);
Make the terminal the controlling terminal for the process
(the process must not currently have a controlling terminal).
It seems to be missing. I'm not seeing problems, but then I'm not trying to run something like an interactive bash in the sub-process. I'll look into it.
(void) dup2(fd, STDIN_FILENO);
(void) dup2(fd, STDOUT_FILENO);
(void) dup2(fd, STDERR_FILENO);
if (fd != STDIN_FILENO && fd != STDOUT_FILENO && fd != STDERR_FILENO)
(void) close(fd);
this is handled by the file_actions=... argument.
Secondly, the
posix_spawn
call ultimately seems to usefork()
inside glibc:
BTW, on Solaris and NetBSD, at least, it's implemented as a system call.
Presumably the glibc implementation is pretty good, but I'd still have some concern that we're trading one set of problems for another. The possibility that the whole block needs wrapping in a Python-level threading lock doesn't inspire confidence - even if it ultimately turns out not to need that, the fact that it looks like it might isn't great.
The problem's with os.openpty() / openpty(3) which returns the PTY/TTY without O_CLOEXEC set. This means that fork()/exec() in a separate thread would have the FD left open :-(
However, this also means I can simplify things and just wrap the os.openpty() and setting non-inheritable.
Having done some more reading, I still think the missing 'controlling tty' part is important. It looks like this may be used for:
I also looked into the source code of libvte, the terminal emulator library used by Gnome terminal (among other applications). As best I can tell, it also uses fork()
followed by setup steps in the child process.
As attractive as it is, I think posix_spawn
is a dead end for this purpose. If you want to make the spawning process more robust, I think you'll need to write C code to do the fork+pty setup+exec safely, and either distribute it as a new extension module or contribute it to the standard library.
Python 3.8 has added os.posix_spawn(), this changes ptyprocess to use it when available.
Since os.posix_spawn() completey bypasses os.fork(), pty.fork(), it avoids problems with logging locks and code needing to be thread-safe.