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
make graceful shutdown thread-safe #1033
Conversation
+1 can anyone else review it? So far looks good for a merge |
how can I reproduce the issue? |
Sorry I didn't see the related issue #1032 . looking at it. |
@@ -50,10 +49,27 @@ def watchdog(self): | |||
self.log.info("Parent changed, shutting down: %s", self) | |||
self.stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stop
method is not defined. You have remove the method stop
.
@fatelei Thanks.I'm so sorry for missed it out.Parent changed and Auto-restart has been tested. |
def heartbeat(self): | ||
if not self.alive: | ||
if self.server_alive: | ||
if hasattr(self, 'server'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we write this like the following snippet? (untested)
if hasattr(self, 'server'):
try:
self.server.stop()
except Exception:
pass
self.server_alive = False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, if the server doesn't exist we need to stop the worker anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception is ignored,but in fact we don't know how to deal with it.LGTM.
LGTM, thanks! Two minor comments:
|
done. |
make graceful shutdown thread-safe
No description provided.