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

add a delay between SIGABRT and SIGKILL for worker timeouts #2455

Closed
wants to merge 1 commit into from

Conversation

tardyp
Copy link

@tardyp tardyp commented Nov 10, 2020

candidate fix for #2454

@tilgovi
Copy link
Collaborator

tilgovi commented Feb 16, 2021

Thank you for the patch, but I don't think this is the way I'd like to handle this issue. This function is meant for killing workers that are stuck. A proper implementation of graceful shutdown in the async workers should continue to notify the arbiter of their liveness during shutdown. Not all workers are doing this correctly right now. There is some discussion in #2317 where I'm working on fixing this.

I'll close your PR, but if you think I've misunderstood I'll be happy to discuss further.

@tilgovi tilgovi closed this Feb 16, 2021
@tardyp
Copy link
Author

tardyp commented Feb 16, 2021

Hi @tilgovi, I indeed think there is a misunderstanding.

This fix is in reality for the worker stuck use case.
When this is the case, there is no delay between the SIKABRT and SIGKILL, because of the logic I am fixing.

In #2454, you say there is 1 second delay, please point me to the actual code that is introducing this delay, because I can't find it, and in my tests and logs I never see a one second delay between those two signals.

@tilgovi
Copy link
Collaborator

tilgovi commented Feb 16, 2021

@tardyp the arbiter process is supposed to sleep for one second if it does not receive any signals: https://github.com/benoitc/gunicorn/blob/master/gunicorn/arbiter.py#L357

We could re-open this PR and modify it if you find that this is not reliable enough, but we should not wait the whole graceful shutdown period since abort is not graceful.

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

Successfully merging this pull request may close these issues.

None yet

2 participants