From 4543837fc674f82f131c6a1e0e7e897461feaffd Mon Sep 17 00:00:00 2001 From: eekstunt <51318131+eekstunt@users.noreply.github.com> Date: Thu, 18 Jul 2024 04:56:47 +0100 Subject: [PATCH] Enhance dotenv run: Switch to execvpe for better resource management and signal handling (#523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current implementation of `dotenv run` CLI uses `subprocess.Popen`, which spawns a child process to execute the specified command. ``` p = Popen(command, universal_newlines=True, bufsize=0, shell=False, env=cmd_env) ``` After spawning the child process, it exits with the same exit code returned by the child process. ``` ret = run_command(commandline, dotenv_as_dict) exit(ret) ``` ### We can enhance `dotenv run` usage dramatically while preserving exactly the same behaviour By switching to `os.execvpe` instead of `subprocess.Popen`, we can replace the parent dotenv process with the new process specified by the user. This results in only one active process—the program the user intended to run. **Benefits:** 1. No hanging parent process `dotenv run` acts as a launcher, so after executing `dotenv run redis-server`, only the Redis server process remains. The dotenv process, along with its Python interpreter, is completely replaced. This prevents the dotenv process from consuming RAM and other resources, which would otherwise persist until the Redis server exits. 2. Proper signal handling When using `subprocess.Popen`, the parent process (e.g., `dotenv`) remains responsible for handling and forwarding signals, which can lead to issues if the command doesn’t receive them directly. For instance, in Docker, if Redis was started without `exec`, it may not get important signals like `SIGTERM` when the container stops, potentially resulting in improper shutdowns or zombie processes. Using `os.execvpe` ensures that the command receives signals directly, improving reliability and making `dotenv` more suitable for production environments and improving reliability for DevOps engineers managing containerized applications. All current logic will be preserved because dotenv run does not do anything special except propagate the child process exit code. Thanks / @eekstunt --- src/dotenv/cli.py | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/dotenv/cli.py b/src/dotenv/cli.py index 65ead461..b5a97f8b 100644 --- a/src/dotenv/cli.py +++ b/src/dotenv/cli.py @@ -3,7 +3,6 @@ import shlex import sys from contextlib import contextmanager -from subprocess import Popen from typing import Any, Dict, IO, Iterator, List try: @@ -161,14 +160,13 @@ def run(ctx: click.Context, override: bool, commandline: List[str]) -> None: if not commandline: click.echo('No command given.') exit(1) - ret = run_command(commandline, dotenv_as_dict) - exit(ret) + run_command(commandline, dotenv_as_dict) -def run_command(command: List[str], env: Dict[str, str]) -> int: - """Run command in sub process. +def run_command(command: List[str], env: Dict[str, str]) -> None: + """Replace the current process with the specified command. - Runs the command in a sub process with the variables from `env` + Replaces the current process with the specified command and the variables from `env` added in the current environment variables. Parameters @@ -180,8 +178,8 @@ def run_command(command: List[str], env: Dict[str, str]) -> int: Returns ------- - int - The return code of the command + None + This function does not return any value. It replaces the current process with the new one. """ # copy the current environment variables and add the vales from @@ -189,11 +187,4 @@ def run_command(command: List[str], env: Dict[str, str]) -> int: cmd_env = os.environ.copy() cmd_env.update(env) - p = Popen(command, - universal_newlines=True, - bufsize=0, - shell=False, - env=cmd_env) - _, _ = p.communicate() - - return p.returncode + os.execvpe(command[0], args=command, env=cmd_env)