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.

Sign up for GitHub

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

Jump to bottom

Fix Ctrl+C hang on Python 3.12 when connections are open #2145

Merged
merged 4 commits into from
Nov 4, 2023
Merged

Fix Ctrl+C hang on Python 3.12 when connections are open #2145

merged 4 commits into from
Nov 4, 2023

Conversation

jcheng5
Copy link
Contributor

@jcheng5 jcheng5 commented Nov 1, 2023 •
edited
Loading

Summary

The asyncio.Server.wait_closed() method was a no-op in versions of Python earlier than 3.12. The intention of this method was to block until all existing connections are closed, but due to a bug in its implementation, it wouldn't actually wait. This bug was fixed in Python 3.12, which exposed uvicorn's dependence on the buggy behavior: the implementation of uvicorn.Server.shutdown() called wait_closed() before asking existing connections to close. As a result, attempting to stop a Uvicorn server with an open connection would result in a blocked process, with further attempts to Ctrl+C having no effect.

See discussion at #2122

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@jcheng5
Don't wait_closed() on servers until connections are closed
a670b3b 
The asyncio.Server.wait_closed() method was a no-op in versions of Python
earlier than 3.12. The intention of this method was to block until all
existing connections are closed, but due to a bug in its implementation,
it wouldn't actually wait. This bug was fixed in Python 3.12, which
exposed uvicorn's dependence on the buggy behavior: the implementation of
uvicorn.Server.shutdown() called wait_closed() before asking existing
connections to close. As a result, attempting to stop a Uvicorn server
with an open connection would result in a blocked process, with further
attempts to Ctrl+C having no effect.
@jcheng5 jcheng5 changed the title Don't wait_closed() on servers until connections are closed Fix Ctrl+C hang on Python 3.12 when connections are open Nov 1, 2023
zanieb
zanieb approved these changes Nov 1, 2023
View reviewed changes
Copy link

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for contributing this fix

@zanieb
Copy link

zanieb commented Nov 1, 2023

Did you test this with Python 3.12?

@jcheng5
Copy link
Contributor Author

jcheng5 commented Nov 1, 2023 •
edited
Loading

@zanieb Yes, and I ran pytest on 3.12. Are there other tests I should run?

Edit: This was on Mac. I'll try on Windows as well

@jcheng5
Copy link
Contributor Author

jcheng5 commented Nov 1, 2023

Works well on Windows, except that on the combination of Windows Terminal and PowerShell, Uvicorn doesn't respond to Ctrl+C or Ctrl+Break at all, even with much older versions of Uvicorn. No problem with:

  • Windows Terminal + cmd
  • Windows console host + cmd
  • Windows console host + PowerShell

In all of those instances, the behavior is what I expect--hang when exiting with open connections without this fix, clean exit with this fix.

@jcheng5
Copy link
Contributor Author

jcheng5 commented Nov 1, 2023 •
edited
Loading

On the combination of Windows Terminal (v1.18.2822.0) and PowerShell, I can't even use Ctrl+C to stop py -m http.server. So I guess whatever is causing that is completely unrelated to Uvicorn and something I should figure out about my own configuration.

Update: Restarted Windows Terminal and now WT + PS work as expected, just like the other configurations. 🤷‍♂️

This was referenced Nov 1, 2023
Closed
Open
@zanieb
Copy link

zanieb commented Nov 2, 2023

Nice :)

@Kludex Kludex added the hold Don't merge yet label Nov 2, 2023
Kludex
Kludex reviewed Nov 2, 2023
View reviewed changes
uvicorn/server.py Outdated Show resolved Hide resolved
uvicorn/server.py Outdated Show resolved Hide resolved
jcheng5 and others added 2 commits November 3, 2023 09:42
@jcheng5 @Kludex
Code review feedback
92f48bc 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@jcheng5
Fix formatting
da2d5a1
@Kludex Kludex merged commit dff1a46 into encode:master Nov 4, 2023
15 checks passed
@foarsitter foarsitter mentioned this pull request Nov 4, 2023
Closed
@jcheng5
Copy link
Contributor Author

jcheng5 commented Nov 5, 2023

Thank you @zanieb and @Kludex! Our users will appreciate it!

@jcheng5 jcheng5 deleted the python-3-12-hang branch November 5, 2023 01:10
kmichel-aiven added a commit to Aiven-Open/astacus that referenced this pull request Apr 11, 2024
@kmichel-aiven
Fix dangling socket causing uvicorn shutdown failure
68bf182 
When using urllib3<=1.26.18 with uvicorn<=0.23.2 and python>=3.12,
the socket shutdown method of urllib3 combines with the uvicorn
server shutdown to leave the server stuck for many seconds:

encode/uvicorn#2145

Updating urllib3 or uvicorn (or downgrading Python) fixes the bug,
but Fedora 39 shipped Python 3.12 with old urllib3 and uvicorn.

Since we (sadly) open a connection just for a single request every
time, we can disable the HTTP keep-alive. This allows the server to
close the connection earlier, without waiting for the client.

This fixes the failing tests/system/test_astacus.py::test_astacus on
Fedora 39.
@kmichel-aiven kmichel-aiven mentioned this pull request Apr 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kludex Kludex Kludex left review comments

@zanieb zanieb zanieb approved these changes

Assignees
No one assigned
Labels
hold Don't merge yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jcheng5 @zanieb @Kludex