Avoid subprocess memory copy when c library supports posix_spawn #87958
Conversation
By default python 3.10 will use the fork() which has to copy all the memory of the parent process (in our case this can be huge since Home Assistant core can use hundreds of megabytes of RAM). By using posix_spawn this is avoided. In python 3.11 vfork will also be available python/cpython#80004 (comment) python/cpython#11671 but we won't always be able to use it and posix_spawn is considered safer https://bugzilla.kernel.org/show_bug.cgi?id=215813#c14 The subprocess library doesn't know about musl though even though it supports posix_spawn https://git.musl-libc.org/cgit/musl/log/src/process/posix_spawn.c so we have to teach it since it only has checks for glibc https://github.com/python/cpython/blob/1b736838e6ae1b4ef42cdd27c2708face908f92c/Lib/subprocess.py#L745 The constant is documented as being able to be flipped here: https://docs.python.org/3/library/subprocess.html#disabling-use-of-vfork-or-posix-spawn
By default python 3.10 will use the fork() which has to copy memory of the parent process (in our case this can be huge since Home Assistant core can use hundreds of megabytes of RAM). By using posix_spawn this is avoided and subprocess creation does not get discernibly slow the larger the Home Assistant python process grows. In python 3.11 vfork will also be available python/cpython#80004 (comment) python/cpython#11671 but we won't always be able to use it and posix_spawn is considered safer https://bugzilla.kernel.org/show_bug.cgi?id=215813#c14 The subprocess library doesn't know about musl though even though it supports posix_spawn https://git.musl-libc.org/cgit/musl/log/src/process/posix_spawn.c so we have to teach it since it only has checks for glibc https://github.com/python/cpython/blob/1b736838e6ae1b4ef42cdd27c2708face908f92c/Lib/subprocess.py#L745 The constant is documented as being able to be flipped here: https://docs.python.org/3/library/subprocess.html#disabling-use-of-vfork-or-posix-spawn
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
homeassistant/runner.py
Outdated
| # on Alpine Linux which is supported by musl. | ||
| _exists = os.path.exists | ||
| subprocess._USE_POSIX_SPAWN = _exists( # pylint: disable=protected-access | ||
| ALPINE_RELEASE_FILE |
There was a problem hiding this comment.
I was going back and forth on this check because there could be other cases where they use musl but not alpine. I think that's rare enough that it's not worth the additional overhead of importing the packaging module or requiring it.
| def _enable_posix_spawn() -> None: | ||
| """Enable posix_spawn on Alpine Linux.""" | ||
| # pylint: disable=protected-access | ||
| if subprocess._USE_POSIX_SPAWN: |
There was a problem hiding this comment.
This will already be true for any modern glibc system
closing fds can take a long time and its unlikely python is going to implement the close_range() system call [https://lwn.net/Articles/789023/] any time soon. Since we do not need to close the fds here as it does not cause any issues for ffmpeg we can spawn ffmpeg stream much faster in HA after home-assistant/core#87958
closing fds can take a long time and its unlikely python is going to implement the close_range() system call [https://lwn.net/Articles/789023/] any time soon. Since we do not need to close the fds here as it does not cause any issues for ffmpeg we can spawn ffmpeg stream much faster in HA after home-assistant/core#87958
| *self.config[CONF_ARGS], | ||
| env=env, | ||
| stdout=asyncio.subprocess.PIPE if self.config[CONF_META] else None, | ||
| close_fds=False, # required for posix_spawn |
There was a problem hiding this comment.
The problem is that this will break any 3rd party lib using subprocess
There was a problem hiding this comment.
What am I missing? Are there specific fds open that will be a problem for the subprocess in this instance?
If close_fds is true, all file descriptors except 0, 1 and 2 will be closed before the child process is executed. Otherwise when close_fds is false, file descriptors obey their inheritable flag as described in Inheritance of File Descriptors.
A file descriptor has an “inheritable” flag which indicates if the file descriptor can be inherited by child processes. Since Python 3.4, file descriptors created by Python are non-inheritable by default.
On UNIX, non-inheritable file descriptors are closed in child processes at the execution of a new program, other file descriptors are inherited.
There was a problem hiding this comment.
Some more back story. python/cpython#86904
I don’t think we are crossing a security boundary here (but maybe I’m not seeing it). Most of these invocations are even using a shell directly so I’d expect anything they are executing to run at the same privilege level as the python process.
There was a problem hiding this comment.
Sorry, I meant in general. If we have to update all our calls to pass close_fds=False, then 3rd party libs calling a sub process would have to do the same.
There was a problem hiding this comment.
Anything that doesn't add the flag will get the existing slow behavior (effectively no change)
If they can add the flag and they don't have anything else special (like changing uids), the subprocess module should automatically use the faster path.
If we know of a library that frequently spawns subprocesses it would be good to adjust it. At least in the profiles users sent, it was almost always the two integrations mentioned above that were generating the load on the system. I expect this change will solve the issue for the 99% case
Proposed change
The
command_line(8.2% of the active installations) andshell_command(4.7% of the active installations) integrations are quite popular. They suffer from a performance issue due to the memory size of the Home Assistant python instance and python's subprocess implementation when using musl. We also experience the issue when pip is called to install a new module.By default python 3.10 will use
fork()which has to copy memory of the parent process. In our casethis can be huge since Home Assistant core can use hundreds of megabytes of RAM. By using
posix_spawnthis is avoided and subprocess creation does not get discernibly slower the larger the Home Assistant python process grows.Additional reading: https://github.com/rtomayko/posix-spawn
In python 3.11 vfork will also be available (vfork is stubbed out in 3.10)
python/cpython#80004 (comment) python/cpython#11671 but we won't always be able to use it and posix_spawn is likely safer https://bugzilla.kernel.org/show_bug.cgi?id=215813#c14
The subprocess library doesn't know about
muslthough even though it supportsposix_spawnhttps://git.musl-libc.org/cgit/musl/log/src/process/posix_spawn.c so we have to teach it since it only has checks forglibchttps://github.com/python/cpython/blob/1b736838e6ae1b4ef42cdd27c2708face908f92c/Lib/subprocess.py#L745The constant is documented as being able to be flipped off at https://docs.python.org/3/library/subprocess.html#disabling-use-of-vfork-or-posix-spawn so it seems likely to not change in the future.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: