From 03eea7bd3fa27c8be4de856a29e1594d34137d56 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 13 Feb 2023 08:02:51 -0600 Subject: [PATCH] Avoid subprocess memory copy when c library supports posix_spawn (#87958) * use posix spawn on alpine * Avoid subprocess memory copy when c library supports posix_spawn 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 https://github.com/python/cpython/issues/80004#issuecomment-1093810689 https://github.com/python/cpython/pull/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 * Avoid subprocess memory copy when c library supports 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 https://github.com/python/cpython/issues/80004#issuecomment-1093810689 https://github.com/python/cpython/pull/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 * missed some * adjust more tests * coverage --- homeassistant/auth/providers/command_line.py | 1 + .../components/command_line/__init__.py | 10 ++++++++-- .../components/command_line/notify.py | 1 + .../components/ping/binary_sensor.py | 1 + .../components/ping/device_tracker.py | 5 ++++- homeassistant/components/rpi_camera/camera.py | 10 ++++++++-- .../seven_segments/image_processing.py | 5 ++++- .../components/shell_command/__init__.py | 2 ++ homeassistant/runner.py | 16 ++++++++++++++++ homeassistant/util/package.py | 10 +++++++++- tests/components/command_line/test_cover.py | 5 ++++- tests/components/command_line/test_sensor.py | 1 + tests/test_runner.py | 19 +++++++++++++++++++ tests/util/test_package.py | 7 ++++++- 14 files changed, 84 insertions(+), 9 deletions(-) diff --git a/homeassistant/auth/providers/command_line.py b/homeassistant/auth/providers/command_line.py index 92d8d617481..f63d6d465f6 100644 --- a/homeassistant/auth/providers/command_line.py +++ b/homeassistant/auth/providers/command_line.py @@ -68,6 +68,7 @@ class CommandLineAuthProvider(AuthProvider): *self.config[CONF_ARGS], env=env, stdout=asyncio.subprocess.PIPE if self.config[CONF_META] else None, + close_fds=False, # required for posix_spawn ) stdout, _ = await process.communicate() except OSError as err: diff --git a/homeassistant/components/command_line/__init__.py b/homeassistant/components/command_line/__init__.py index 172c321c0ec..c0713d0780b 100644 --- a/homeassistant/components/command_line/__init__.py +++ b/homeassistant/components/command_line/__init__.py @@ -18,7 +18,10 @@ def call_shell_with_timeout( try: _LOGGER.debug("Running command: %s", command) subprocess.check_output( - command, shell=True, timeout=timeout # nosec # shell by design + command, + shell=True, # nosec # shell by design + timeout=timeout, + close_fds=False, # required for posix_spawn ) return 0 except subprocess.CalledProcessError as proc_exception: @@ -41,7 +44,10 @@ def check_output_or_log(command: str, timeout: int) -> str | None: """Run a shell command with a timeout and return the output.""" try: return_value = subprocess.check_output( - command, shell=True, timeout=timeout # nosec # shell by design + command, + shell=True, # nosec # shell by design + timeout=timeout, + close_fds=False, # required for posix_spawn ) return return_value.strip().decode("utf-8") except subprocess.CalledProcessError as err: diff --git a/homeassistant/components/command_line/notify.py b/homeassistant/components/command_line/notify.py index 7bce5010d45..c41e26c21bb 100644 --- a/homeassistant/components/command_line/notify.py +++ b/homeassistant/components/command_line/notify.py @@ -52,6 +52,7 @@ class CommandLineNotificationService(BaseNotificationService): self.command, universal_newlines=True, stdin=subprocess.PIPE, + close_fds=False, # required for posix_spawn shell=True, # nosec # shell by design ) as proc: try: diff --git a/homeassistant/components/ping/binary_sensor.py b/homeassistant/components/ping/binary_sensor.py index ff662b55a93..7500d9988af 100644 --- a/homeassistant/components/ping/binary_sensor.py +++ b/homeassistant/components/ping/binary_sensor.py @@ -227,6 +227,7 @@ class PingDataSubProcess(PingData): stdin=None, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + close_fds=False, # required for posix_spawn ) try: out_data, out_error = await asyncio.wait_for( diff --git a/homeassistant/components/ping/device_tracker.py b/homeassistant/components/ping/device_tracker.py index c3729f04c14..68111df89ea 100644 --- a/homeassistant/components/ping/device_tracker.py +++ b/homeassistant/components/ping/device_tracker.py @@ -55,7 +55,10 @@ class HostSubProcess: def ping(self): """Send an ICMP echo request and return True if success.""" with subprocess.Popen( - self._ping_cmd, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL + self._ping_cmd, + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, + close_fds=False, # required for posix_spawn ) as pinger: try: pinger.communicate(timeout=1 + PING_TIMEOUT) diff --git a/homeassistant/components/rpi_camera/camera.py b/homeassistant/components/rpi_camera/camera.py index 59ce8fee5f9..3f9b5fd5860 100644 --- a/homeassistant/components/rpi_camera/camera.py +++ b/homeassistant/components/rpi_camera/camera.py @@ -32,7 +32,10 @@ _LOGGER = logging.getLogger(__name__) def kill_raspistill(*args): """Kill any previously running raspistill process..""" with subprocess.Popen( - ["killall", "raspistill"], stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT + ["killall", "raspistill"], + stdout=subprocess.DEVNULL, + stderr=subprocess.STDOUT, + close_fds=False, # required for posix_spawn ): pass @@ -132,7 +135,10 @@ class RaspberryCamera(Camera): # Therefore it must not be wrapped with "with", since that # waits for the subprocess to exit before continuing. subprocess.Popen( # pylint: disable=consider-using-with - cmd_args, stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT + cmd_args, + stdout=subprocess.DEVNULL, + stderr=subprocess.STDOUT, + close_fds=False, # required for posix_spawn ) def camera_image( diff --git a/homeassistant/components/seven_segments/image_processing.py b/homeassistant/components/seven_segments/image_processing.py index b6accf30de8..58d532f58f8 100644 --- a/homeassistant/components/seven_segments/image_processing.py +++ b/homeassistant/components/seven_segments/image_processing.py @@ -130,7 +130,10 @@ class ImageProcessingSsocr(ImageProcessingEntity): img.save(self.filepath, "png") with subprocess.Popen( - self._command, stdout=subprocess.PIPE, stderr=subprocess.PIPE + self._command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + close_fds=False, # Required for posix_spawn ) as ocr: out = ocr.communicate() if out[0] != b"": diff --git a/homeassistant/components/shell_command/__init__.py b/homeassistant/components/shell_command/__init__.py index d4a0a3ac1d5..cade4eaff68 100644 --- a/homeassistant/components/shell_command/__init__.py +++ b/homeassistant/components/shell_command/__init__.py @@ -65,6 +65,7 @@ async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool: stdin=None, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + close_fds=False, # required for posix_spawn ) else: # Template used. Break into list and use create_subprocess_exec @@ -76,6 +77,7 @@ async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool: stdin=None, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + close_fds=False, # required for posix_spawn ) process = await create_process diff --git a/homeassistant/runner.py b/homeassistant/runner.py index d3c2633bc7b..8c5766cbb2b 100644 --- a/homeassistant/runner.py +++ b/homeassistant/runner.py @@ -6,6 +6,7 @@ from asyncio import events import dataclasses import logging import os +import subprocess import threading import traceback from typing import Any @@ -28,6 +29,7 @@ from .util.thread import deadlock_safe_shutdown # MAX_EXECUTOR_WORKERS = 64 TASK_CANCELATION_TIMEOUT = 5 +ALPINE_RELEASE_FILE = "/etc/alpine-release" _LOGGER = logging.getLogger(__name__) @@ -153,8 +155,22 @@ async def setup_and_run_hass(runtime_config: RuntimeConfig) -> int: return await hass.async_run() +def _enable_posix_spawn() -> None: + """Enable posix_spawn on Alpine Linux.""" + # pylint: disable=protected-access + if subprocess._USE_POSIX_SPAWN: + return + + # The subprocess module does not know about Alpine Linux/musl + # and will use fork() instead of posix_spawn() which significantly + # less efficient. This is a workaround to force posix_spawn() + # on Alpine Linux which is supported by musl. + subprocess._USE_POSIX_SPAWN = os.path.exists(ALPINE_RELEASE_FILE) + + def run(runtime_config: RuntimeConfig) -> int: """Run Home Assistant.""" + _enable_posix_spawn() asyncio.set_event_loop_policy(HassEventLoopPolicy(runtime_config.debug)) # Backport of cpython 3.9 asyncio.run with a _cancel_all_tasks that times out loop = asyncio.new_event_loop() diff --git a/homeassistant/util/package.py b/homeassistant/util/package.py index b67e9923b9c..c2c84bf855d 100644 --- a/homeassistant/util/package.py +++ b/homeassistant/util/package.py @@ -94,7 +94,14 @@ def install_package( args += ["--user"] env["PYTHONUSERBASE"] = os.path.abspath(target) _LOGGER.debug("Running pip command: args=%s", args) - with Popen(args, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=env) as process: + with Popen( + args, + stdin=PIPE, + stdout=PIPE, + stderr=PIPE, + env=env, + close_fds=False, # required for posix_spawn + ) as process: _, stderr = process.communicate() if process.returncode != 0: _LOGGER.error( @@ -121,6 +128,7 @@ async def async_get_user_site(deps_dir: str) -> str: stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.DEVNULL, env=env, + close_fds=False, # required for posix_spawn ) stdout, _ = await process.communicate() lib_dir = stdout.decode().strip() diff --git a/tests/components/command_line/test_cover.py b/tests/components/command_line/test_cover.py index 8940f9d959f..220da18409c 100644 --- a/tests/components/command_line/test_cover.py +++ b/tests/components/command_line/test_cover.py @@ -73,7 +73,10 @@ async def test_poll_when_cover_has_command_state(hass: HomeAssistant) -> None: async_fire_time_changed(hass, dt_util.utcnow() + SCAN_INTERVAL) await hass.async_block_till_done() check_output.assert_called_once_with( - "echo state", shell=True, timeout=15 # nosec # shell by design + "echo state", + shell=True, # nosec # shell by design + timeout=15, + close_fds=False, ) diff --git a/tests/components/command_line/test_sensor.py b/tests/components/command_line/test_sensor.py index 0d86b3fe7f0..c67e97ef81a 100644 --- a/tests/components/command_line/test_sensor.py +++ b/tests/components/command_line/test_sensor.py @@ -97,6 +97,7 @@ async def test_template_render_with_quote(hass: HomeAssistant) -> None: 'echo "template_value" "3 4"', shell=True, # nosec # shell by design timeout=15, + close_fds=False, ) diff --git a/tests/test_runner.py b/tests/test_runner.py index 31e5e208cad..11a55031493 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -137,3 +137,22 @@ async def test_unhandled_exception_traceback(hass, caplog): assert "Task exception was never retrieved" in caplog.text assert "This is unhandled" in caplog.text assert "_unhandled_exception" in caplog.text + + +def test__enable_posix_spawn(): + """Test that we can enable posix_spawn on Alpine.""" + + def _mock_alpine_exists(path): + return path == "/etc/alpine-release" + + with patch.object(runner.subprocess, "_USE_POSIX_SPAWN", False), patch.object( + runner.os.path, "exists", _mock_alpine_exists + ): + runner._enable_posix_spawn() + assert runner.subprocess._USE_POSIX_SPAWN is True + + with patch.object(runner.subprocess, "_USE_POSIX_SPAWN", False), patch.object( + runner.os.path, "exists", return_value=False + ): + runner._enable_posix_spawn() + assert runner.subprocess._USE_POSIX_SPAWN is False diff --git a/tests/util/test_package.py b/tests/util/test_package.py index 2c0b4dc2e55..7c9ecdd75fe 100644 --- a/tests/util/test_package.py +++ b/tests/util/test_package.py @@ -95,6 +95,7 @@ def test_install(mock_sys, mock_popen, mock_env_copy, mock_venv): stdout=PIPE, stderr=PIPE, env=env, + close_fds=False, ) assert mock_popen.return_value.communicate.call_count == 1 @@ -118,6 +119,7 @@ def test_install_upgrade(mock_sys, mock_popen, mock_env_copy, mock_venv): stdout=PIPE, stderr=PIPE, env=env, + close_fds=False, ) assert mock_popen.return_value.communicate.call_count == 1 @@ -142,7 +144,7 @@ def test_install_target(mock_sys, mock_popen, mock_env_copy, mock_venv): assert package.install_package(TEST_NEW_REQ, False, target=target) assert mock_popen.call_count == 2 assert mock_popen.mock_calls[0] == call( - args, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=env + args, stdin=PIPE, stdout=PIPE, stderr=PIPE, env=env, close_fds=False ) assert mock_popen.return_value.communicate.call_count == 1 @@ -185,6 +187,7 @@ def test_install_constraint(mock_sys, mock_popen, mock_env_copy, mock_venv): stdout=PIPE, stderr=PIPE, env=env, + close_fds=False, ) assert mock_popen.return_value.communicate.call_count == 1 @@ -211,6 +214,7 @@ def test_install_find_links(mock_sys, mock_popen, mock_env_copy, mock_venv): stdout=PIPE, stderr=PIPE, env=env, + close_fds=False, ) assert mock_popen.return_value.communicate.call_count == 1 @@ -233,6 +237,7 @@ async def test_async_get_user_site(mock_env_copy): stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.DEVNULL, env=env, + close_fds=False, ) assert ret == os.path.join(deps_dir, "lib_dir")