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

Kill worker on UnknownResourceFault's during a heartbeat (#88) #263

Merged
merged 2 commits into from
May 18, 2017

Conversation

jbbarth
Copy link
Collaborator

@jbbarth jbbarth commented May 18, 2017

Note that we use SIGKILL here, which might look a bit violent for the
purpose of stopping a process (the process won't be able to cleanup
anything before dying for instance). This should probably be a SIGTERM
but we already handle SIGTERM signals today and we alias it to a
graceful shutdown. Maybe we should change this behavior, but that's a
first version.

Closes #88.

Note that we use SIGKILL here, which might look a bit violent for the
purpose of stopping a process (the process won't be able to cleanup
anything before dying for instance). This should probably be a SIGTERM
but we already handle SIGTERM signals today and we alias it to a
graceful shutdown. Maybe we should change this behavior, but that's a
first version.

Closes #88.
@jbbarth jbbarth self-assigned this May 18, 2017
@jbbarth jbbarth requested a review from ybastide May 18, 2017 13:54
Copy link
Contributor

@ybastide ybastide left a comment

Choose a reason for hiding this comment

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

Minor comment 🙂

# that the worker process is still alive.
os.kill(worker.pid, signal.SIGKILL)
except OSError as e:
if "No such process" not in e.strerror:
Copy link
Contributor

Choose a reason for hiding this comment

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

if e.errno != errno.ESRCH:

@ybastide
Copy link
Contributor

Check failed: json_dumps need to set sort_key; PR incoming.

@@ -229,7 +229,8 @@ def worker_alive():
# that the worker process is still alive.
os.kill(worker.pid, signal.SIGKILL)
except OSError as e:
if "No such process" not in e.strerror:
# Compare errno to the errno for "No such process"
if e.errno != errno.ESRCH:
Copy link
Contributor

Choose a reason for hiding this comment

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

(Missing import)

@jbbarth
Copy link
Collaborator Author

jbbarth commented May 18, 2017

Worked this time... The PR about json_dumps/sort_key would be welcome though! Thanks!

@ybastide
Copy link
Contributor

#264, maybe I'll merge it without waiting Travis to timeout

@jbbarth jbbarth merged commit cf778d5 into master May 18, 2017
@jbbarth jbbarth deleted the enhancement/kill-worker branch May 18, 2017 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants