search

pexpect / ptyprocess

Run a subprocess in a pseudo terminal
https://ptyprocess.readthedocs.io/en/latest/
Other
217 stars 71 forks source link

PtyProcess.spawn (and thus pexpect) slowdown in close() loop #50

Open kolyshkin opened 5 years ago

kolyshkin commented 5 years ago

The following code in ptyprocess

https://github.com/pexpect/ptyprocess/blob/3931cd45db50ee8533b8b0fef424b8d75f7ba1c2/ptyprocess/ptyprocess.py#L260-L269

is looping through all possible file descriptors in order to close those (note that closerange() implemented as a loop at least on Linux). In case the limit of open fds (aka ulimit -n, aka RLIMIT_NOFILE, aka SC_OPEN_MAX) is set too high (for example, with recent docker it is 1024*1024), this loop takes considerable time (as it results in about a million close() syscalls).

The solution (at least for Linux and Darwin) is to obtain the list of actually opened fds, and only close those. This is implemented in subprocess module in Python3, and there is a backport of it to Python2 called subprocess32.

This issue was originally reported to docker: https://github.com/docker/for-linux/issues/502

Other good reason for using subprocess (being multithread-safe) is described in https://github.com/pexpect/ptyprocess/issues/43

takluyver commented 5 years ago

Hmm, it's annoying that closerange isn't implemented more efficiently, but I guess there's not much I can do about that.

The difficulty with using subprocess is that at the heart of ptyprocess, we call forkpty() rather than fork(). That's a different call all the way down to libc. There is a fallback implementation using fork(), with comments indicating it's for Solaris, but I don't know how well-tested it is, and I'm not confident in my abilities to write properly bulletproof low-level system code like this.

ptyprocess is also (as a component pulled out of pexpect), old code which has, over the years, attempted to support many platforms. I'm reluctant to make major changes to it, because I don't know all the platforms and use cases where it's running in the wild. So maybe you're better off writing a new module under a different name to do the same thing without all the legacy concerns. I'm happy to link to it from the docs.

kolyshkin commented 5 years ago

@takluyver well, perhaps you can do something like this instead: https://github.com/python/cpython/pull/11584/commits/5626fff54ebe5863e64454c081ec585f85cf141c

The good news is:

  1. it's already written, just need to be adopted to your codebase;
  2. it's a small change, relatively easy to review;
  3. this is Linux-only and won't break any other platform;
  4. it falls back to the existing implementation in case it's not working;
  5. it will speed up your software considerably under Docker on Linux.