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

Set SO_KEEPALIVE on listener socket #3116

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cedk
Copy link

@cedk cedk commented Dec 15, 2023

Closes #3115

Copy link
Collaborator

@tilgovi tilgovi left a comment

Choose a reason for hiding this comment

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

While this might be fine, I think it'd be even easier to say yes and merge with a configuration option. Any chance you want to update the PR to make this configurable?

@pajod
Copy link
Contributor

pajod commented Dec 27, 2023

What scenario would this be useful in (or even observably different from defaults)? Are you concerned about the 2 hour timeframe, which I fear may not be easy to write a test for on non-Linux systems?

@tilgovi
Copy link
Collaborator

tilgovi commented Dec 27, 2023

While the Linux default is 2 hours before keepalives start, and that is much higher than a typical Gunicorn HTTP keep-alive timeout, it's still a fine idea for cases where someone does set a very high HTTP keep-alive timeout or wants to detect stalled, long-lived requests or something. Maybe they set their system keepalive timeout to be very low.

Nevertheless, it's not enabled at all unless we set this socket option, so I think we probably should make that possible.

@tilgovi
Copy link
Collaborator

tilgovi commented Dec 27, 2023

The original linked issue talks about closing long running connections. A good example of where this is useful is probably if someone uses gunicorn with an application that handles WebSocket upgrades.

@benoitc
Copy link
Owner

benoitc commented Jan 5, 2024

I agree this should be an option. Using it as default will break some behaviors.

I am also unsure why it's needed for a proxy, HTTP connections should be stateless but it's good addion as an option anyway.

@cedk
Copy link
Author

cedk commented Jan 5, 2024

Indeed our host tells us to send keep-alive to avoid the proxy to close the request before it is answered.
But now they came back on this statement and just ask us to use asynchronous request.

So I'm no more sure if this option will be useful for us but I can add the option.

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.

Setting SO_KEEPALIVE on server socket
4 participants