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 SIGQUIT handler to UvicornWorker #1710

Merged
merged 5 commits into from Nov 1, 2022

Conversation

adnaanbheda
Copy link
Contributor

@adnaanbheda adnaanbheda commented Oct 17, 2022

Gunicorn sends a SIGQUIT instead of a SIGINT even when it receives a SIGINT, in any case, uvicorn should be able to handle SIGQUIT as well, so this is a small fix that does that.

Fixes

Ref: benoitc/gunicorn#2604

uvicorn/workers.py Outdated Show resolved Hide resolved
@adnaanbheda adnaanbheda marked this pull request as ready for review October 19, 2022 16:13
@Kludex
Copy link
Sponsor Member

Kludex commented Oct 22, 2022

I have this Error while closing socket [Errno 9] Bad file descriptor when shutting down with this PR. We need to solve that before this gets in. 👀

uvicorn/workers.py Outdated Show resolved Hide resolved
@Kludex Kludex added this to the Version 0.20.0 milestone Oct 22, 2022
@Kludex
Copy link
Sponsor Member

Kludex commented Oct 27, 2022

Hi @adnaanbheda , are you still interested on working on this PR?

@adnaanbheda
Copy link
Contributor Author

@Kludex yep, lemme check on this tomorrow.

@Kludex Kludex mentioned this pull request Oct 28, 2022
13 tasks
@adnaanbheda
Copy link
Contributor Author

adnaanbheda commented Oct 28, 2022

@Kludex Managed to fix this by modifying the server, closing down / releasing the sockets first instead of the servers.

I understand that's not the way we want to go ahead, we would like to steer this change towards UvicornWorker ! 👀
But, I think the change is appropriate in the scope of uvicorn as well, let me know what you think.
Anyways, I'll look at this again and try to keep it worker-only!

@Kludex
Copy link
Sponsor Member

Kludex commented Oct 31, 2022

But, I think the change is appropriate in the scope of uvicorn as well, let me know what you think.

It's not. The order was changed on #566.

@Kludex
Copy link
Sponsor Member

Kludex commented Oct 31, 2022

So... Maybe the sock.close() logic shouldn't even be here, as the server.close() closes the socket as well, see here.

That said, the issue we are facing can be found on benoitc/gunicorn#1877.

Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Please revert the last commit, and we are good. 👍

The Bad descriptor issue is on gunicorn, as mentioned in the last comment.

uvicorn/workers.py Outdated Show resolved Hide resolved
Kludex
Kludex approved these changes Nov 1, 2022
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Thanks @adnaanbheda ! 👍

@Kludex Kludex changed the title SIGQUIT handling in UvicornWorker Add SIGQUIT handler to UvicornWorker Nov 1, 2022
@Kludex Kludex merged commit 4fd5077 into encode:master Nov 1, 2022
15 checks passed
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