gh-109538: Catch closed loop runtime error and issue warning #111983
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
5dd6f38 to
a0dbe8b
Compare
Misc/NEWS.d/next/Library/2023-11-11-16-42-48.gh-issue-109538.cMG5ux.rst
Outdated
Show resolved
Hide resolved
2f72974 to
075a7fd
Compare
…MG5ux.rst Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
075a7fd to
7461e4e
Compare
|
Tip:
https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide |
I see, thanks for letting me know. |
Lib/asyncio/streams.py
Outdated
|
|
||
| try: | ||
| self.close() | ||
| except RuntimeError: |
There was a problem hiding this comment.
Catching all RuntimeErrors always feels scary -- it could mark other, more serious bugs. How about only issuing the new warning if self._loop.is_closed()?
There was a problem hiding this comment.
Change looks good. Thanks. Happy to merge after we have a test. This would be the file to add the test: https://github.com/python/cpython/blob/main/Lib/test/test_asyncio/test_streams.py
|
Thanks @dpr-0 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
|
Sorry, @dpr-0 and @gvanrossum, I could not cleanly backport this to |
|
Sorry, @dpr-0 and @gvanrossum, I could not cleanly backport this to |
|
@dpr-0 Are you interested in attempting the backports? |
Yes, any brief backport steps or I just cherry-pick and create two PRs? |
|
I think it's just a matter of checking out the right branch (3.11 or 3.12), cherry-picking the commit from main, resolving conflicts, and submitting a PR. Repeat for the other branch. The PR title should follow certain conventions (see either the devguide or just wait for the test to fail -- I always forget what it is and I use the latter). I don't know what caused the conflict -- if it's complicated just ping this thread with the conflict info. |
| try: | ||
| self.loop.run_until_complete(inner(httpd)) | ||
| # This exception is caused by `self.loop.stop()` as expected. | ||
| except RuntimeError: |
There was a problem hiding this comment.
If this exception is expected then why not use self.assertRaises etc?
There was a problem hiding this comment.
except allows/tolerates an exception, assert requires one and will fail if it does not happen because of other improvements.
|
@kumaraditya303 @gvanrossum I am planning to backport this PR and issue a new PR to improve test case. |
|
GH-112141 is a backport of this pull request to the 3.11 branch. |
|
GH-112142 is a backport of this pull request to the 3.12 branch. |
|
GH-112141 is a backport of this pull request to the 3.12 branch. |
|
GH-112141 is a backport of this pull request to the 3.11 branch. |
|
GH-112142 is a backport of this pull request to the 3.12 branch. |
Catch RuntimeError("Event loop is closed") from base_events.py line 514 then issue a warning to user