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

PtyProcess.spawn (and thus pexpect) is not safe for use in multithreaded applications #43

Open
gpshead opened this issue Aug 28, 2017 · 6 comments

Comments

@gpshead
Copy link

gpshead commented Aug 28, 2017

Just the same as the old pure Python subprocess module from Python 2, the PtyProcess.spawn method is not async signal safe. The entire fork() to exec code path must be written in async signal safe C code. Both pty.fork and your own PtyProcess.spawn method violate this constraint.

See the Python 3 subprocess module's _posixsubprocess extension module (and the https://pypi.python.org/pypi/subprocess32/ backport for Python 2 users).

Likely symptoms when this bites you: random seeming deadlocks and hangs.

Workaround for users today: Do not use pexpect.spawn or pexpect.pty_spawn in a multithreaded application.
A pexpect.popen_spawn module appears to exist and will use the subprocess module. If you are on 2.7 make sure you've replaced sys.modules['subprocess'] with a reference to the subprocess32 module first (if you haven't already done so within your Python 2.7 interpreter's stdlib)

@gpshead
Copy link
Author

gpshead commented Aug 28, 2017

as the maintainer of Python's subprocess module, if there are API enhancements you want from subprocess to be able to make popen_spawn better and be your default implementation instead of ptyprocess.pty_spawn, please let me know what they are.
https://bugs.python.org/issue31296 to track that.

@kolyshkin
Copy link

Another issue is, ptyprocess is using its own implementation of closing fds before doing exec(), which suffers from a slowdown in case ulimit -n is set way too high. Some more details here: docker/for-linux#502. In particular, this means 1 million close() syscalls when run under a recent version of Docker.

The solution would be to use subprocess (or subprocess32 for Python 2) -- it looks at /proc/self/fd to close fds that are actually opened, rather than looping through all possible ones.

@takluyver
Copy link
Member

@gpshead belatedly, thanks for the offer. I'm not familiar enough with these kinds of restrictions to write all the pty setup code in low-level C. And even if I could, I'd be wary of such a big rewrite of old code that's in widespread use which I don't know about. I've been bitten before by trying to refactor pexpect and unwittingly breaking people's use cases.

Maybe one option would be for subprocess to grow a way of starting a new pty - either a parameter similar to start_new_session, or a separate class similar to Popen. This would largely remove the need for ptyprocess to exist, except as a legacy thing. That would be a pretty big extra maintenance burden, though, and I can't offer much help if it has to be written in C.

@takluyver
Copy link
Member

(I realise that, since Ptyprocess is essentially meant to be a mirror of Popen for working with subprocesses in a pty, I basically just suggested you take over maintenance of it and make it part of the stdlib. This isn't really a reasonable request, but unfortunately I don't see any small piece of functionality you could provide to enable ptyprocess to live on top. Although maybe it's less effort than I imagine given all the code already in subprocess. In any case, it's not a demand :-)

@hawkinsp
Copy link

hawkinsp commented May 31, 2024

We found that the ! shell escape in jupyter uses os.fork() via ptyprocess, which is problematic if any package in a jupyter process is multithreaded.

I'm wondering if there's a path forward here.

Would you be willing to accept a PR that changes this package to use a C extension to implement PtyProcess.spawn, rather than the problematic pure-Python implementation? I might be willing to write such a PR if you'd accept it.

@gpshead
Copy link
Author

gpshead commented May 31, 2024

I would've guessed that jupyter would use subprocess to implement !... There was some recent discussion among a couple of cpython core devs about maybe subprocess's _posixsubprocess implementation needing to gain some pty-like abilities. (unfortunately yet more things piled into its API soup, but that's the natural consequence of it being the centralized place for safely launching child processes)

But from this pexpect/ptyprocess projects perspective and from users perspective, waiting on CPython to have a feature would be a long wait. Your own extension could make sense.

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

No branches or pull requests

4 participants