Add setpgid for all called commands #1494
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eikenb
force-pushed
the
child-set-process-group
branch
from
41460d9 to
13b5204
Compare
eikenb
force-pushed
the
child-set-process-group
branch
from
13b5204 to
306ce90
Compare
Need to make sure signals are sent to all processes as sub-shells interfere with signal propagation. Using Setpgid to create a process group for the command and all it's subprocesses fixes this by allowing child to send the signals to the pgid, which thus gets sent to all processes in that group. This is required for upcoming change to use sub-shells for all executed commands to eliminate need for shell parsing. With this change it is always set to true and the pgid is always used to send signals to the process(es). In the future it could be made configurable if needed.
Small issue with subshells sending text to STDERR about the signal received, breaking the text check (result of sending signals to process group). Changed them to only check STDOUT, where the test text is sent. Previously used a sleep time of 500 milliseconds when 50 worked fine. This sped up the child tests a lot. Everything else are cleaning up lots of useless checks, hardcoded times, and replace subshell test calls use of bash with standard /bin/sh.
eikenb
force-pushed
the
child-set-process-group
branch
from
306ce90 to
7d17648
Compare
amCap1712
added a commit
to amCap1712/consul-template
that referenced
this pull request
Nov 26, 2023
hashicorp#1494 added the setpgid setting to ensure signals are propagated to all processes and not just the subshell. However, this might not always be desirable. For instance, servers like uWSGI run workers in subprocesses and wait for workers to finish processing active requests for a while before restarting them to support graceful reloading. consul-template sending the reload signal to all processes, sends singals to these worker processes in uWSGI as well which can interfere with graceful reloads. Currently, we are working around this by using the array form of the command which does not set the pgid on the child process. But I feel it might be useful to make this option configurable as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Need to make sure signals are sent to all processes as sub-shells interfere with signal propagation. Using Setpgid to create a process group for the command and all it's subprocesses fixes this by allowing child to send the signals to the pgid, which thus gets sent to all processes in that group.
This is required for upcoming change to use sub-shells for all executed commands to eliminate need for shell parsing.
With this change it is always set to true and the pgid is always used to send signals to the process(es).
In the future it could be made configurable if needed.
Might be easier to review using the commits. The second commit is test cleanups and fixes where the first one contains the new code and tests.