Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use and prefer os.posix_spawn() when available #54

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Use and prefer os.posix_spawn() when available #54

wants to merge 5 commits into from

Conversation

cagney
Copy link

@cagney cagney commented Apr 11, 2019

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.

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.
@takluyver
Copy link
Member

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:

pty_make_controlling_tty(child_fd)

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/forkpty.c#L43-L44

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:

https://github.com/lattera/glibc/blob/895ef79e04a953cac1493863bcae29ad85657ee1/sysdeps/posix/spawni.c#L280-L281

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.

(non-inheritable is code for O_CLOEXEC)
@cagney
Copy link
Author

cagney commented Apr 12, 2019

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 use fork() inside glibc:

BTW, on Solaris and NetBSD, at least, it's implemented as a system call.

https://github.com/lattera/glibc/blob/895ef79e04a953cac1493863bcae29ad85657ee1/sysdeps/posix/spawni.c#L280-L281

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.

@takluyver
Copy link
Member

Having done some more reading, I still think the missing 'controlling tty' part is important. It looks like this may be used for:

  • Sending signals from the terminal to processes running inside it
  • (Possibly) Job control, foreground & background processes
  • Some programs read directly from their controlling terminal to bypass stdin for things like password prompts.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants