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

workers should ignore HUP signal Only master should kill them #1868

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ashishmjn
Copy link

No description provided.

@benoitc
Copy link
Owner

benoitc commented Aug 29, 2018

what issue are you trying to solve?

Copy link
Owner

@benoitc benoitc left a comment

Choose a reason for hiding this comment

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

I disagree with this change. USR signal is actually resetted to default in the worker (set to SIG_DFL), which the whole point of having it there. Why do you want to ignore it anyway?

@@ -415,6 +415,9 @@ def reload(self):
for i in range(self.cfg.workers):
self.spawn_worker()

#Sleeping so that new workers get time to boot
time.sleep(2)
Copy link
Owner

Choose a reason for hiding this comment

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

the arbiter should be able to manage immediately the new worker. There is no need to sleep thre normally, why adding it?

Copy link
Author

Choose a reason for hiding this comment

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

Wanted to give some time to the newly spawned workers to initialise themselves.
By I guess you are correct it should not stop the arbiter to manage the worker count. Will remove this.

@benoitc
Copy link
Owner

benoitc commented Sep 15, 2018

@ashishmjn bump, which issue are you trying to solve with this patch?

@ashishmjn
Copy link
Author

@benoitc Sorry for the late reply.
The problem I wanted to solve here was, whenever I wanted to reload the gunicorn workers I used the command pkill -HUP gunicorn, I saw that my app was not serving for brief period.
What I got to know from the documentation is that HUP signal is not handled by the worker processes.
So as soon as I executed the command the worker processes exited by receiving the HUP SIG along with the master process.
And till the time master process spawn new workers there was no one to serve the request.
So I am sure if this correct or not but the HUP signal should also be handled by the worker process (either by exiting gracefully or ignoring).
Correct me if I'm wrong.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 17, 2018

I don't think I'm opposed to this, but you can work around it by not using pkill this way. Use the --pidfile option and send the signal to the master directly.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 24, 2018

I would be more inclined to accept a handle_exit, as this is what happens when the arbiter gets SIGHUP (it sends SIGTERM to the workers).

@benoitc how do you feel about that change?

If that's acceptable, I propose we document it in docs/source/signals.rst and commit it just to master (no backport).

@ashishmjn
Copy link
Author

@tilgovi Agreed
Worker should gracefully exit as in the case of TERM signal

@benoitc
Copy link
Owner

benoitc commented Sep 26, 2018

i guess it should work, provided we don't break any other applications that may using it. If we do the patch we should indeed document it and mark it as a breaking change imo.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 26, 2018

We initialize signals before post_worker_init hook and load_wsgi, so hopefully anyone who relies on setting up their own signals in their WSGI application can still override the behavior.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 26, 2018

Agree that we'll need to just document it carefully.

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

Successfully merging this pull request may close these issues.

3 participants