Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for main thread having terminated pid, before ThreadPoolExecutor threads #54017

Merged
merged 11 commits into from Aug 2, 2019
24 changes: 19 additions & 5 deletions salt/utils/process.py
Expand Up @@ -807,11 +807,25 @@ def _handle_signals(self, signum, sigframe):
msg += '. Exiting'
log.debug(msg)
if HAS_PSUTIL:
process = psutil.Process(self.pid)
if hasattr(process, 'children'):
for child in process.children(recursive=True):
if child.is_running():
child.terminate()
try:
process = psutil.Process(self.pid)
if hasattr(process, 'children'):
for child in process.children(recursive=True):
try:
if child.is_running():
child.terminate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not mentioning this earlier, but maybe it's worth adding a try-catch here as well, if any children fail to terminate (due to race-conditions), to still try to terminate the rest of the child (not to stop on the first one that fails). Or maybe it's okay to stop on the first one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rares-pop @dmurphy18 I don't think we need this if psutil.pid_exists(self.pid): at all. Just let it raise an exception if the parent doesn't exist. However, when iterating through the children I think we should except each one individually in case one goes away but others don't.

Maybe move this functionality to a utility method and add some unit tests for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwoz - that was exactly my feedback too. 👍

The first comment was about the unneeded pid_exists (I mentioned the possible race-condition), and the second about having the try-raise for children.

except psutil.NoSuchProcess:
log.warn(
'Unable to kill child of process %d, it does '
'not exist. My pid is %d',
self.pid, os.getpid()
)
except psutil.NoSuchProcess:
log.warn(
'Unable to kill children of process %d, it does not exist.'
'My pid is %d',
self.pid, os.getpid()
)
sys.exit(salt.defaults.exitcodes.EX_OK)

def start(self):
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/utils/test_process.py
Expand Up @@ -24,6 +24,7 @@
# Import 3rd-party libs
from salt.ext import six
from salt.ext.six.moves import range # pylint: disable=import-error,redefined-builtin
import psutil


def die(func):
Expand Down Expand Up @@ -233,3 +234,37 @@ def test_daemonize_if(self):
salt.utils.process.daemonize_if({})
self.assertTrue(salt.utils.process.daemonize.called)
# pylint: enable=assignment-from-none


@skipIf(sys.platform.startswith('win'), 'pickling nested function errors on Windows')
class TestSignalHandlingMultiprocessingProcess(TestCase):

@skipIf(NO_MOCK, NO_MOCK_REASON)
def test_process_does_not_exist(self):
def Process(pid):
raise psutil.NoSuchProcess(pid)

def target():
os.kill(os.getpid(), signal.SIGTERM)

try:
with patch('psutil.Process', Process):
proc = salt.utils.process.SignalHandlingMultiprocessingProcess(target=target)
proc.start()
except psutil.NoSuchProcess:
assert False, "psutil.NoSuchProcess raised"

@skipIf(NO_MOCK, NO_MOCK_REASON)
def test_process_children_do_not_exist(self):
def children(*args, **kwargs):
raise psutil.NoSuchProcess(1)

def target():
os.kill(os.getpid(), signal.SIGTERM)

try:
with patch('psutil.Process.children', children):
proc = salt.utils.process.SignalHandlingMultiprocessingProcess(target=target)
proc.start()
except psutil.NoSuchProcess:
assert False, "psutil.NoSuchProcess raised"