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

Conversation

@dmurphy18
Copy link
Contributor

commented Jul 25, 2019

What does this PR do?

Checks if pid exists before using psutil to kill any children of the pid

What issues does this PR fix or reference?

https://jenkinsci.saltstack.com/job/2019.2.1/job/salt-debian-9-py2/90/testReport/junit/unit.transport.test_zeromq/PubServerChannel/test_publish_to_pubserv_tcp

Previous Behavior

Test would fail.

New Behavior

test should pass. The problem was that the ThreadExecutorPool executor.submit (4 of them) are handling the signal for the main thread too, which by the time they are called, is terminated. Fix is to check if the pid exists before having psutil attempt to use it and kill any existing children.

Tests written?

Yes, using existing tests, e.g. unit/transport/test_zeromq.py

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

salt/utils/process.py Outdated Show resolved Hide resolved
@dmurphy18 dmurphy18 requested review from Ch3LL and dwoz Jul 26, 2019
salt/utils/process.py Outdated Show resolved Hide resolved
salt/utils/process.py Outdated Show resolved Hide resolved
@dmurphy18 dmurphy18 force-pushed the dmurphy18:fix_deb9_build90_tests branch from c60946c to aa2a43f Jul 29, 2019
if hasattr(process, 'children'):
for child in process.children(recursive=True):
if child.is_running():
child.terminate()

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 29, 2019

Contributor

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?

This comment has been minimized.

Copy link
@dwoz

dwoz Jul 31, 2019

Contributor

@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.

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 31, 2019

Contributor

@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.

if hasattr(process, 'children'):
for child in process.children(recursive=True):
if child.is_running():
child.terminate()

This comment has been minimized.

Copy link
@dwoz

dwoz Jul 31, 2019

Contributor

@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.

@dmurphy18

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@rares-pop @dwoz While just executing the try/except and catching the exception is still functional code, testing and skipping is faster code than having to handle an exception case, but this is a closing case, speed not really an issue. I shall adjust it so each child is tried rather than exiting after the first one, as for making it a utility and associated unit tests, I believe that is over-complicating code.

@dmurphy18 dmurphy18 force-pushed the dmurphy18:fix_deb9_build90_tests branch from aa2a43f to 406d382 Jul 31, 2019
dwoz added 2 commits Jul 31, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

ping @rares-pop can you re-review here? thanks

dmurphy18 added 2 commits Aug 1, 2019
Fix deb9 build90 tests
@waynew
waynew approved these changes Aug 1, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

ping @dmurphy18 some lint failures in the new tests.

David Murphy
Copy link
Contributor

left a comment

LGTM!

David Murphy
@waynew
waynew approved these changes Aug 2, 2019
@dwoz
dwoz approved these changes Aug 2, 2019
@dwoz dwoz merged commit 2cb5a0b into saltstack:2019.2.1 Aug 2, 2019
24 checks passed
24 checks passed
WIP Ready for review
Details
ci/docs This commit looks good
Details
ci/lint This commit looks good
Details
ci/py2/amazon2 This commit looks good
Details
ci/py2/centos6 This commit looks good
Details
ci/py2/centos7 This commit looks good
Details
ci/py2/centos7/tcp This commit looks good
Details
ci/py2/debian8 This commit looks good
Details
ci/py2/debian9 This commit looks good
Details
ci/py2/fedora29 This commit looks good
Details
ci/py2/ubuntu1604 This commit looks good
Details
ci/py2/ubuntu1604/tcp This commit looks good
Details
ci/py2/ubuntu1804 This commit looks good
Details
ci/py2/windows2016 This commit looks good
Details
ci/py3/amazon2 This commit looks good
Details
ci/py3/centos7 This commit looks good
Details
ci/py3/centos7/tcp This commit looks good
Details
ci/py3/debian8 This commit looks good
Details
ci/py3/debian9 This commit looks good
Details
ci/py3/fedora29 This commit looks good
Details
ci/py3/ubuntu1604 This commit looks good
Details
ci/py3/ubuntu1604/tcp This commit looks good
Details
ci/py3/ubuntu1804 This commit looks good
Details
ci/py3/windows2016 This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.