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

3227: shutdown socket when worker is not alive #3228

Closed
wants to merge 1 commit into from

Conversation

michdan
Copy link

@michdan michdan commented Jun 13, 2024

More info: #3227

Simply close the waiting reads (pending due to keepalive) and allow the worker to restart without having to wait for the graceful period.

@pajod
Copy link
Contributor

pajod commented Jun 15, 2024

Would prefer a more obviously consistent behaviour for both GreenletExit origins. This looks more like a spot-fix to me. Suggest awaiting / working towards something we can run from pytest that gracefully restarts the worker in each of the possible states (idle, keepalive, busy) and asserts the workers behave consistently and without emitting the warning (so far, I only have a test for auto-reload of idle workers, and not even that one is close to ready for merge).

michdan pushed a commit to michdan/gunicorn_performance_fix_showcase that referenced this pull request Jun 17, 2024
@michdan
Copy link
Author

michdan commented Jun 17, 2024

Would prefer a more obviously consistent behaviour for both GreenletExit origins. This looks more like a spot-fix to me. Suggest awaiting / working towards something we can run from pytest that gracefully restarts the worker in each of the possible states (idle, keepalive, busy) and asserts the workers behave consistently and without emitting the warning (so far, I only have a test for auto-reload of idle workers, and not even that one is close to ready for merge).

@pajod Thanks for the quick response!

This is indeed a quick fix. It's up to you whether you want to release it before making major changes, but this fix would have a positive impact on Gunicorn's performance. It's not about handling GreenletExit, but rather preventing it and making worker restarts faster. To demonstrate this, I've created a repository with some showcases and recorded a screen from those showcases. You can watch it on YouTube here: https://youtu.be/xqv4vGKjGBM
The repo is here: https://github.com/michdan/gunicorn_performance_fix_showcase

@michdan michdan marked this pull request as draft June 28, 2024 09:52
@michdan
Copy link
Author

michdan commented Jun 28, 2024

I have moved it to a draft because it would require comprehensive testing, there is a risk that it can introduce another bug.
However, the problem described in #3227 is still unresolved

@michdan michdan marked this pull request as ready for review July 3, 2024 14:11
@michdan michdan marked this pull request as draft July 3, 2024 14:40
@michdan
Copy link
Author

michdan commented Jul 3, 2024

New version soon

@michdan michdan closed this Jul 3, 2024
@michdan michdan deleted the 3227_shutdown_socket branch July 4, 2024 22:20
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.

2 participants