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.
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
Use a new loop in run_app() #5572
Use a new loop in run_app() #5572
Changes from 6 commits
99d1c7f
c3e8c3c
2049e21
13ae734
0c79ed3
f7ee104
dce79e0
a2aa0a9
870ddda
1d89276
5bdc309
a5dd9b8
03dffb7
0621e30
add49b0
84f2a13
c9f1536
cdf1835
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't think it's a good idea to have it here. At least, put it right before
try:
. The reason is that it's an antipattern to have more than one instruction in a try block (it tends to shadow exceptions sometimes and causes hard-to-debug situations).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.
Yeah, I just noticed that it was inside the try block in
asyncio.run()
, figured there was probably a good reason for 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.
Well, you could add a separate try/except if you want to call
loop.close()
and re-raise. There's no need toasyncio.set_event_loop(None)
because setting it didn't happen anyway.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.
I've kept these 2 lines together in the try to match https://github.com/python/cpython/blob/main/Lib/asyncio/runners.py#L40-L44
But, I've moved the create_task() to before the try statement, as it doesn't seem to have any business being there.