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

remove checking timeouts while graceful stopping tornado worker #1106

Merged
merged 1 commit into from Oct 8, 2015

Conversation

Projects
None yet
3 participants
@keakon
Contributor

keakon commented Aug 26, 2015

Tornado add many timeouts internally which can be ignored while graceful stopping.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Aug 26, 2015

Collaborator

I think this is probably fine. @benoitc?

Collaborator

tilgovi commented Aug 26, 2015

I think this is probably fine. @benoitc?

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Sep 30, 2015

Owner

👍 @keakon can you eventually squash the commits? Sorry for the delay to review it, I'm incredibly busy these days...

Owner

benoitc commented Sep 30, 2015

👍 @keakon can you eventually squash the commits? Sorry for the delay to review it, I'm incredibly busy these days...

@keakon

This comment has been minimized.

Show comment
Hide comment
@keakon

keakon Oct 6, 2015

Contributor

Hello, @benoitc. The first 2 commits were about 2 years ago, and you should have merged it.

Actually, there would be anther issue when I using Supervisor to manage Gunicorn process.

The default graceful_timeout of Gunicorn is 30 seconds. So when I restarting Gunicorn master process, it will wait its workers to exit up to 30 seconds.
But the default stopwaitsecs of Supervisor is 10 seconds. So once the Gunicorn workers didn't exit within 10 seconds, the master would be killed by Supervisor, then the workers would become zombie processes.

I have changed the graceful_timeout to 8 seconds in my project, but I think there could be a mechanism to kill the workers once the master got killed.

Contributor

keakon commented Oct 6, 2015

Hello, @benoitc. The first 2 commits were about 2 years ago, and you should have merged it.

Actually, there would be anther issue when I using Supervisor to manage Gunicorn process.

The default graceful_timeout of Gunicorn is 30 seconds. So when I restarting Gunicorn master process, it will wait its workers to exit up to 30 seconds.
But the default stopwaitsecs of Supervisor is 10 seconds. So once the Gunicorn workers didn't exit within 10 seconds, the master would be killed by Supervisor, then the workers would become zombie processes.

I have changed the graceful_timeout to 8 seconds in my project, but I think there could be a mechanism to kill the workers once the master got killed.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Oct 7, 2015

Owner

I should probably have but I am really busy these days launching a new business.

About the patches can you split the prs? the possible infinite loop should be done in another PR.

About the timeout can't you tell to supervisor to send a SIGTERM instead of a SIGQUIT? There is indeed already a mechanism to exit immediately Gunicorn. If it doesn't work with the tornado worker, let me know.

Owner

benoitc commented Oct 7, 2015

I should probably have but I am really busy these days launching a new business.

About the patches can you split the prs? the possible infinite loop should be done in another PR.

About the timeout can't you tell to supervisor to send a SIGTERM instead of a SIGQUIT? There is indeed already a mechanism to exit immediately Gunicorn. If it doesn't work with the tornado worker, let me know.

@keakon

This comment has been minimized.

Show comment
Hide comment
@keakon

keakon Oct 8, 2015

Contributor

OK, I have updated the pull request.

Changing the signal is fine, but not graceful. Maybe adding a note in the document would be helpful.

Contributor

keakon commented Oct 8, 2015

OK, I have updated the pull request.

Changing the signal is fine, but not graceful. Maybe adding a note in the document would be helpful.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 8, 2015

Collaborator

It seems like this is ready to go. I'm going to merge it.

Collaborator

tilgovi commented Oct 8, 2015

It seems like this is ready to go. I'm going to merge it.

tilgovi added a commit that referenced this pull request Oct 8, 2015

Merge pull request #1106 from keakon/master
remove checking timeouts while graceful stopping tornado worker

@tilgovi tilgovi merged commit 9e9f1b6 into benoitc:master Oct 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Oct 8, 2015

Owner

@tilgovi thanks!

@keakon it's document there:
http://docs.gunicorn.org/en/19.3/signals.html

If you want something more explicit let me know :)

Owner

benoitc commented Oct 8, 2015

@tilgovi thanks!

@keakon it's document there:
http://docs.gunicorn.org/en/19.3/signals.html

If you want something more explicit let me know :)

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

Merge pull request #1106 from keakon/master
remove checking timeouts while graceful stopping tornado worker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment