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

gthread: only read sockets when they are readable (#2917) #2918

Merged
merged 1 commit into from
May 7, 2023

Conversation

JorisOnGithub
Copy link
Contributor

This PR solves issue #2917

It changes the behaviour of the gthread workers. Instead of always reading from tcp sockets opened by a client, the worker only does so when the socket is readable. The main thread does not immediately enqueue new connections anymore to the worker thread queue. Instead it adds new connections to the poller, these connections will be added to the worker thread queue when they are readable.

Please let me know if there are any questions or suggestions for this PR!

@TBoshoven
Copy link

Verified as follows that this resolves the issue:

  • Started gunicorn application with 4 gthread workers
  • Open four TCP connections to the listening port
  • Attempt to make a request

Before this change there was no answer. After this change there is.

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.

This looks great. Thanks for helping to resolve this issue!

I have only one question. Should we add the connection to the keepalive queue with an initial timeout? In other words, should we treat the connection as initially idle, and close it after the keepalive timeout?

@tilgovi tilgovi self-assigned this Mar 25, 2023
@TBoshoven
Copy link

should we treat the connection as initially idle, and close it after the keepalive timeout?

I think that's a reasonable way to approach this.

@benoitc
Copy link
Owner

benoitc commented May 7, 2023

This looks great. Thanks for helping to resolve this issue!

I have only one question. Should we add the connection to the keepalive queue with an initial timeout? In other words, should we treat the connection as initially idle, and close it after the keepalive timeout?

hrm yes I think that could be a nice addition as well.

@benoitc benoitc added this to the 21.0 release milestone May 7, 2023
@benoitc benoitc merged commit a423fca into benoitc:master May 7, 2023
@benoitc
Copy link
Owner

benoitc commented May 7, 2023

merged it , i will add the timeout in a separate commit. Thanks a lot for this patch! Thanks @tilgovi for the review as well.

@benoitc
Copy link
Owner

benoitc commented May 7, 2023

javiertejero added a commit to travelperk/gunicorn that referenced this pull request Feb 1, 2024
when opening many sockets the worker gets frozen consuming 100% cpu
javiertejero added a commit to travelperk/gunicorn that referenced this pull request Feb 1, 2024
when opening many sockets the worker gets frozen consuming 100% cpu

to reproduce:

```
cd examples
python standalone_app_gthread.py
```

and then using Apache Bench tool:
```
ab -n1000 -c100 http://0.0.0.0:8080/
```

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

Successfully merging this pull request may close these issues.

4 participants