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

Prevent GreenletExit exceptions (and improve shutdown time) when shutting down with active keepalive connections #1009

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

chriskuehl
Copy link
Member

@chriskuehl chriskuehl commented Oct 27, 2024

Internal ticket: SVF-438

This fixes an issue where a Baseplate WSGI server in shutdown doesn't proactively close existing "keepalive" connections and continues to process requests from them. This causes two issues:

  1. Requests can be killed in-flight with GreenletExit if the server's stop_time is reached.
  2. Shutdowns are unnecessarily slow because the gevent server waits for connections to be closed (but does nothing to close them itself).

This fix causes gevent to close connections once a server begins graceful shutdown (i.e. once server.stop() is called). It does this by adding the Connection: close header on responses and by closing connections in the pool.

Commentary

There doesn't appear to be an easy way to fix this without subclassing gevent's WSGIHandler and/or WSGIServer classes. These classes are designed to be subclassed according to the documentation, though it would still be better to fix this in gevent.

I believe most gevent users don't hit this as it is fairly rare (and not recommended) to use gevent's built-in WSGI server instead of something like gunicorn.

I've emailed the gevent mailing list to ask about this issue. If the reception is positive I'll submit a patch upstream.

Validation

  • First commit adds a regression test (expected to fail on develop branch).
  • Second commit adds the fix, which also fixes the regression test.

@chriskuehl chriskuehl force-pushed the gevent-shutdown-connections branch from b9bfb35 to a71cacd Compare October 27, 2024 02:27
This is expected to fail with the current Baseplate version.
@chriskuehl chriskuehl force-pushed the gevent-shutdown-connections branch from a71cacd to 593ceb4 Compare October 27, 2024 02:31
@chriskuehl chriskuehl marked this pull request as ready for review October 28, 2024 02:28
@chriskuehl chriskuehl requested a review from a team as a code owner October 28, 2024 02:28
Copy link

@mjbonifacio mjbonifacio left a comment

Choose a reason for hiding this comment

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

I'm informed and this appears sensible -- excited to try it. I wouldn't consider my approval authoritative as I don't think I'd be able to foresee other issues this might cause.

Copy link
Contributor

@salomon-smekecohen salomon-smekecohen left a comment

Choose a reason for hiding this comment

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

I believe the way the pywsgi and event modules are accessed will cause issues.
Screenshot 2024-11-02 at 6 00 49 PM

baseplate/server/wsgi.py Outdated Show resolved Hide resolved
tests/integration/requests_tests.py Outdated Show resolved Hide resolved
@chriskuehl chriskuehl merged commit 1cf7ad5 into develop Nov 14, 2024
4 checks passed
@chriskuehl chriskuehl deleted the gevent-shutdown-connections branch November 14, 2024 17:23
@salomon-smekecohen
Copy link
Contributor

ty! sorry for not re-reviewing quickly here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants