-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
bugfix: ASGI support for newer python versions > 3.9 #11541
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: I am not a maintainer
Quite a good solution, although I personally would do this differently. Either way, this would fix my issue (see #11545); it would be great, if you could mention it with the "closes" keyword :)
return self.response | ||
|
||
async def run_asgi(self, asgi_instance): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this should be run_asgi_instance
as it appears line 215 is trying to call it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@winglian - just checking to see if you saw this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should solve the issue. I have a couple more nitpicks, though, to (hopefully) make the code more readable :)
asgi_task = loop.create_task(asgi_instance) | ||
loop.run_until_complete(asgi_task) | ||
else: | ||
asyncio.run(self.run_asgi_instance(asgi_instance)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need a separate function? From what I see in the handler, at this point in time, we already know that app
(or app.__call__
) is a coroutine. Can't we just run it directly instead?
asyncio.run(self.run_asgi_instance(asgi_instance)) | |
asyncio.run(asgi_instance) |
@@ -191,23 +191,33 @@ def __init__(self, scope): | |||
self.state = ASGICycleState.REQUEST | |||
self.app_queue = None | |||
self.response = {} | |||
self._legacy_asyncio = sys.version_info < (3, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of creating a class member variable just for checking. How about we declare a local variable at the top of the __call__
function instead?
Closing in favor of #11675, which includes these commits and (some) feedback. |
When we added the option of getting `al2023` in addition to `al2`, this required a python version bump for those apps on `al2023`. Python 3.10 had a breaking minor change. #11541 mostly fixed it, with some comments. OP hasn't replied to feedback on that PR, so I've take it over from them.
using any version of python > 3.9 results in
<class 'TypeError'>
when trying to launch an application like FastAPIedit: closes #11545