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

Gracefully shutdown worker threads #47279

Merged
merged 8 commits into from
Apr 25, 2018
Merged

Conversation

dwoz
Copy link
Contributor

@dwoz dwoz commented Apr 24, 2018

What does this PR do?

Gracefully shutdown worker threads when TCPReqServerChannel is closed or destroyed.

Previous Behavior

Running thread causes processes not to terminate.

New Behavior

Threads will stop gracefully and processes will terminate as they should. This fixes the windows python 3 Jenkins build.

Tests written?

No - Fixes test suite

Commits signed with GPG?

No

@dwoz dwoz requested a review from gtmanfred April 24, 2018 17:25
@ghost ghost self-requested a review April 24, 2018 17:25
@dwoz dwoz force-pushed the py3_build_fix branch 2 times, most recently from 2327859 to b0f3627 Compare April 25, 2018 02:23
Since close can be called more than once handle and log exceptions
try:
self.req_server.stop()
except Exception as exc:
log.debug("TCPReqServerChannel close generated an exception: %s" str(exc))
Copy link
Contributor

Choose a reason for hiding this comment

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

This has a syntax error: there is a missing comma after %s".

@gtmanfred
Copy link
Contributor

re-run py

Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

I'll approve this but with a comment that can be handled in a follow-up PR.

try:
self.req_server.stop()
except Exception as exc:
log.debug("TCPReqServerChannel close generated an exception: %s", str(exc))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think log.exception would be better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can get rid of that completely by checking is_alive in the stop method. If not, I'll change this to log exception.

@cachedout cachedout merged commit e0765f5 into saltstack:2017.7 Apr 25, 2018
@dwoz dwoz deleted the py3_build_fix branch May 7, 2018 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants