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

[kolyshkin]

The following code in ptyprocess

ptyprocess/ptyprocess/ptyprocess.py

Lines 260 to 269 in 3931cd4

	# Do not allow child to inherit open file descriptors from parent,
	# with the exception of the exec_err_pipe_write of the pipe
	# and pass_fds.
	# Impose ceiling on max_fd: AIX bugfix for users with unlimited
	# nofiles where resource.RLIMIT_NOFILE is 2^63-1 and os.closerange()
	# occasionally raises out of range error
	max_fd = min(1048576, resource.getrlimit(resource.RLIMIT_NOFILE)[0])
	spass_fds = sorted(set(pass_fds) | {exec_err_pipe_write})
	for pair in zip([2] + spass_fds, spass_fds + [max_fd]):
	os.closerange(pair[0]+1, pair[1])

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: docker/for-linux#502

Other good reason for using subprocess (being multithread-safe) is described in #43

[takluyver]

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]

@takluyver well, perhaps you can do something like this instead: python/cpython@5626fff

The good news is:

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