#3227: Wait for the keepalive period to end before stopping the acceptance of new requests #3236
Conversation
michdan
changed the title
…ance of new requests. This will prevent potential disruptions in request handling.
michdan
force-pushed
the
3227_wait_for_keepalive_before_restart
branch
from
29b7866
to
34ce875
Compare
|
To test such functionality, consider using an approach inspired by Apache httpd: https://github.com/apache/httpd/blob/trunk/test/modules/core/test_002_restarts.py. This approach involves making assertions based on log output. While it would require implementing some additional tools, this method seems to simplify testing complex scenarios such as multiprocessing and greenlets. |
| # Retrieve all active greenlets and repeatedly check, until the keepalive period ends, | ||
| # if any of those greenlets are still active. If none are active, exit the loop. | ||
|
|
||
| greenlets = {id(greenlet) for server in servers for greenlet in server.pool.greenlets} |
There was a problem hiding this comment.
When this line is executed, self.alive is already False, which means that all keepalive connections are either closed or are being handled by the existing greenlets. No new keepalive connections will be created until the worker restarts.
Therefore, if the greenlets found in this line have finished, we do not need to wait any longer, as we are assured that there are no more active keepalive connections.
|
I am not yet confident I understand all possibly implications.. but whether further changed or not.. this definitely needs documentation (It already needed documentation before, but after the change the behaviour can be even more unexpected). Maybe a time table that lists the state transitions from hitting |
gunicorn/workers/ggevent.py
Outdated
| greenlets = {id(greenlet) for server in servers for greenlet in server.pool.greenlets} | ||
| ts = time.time() | ||
|
|
||
| while time.time() - ts <= self.cfg.keepalive: |
There was a problem hiding this comment.
obligatory "only do that with a monotonic clock" comment
There was a problem hiding this comment.
Thanks for pointing this out. I changed it to use a monotonic clock. By the way, shouldn't it be changed in other places as well? For example, I can see: https://github.com/benoitc/gunicorn/blob/master/gunicorn/workers/ggevent.py#L98
| @@ -87,6 +87,10 @@ def run(self): | |||
| self.notify() | |||
| gevent.sleep(1.0) | |||
|
|
|||
| # Wait for pending keepalive connections to complete before stopping the acceptance of new requests | |||
| if self.cfg.keepalive: | |||
There was a problem hiding this comment.
i disagree there, we shouldn't care about waiting for keepalive connections when closing. CLose should be done as fast as possible.
| @@ -36,7 +36,8 @@ def handle(self, listener, client, addr): | |||
| parser = http.RequestParser(self.cfg, client, addr) | |||
| try: | |||
| listener_name = listener.getsockname() | |||
| if not self.cfg.keepalive: | |||
| # do not allow keepalive if the worker is about to be restarted | |||
This change will prevent potential disruptions in request handling.
Currently, when a worker is restarted due to the max_request limit and the keepalive period is greater than zero, there is a gap between stopping the acceptance of new requests and starting a new worker. This happens because the process must wait for the lesser of the grace period and the keepalive period, even if there are no pending requests. It waits due to open connections that remain active for the duration of the keepalive period.
See also: #3227
cc: @benoitc @pajod