workers/gthread: Remove locks + one event queue + general cleanup #3157
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sylt
force-pushed
the
gthread-cleanup
branch
from
ddb1ece
to
17e7abf
Compare
The main purpose is to remove complexity from gthread by: * Removing the lock for handling self._keep and self.poller. This is possible since we now do all such manipulation on the main thread instead. When a connection is done, it posts a callback through the PollableMethodCaller which gets executed on the main thread. * Having a single event queue (self.poller), as opposed to also managing a set of futures. This fixes benoitc#3146 (although there are more minimal ways of doing it). There are other more minor things as well: * Renaming some variables, e.g. self._keep to self.keepalived_conns. * Remove self-explanatory comments (what the code does, not why). * Just decide that socket is blocking. Some complexity has been added to the shutdown sequence, but hopefully for good reason: it's to make sure that all already accepted connections are served within the grace period.
sylt
force-pushed
the
gthread-cleanup
branch
from
17e7abf
to
04243f3
Compare
pajod
reviewed
May 5, 2024
| self.set_accept_enabled(False) | ||
|
|
||
| # ... but try handle all already accepted connections within the grace period | ||
| graceful_timeout = time.time() + self.cfg.graceful_timeout |
There was a problem hiding this comment.
Similar to #1967 this should use a monotonic clock, as the timeout is specified in seconds, with no regard for UTC sync or leap second.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main purpose is to remove complexity from gthread by:
Removing the lock for handling self._keep and self.poller. This is possible since we now do all such manipulation on the main thread instead. When a connection is done, it posts a callback through the PollableMethodCaller which gets executed on the main thread.
Having a single event queue (self.poller), as opposed to also managing a set of futures. This fixes Gunicorn gthread deadlock #3146 (although there are more minimal ways of doing it).
There are other more minor things as well:
Some complexity has been added to the shutdown sequence, but hopefully for good reason: it's to make sure that all already accepted connections are served within the grace period.