From a95213f54908687e9a27c4e1a4ef107352f0d729 Mon Sep 17 00:00:00 2001 From: Wynn Wilkes Date: Fri, 30 Apr 2021 15:50:07 -0600 Subject: [PATCH] Tweaks to the initial environment_{file|loader} implementation - Remove the blocking call to subprocess.check_output in the main supervisord process. Instead make that call in the child process immediately after the initial fork, before doing any changes to the process config and state. This keeps the blocking call out of the danger zone, while still keeping the simple design. --- supervisor/options.py | 21 ++++++++------------- supervisor/process.py | 15 ++++++++++----- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/supervisor/options.py b/supervisor/options.py index 3dc84afd9..86a5a6f4b 100644 --- a/supervisor/options.py +++ b/supervisor/options.py @@ -1955,7 +1955,8 @@ def make_dispatchers(self, proc): def load_external_environment_definition(self): return self.load_external_environment_definition_for_config(self) - # this is separated out in order to make it easier to test + # NOTE - THIS IS BLOCKING CODE AND MUST ONLY BE CALLED IN TESTS OR IN CHILD PROCESSES, NOT THE + # MAIN SUPERVISORD THREAD OF EXECUTION @classmethod def load_external_environment_definition_for_config(cls, config): # lazily load extra env vars before we drop privileges so that this can be used to load a secrets file @@ -1976,21 +1977,15 @@ def load_external_environment_definition_for_config(cls, config): elif config.environment_loader: try: - from subprocess import check_output, CalledProcessError - kwargs = dict(shell=True) - if sys.version_info.major >= 3: - if sys.version_info.minor >= 7: - kwargs['text'] = True - else: - pass # we will decode the bytes returned after reading it for these versions of python - - envdata = check_output(config.environment_loader, **kwargs) + from subprocess import check_output, CalledProcessError, STDOUT as subprocess_STDOUT - if sys.version_info.major >= 3 and sys.version_info.minor < 7: - envdata = envdata.decode('utf8') + envdata = check_output(config.environment_loader, shell=True, stderr=subprocess_STDOUT) + envdata = as_string(envdata) except CalledProcessError as e: - raise ProcessException("environment_loader failure with %s: %d, %s" % (config.environment_loader, e.returncode, e.output)) + raise ProcessException("environment_loader failure with %s: %d, %s" % \ + (config.environment_loader, e.returncode, as_string(e.output)) + ) if envdata: extra_env = {} diff --git a/supervisor/process.py b/supervisor/process.py index b047886e5..e303a6b2d 100644 --- a/supervisor/process.py +++ b/supervisor/process.py @@ -218,9 +218,6 @@ def spawn(self): try: filename, argv = self.get_execv_args() - # check the environment_file/environment_loader options before we fork to simplify child process management - extra_env = self.config.load_external_environment_definition() - except ProcessException as what: self.record_spawnerr(what.args[0]) self._assertInState(ProcessStates.STARTING) @@ -264,7 +261,7 @@ def spawn(self): return self._spawn_as_parent(pid) else: - return self._spawn_as_child(filename, argv, extra_env=extra_env) + return self._spawn_as_child(filename, argv) def _spawn_as_parent(self, pid): # Parent @@ -288,9 +285,17 @@ def _prepare_child_fds(self): for i in range(3, options.minfds): options.close_fd(i) - def _spawn_as_child(self, filename, argv, extra_env=None): + def _spawn_as_child(self, filename, argv): options = self.config.options try: + # check the environment_file/environment_loader options after forking in order to avoid blocking the + # main supervisord thread, but do it before we start to mix up the process signals/state + try: + extra_env = self.config.load_external_environment_definition() + except ProcessException as e: + self.record_spawnerr(e.args[0]) + raise + # prevent child from receiving signals sent to the # parent by calling os.setpgrp to create a new process # group for the child; this prevents, for instance,