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

Handle thread shutdown on system exit #49451

Merged
merged 6 commits into from Aug 30, 2018

Conversation

@gtmanfred
Contributor

gtmanfred commented Aug 30, 2018

What does this PR do?

Handle changes made for https://bugs.python.org/issue18966 in python 3.7

What issues does this PR fix or reference?

Fix saltstack/salt-jenkins#1075

Tests written?

Yes

Commits signed with GPG?

Yes

@rallytime

This looks good to me, well done! But I would like some other folks to review it as well.

@gtmanfred gtmanfred force-pushed the gtmanfred:fluorine branch from c3e8c46 to 27f71f1 Aug 30, 2018

@garethgreenaway

This comment has been minimized.

Member

garethgreenaway commented Aug 30, 2018

Seeing this when attempting to start the minion:

Process Process-1:
Traceback (most recent call last):
  File "/home/gareth/.pyenv/versions/3.7.0/lib/python3.7/multiprocessing/process.py", line 297, in _bootstrap
    self.run()
  File "/home/gareth/.pyenv/versions/3.7.0/lib/python3.7/multiprocessing/process.py", line 99, in run
    self._target(*self._args, **self._kwargs)
  File "/home/gareth/code/salt/salt/scripts.py", line 115, in minion_process
    lock = threading.lock()
AttributeError: module 'threading' has no attribute 'lock'

@gtmanfred gtmanfred force-pushed the gtmanfred:fluorine branch from 27f71f1 to f4aed36 Aug 30, 2018

@garethgreenaway

This comment has been minimized.

Member

garethgreenaway commented Aug 30, 2018

Running with 3.7.0 works great.

Running in the foreground with 2.7.15 and sending the KeyboardInterrupt:

The Salt Minion is shutdown. Minion received a SIGINT. Exited.
Process Process-1:
Traceback (most recent call last):
  File "/home/gareth/.pyenv/versions/2.7.15/lib/python2.7/multiprocessing/process.py", line 267, in _bootstrap
    self.run()
  File "/home/gareth/.pyenv/versions/2.7.15/lib/python2.7/multiprocessing/process.py", line 114, in run
    self._target(*self._args, **self._kwargs)
  File "/home/gareth/code/salt/salt/scripts.py", line 160, in minion_process
    lock.acquire(blocking=True)
TypeError: acquire() takes no keyword arguments
@gtmanfred

This comment has been minimized.

Contributor

gtmanfred commented Aug 30, 2018

bah, i thought it should accept blocking as a call :/ https://docs.python.org/2/library/threading.html#threading.Lock.acquire

double checking now, might be worthwhile to just gate behind a check for python 3.7.

gtmanfred added some commits Aug 30, 2018

@garethgreenaway

This comment has been minimized.

Member

garethgreenaway commented Aug 30, 2018

Looking good on both python 2.7.15 and 3.7.0.

@DmitryKuzmenko

Tricky, smart and awesome! My best to you @gtmanfred!

@dwoz dwoz self-requested a review Aug 30, 2018

@dwoz

dwoz approved these changes Aug 30, 2018

@gtmanfred

This comment has been minimized.

Contributor

gtmanfred commented Aug 30, 2018

@gtmanfred gtmanfred merged commit 5b32954 into saltstack:fluorine Aug 30, 2018

5 of 8 checks passed

jenkins/pr/py2-centos-7 running py2-centos-7...
Details
jenkins/pr/py3-centos-7 running py3-centos-7...
Details
jenkins/pr/py3-ubuntu-1604 running py3-ubuntu-1604...
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details

rallytime added a commit that referenced this pull request Aug 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment