GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-109538: Catch closed loop runtime error and issue warning by dpr-0 · Pull Request #111983 · python/cpython

Conversation

@dpr-0

@dpr-0 dpr-0 commented Nov 11, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Catch RuntimeError("Event loop is closed") from base_events.py line 514 then issue a warning to user

  • Issue: 3.11.5 regression: StreamWriter.__del__ fails if event loop is already closed #109538

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
edgarrmondragon reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@dpr-0
dpr-0 requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners November 11, 2023 16:04
@ghost

ghost commented Nov 11, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting review label Nov 11, 2023
@bedevere-app

bedevere-app Bot commented Nov 11, 2023

Copy link
Copy Markdown

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 skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot mentioned this pull request Nov 11, 2023
3.11.5 regression: StreamWriter.__del__ fails if event loop is already closed #109538
Closed
@Eclips4 Eclips4 added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Nov 11, 2023
@bedevere-app

bedevere-app Bot commented Nov 11, 2023

Copy link
Copy Markdown

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 skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood added type-bug An unexpected behavior, bug, or error topic-asyncio labels Nov 11, 2023
@dpr-0
dpr-0 force-pushed the closed-loop-runtime-error branch 2 times, most recently from 5dd6f38 to a0dbe8b Compare November 11, 2023 17:08
hugovk
hugovk reviewed Nov 11, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-11-11-16-42-48.gh-issue-109538.cMG5ux.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@dpr-0
dpr-0 force-pushed the closed-loop-runtime-error branch from 2f72974 to 075a7fd Compare November 12, 2023 01:59
dpr-0 and others added 3 commits November 12, 2023 10:02
@dpr-0
pythongh-109538: Catch closed loop runtime error and issue warning
354780a
@blurb-it @dpr-0
📜🤖 Added by blurb_it.
0a1c4ef
@dpr-0 @hugovk
Update Misc/NEWS.d/next/Library/2023-11-11-16-42-48.gh-issue-109538.c…
7461e4e 
…MG5ux.rst

Co-authored-by: Hugo van Kemenade <[email protected]>
@dpr-0
dpr-0 force-pushed the closed-loop-runtime-error branch from 075a7fd to 7461e4e Compare November 12, 2023 02:02
@hugovk

hugovk commented Nov 12, 2023

Copy link
Copy Markdown
Member

Tip:

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide
dpr-0 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dpr-0

dpr-0 commented Nov 12, 2023

Copy link
Copy Markdown
Contributor Author

Tip:

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide

I see, thanks for letting me know.
hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum
gvanrossum reviewed Nov 13, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

Sorry to be arguing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/asyncio/streams.py Outdated

try:
self.close()
except RuntimeError:

@gvanrossum gvanrossum Nov 13, 2023

Copy link
Copy Markdown
Member

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.

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()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
dpr-0 and willingc reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@dpr-0
use checking condition instead of try catch
54106eb
gvanrossum
gvanrossum approved these changes Nov 14, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

Have you confirmed that this version solves the problem in your code? How hard would it be to write a unit test to confirm that this warning is reported?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Nov 14, 2023
willingc
willingc reviewed Nov 14, 2023
View reviewed changes

@willingc willingc left a comment

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
dpr-0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@miss-islington-app

miss-islington-app Bot commented Nov 15, 2023

Copy link
Copy Markdown

Thanks @dpr-0 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 15, 2023
@miss-islington-app

miss-islington-app Bot commented Nov 15, 2023

Copy link
Copy Markdown

Sorry, @dpr-0 and @gvanrossum, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker e0f512797596282bff63260f8102592aad37cdf1 3.12
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned gvanrossum Nov 15, 2023
@miss-islington-app

miss-islington-app Bot commented Nov 15, 2023

Copy link
Copy Markdown

Sorry, @dpr-0 and @gvanrossum, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker e0f512797596282bff63260f8102592aad37cdf1 3.11
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dpr-0
dpr-0 deleted the closed-loop-runtime-error branch November 15, 2023 01:28
@gvanrossum

gvanrossum commented Nov 15, 2023

Copy link
Copy Markdown
Member

@dpr-0 Are you interested in attempting the backports?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dpr-0

dpr-0 commented Nov 15, 2023

Copy link
Copy Markdown
Contributor Author

@dpr-0 Are you interested in attempting the backports?

Yes, any brief backport steps or I just cherry-pick and create two PRs?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Nov 15, 2023

Copy link
Copy Markdown
Member

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.
dpr-0 and willingc reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 reviewed Nov 15, 2023
View reviewed changes
Comment thread Lib/test/test_asyncio/test_streams.py
try:
self.loop.run_until_complete(inner(httpd))
# This exception is caused by `self.loop.stop()` as expected.
except RuntimeError:

@kumaraditya303 kumaraditya303 Nov 15, 2023

Copy link
Copy Markdown
Contributor

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.

If this exception is expected then why not use self.assertRaises etc?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@terryjreedy terryjreedy Nov 17, 2023

Copy link
Copy Markdown
Member

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.

except allows/tolerates an exception, assert requires one and will fail if it does not happen because of other improvements.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dpr-0

dpr-0 commented Nov 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@kumaraditya303 @gvanrossum I am planning to backport this PR and issue a new PR to improve test case.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
dpr-0 added a commit to dpr-0/cpython that referenced this pull request Nov 16, 2023
@dpr-0 @hugovk
pythongh-109538: Avoid RuntimeError when StreamWriter is deleted with…
d805ae3 
… closed loop (python#111983)

Issue a ResourceWarning instead.

Co-authored-by: Hugo van Kemenade <[email protected]>
(cherry picked from commit e0f5127)
@bedevere-app

bedevere-app Bot commented Nov 16, 2023

Copy link
Copy Markdown

GH-112141 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.11 only security fixes label Nov 16, 2023
@dpr-0 dpr-0 mentioned this pull request Nov 16, 2023
[3.12] gh-109538: Catch closed loop runtime error and issue warning (GH-111983) #112142
Merged
@bedevere-app

bedevere-app Bot commented Nov 16, 2023

Copy link
Copy Markdown

GH-112142 is a backport of this pull request to the 3.12 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Nov 16, 2023
dpr-0 added a commit to dpr-0/cpython that referenced this pull request Nov 16, 2023
@dpr-0 @hugovk
[3.12] pythongh-109538: Avoid RuntimeError when StreamWriter is delet…
5a71703 
…ed with closed loop (pythonGH-111983)

Issue a ResourceWarning instead.

(cherry picked from commit e0f5127)
pythongh-109538: Avoid RuntimeError when StreamWriter is deleted with closed loop (python#111983)

Issue a ResourceWarning instead.

Co-authored-by: Hugo van Kemenade <[email protected]>
(cherry picked from commit e0f5127)
@bedevere-app

bedevere-app Bot commented Nov 16, 2023

Copy link
Copy Markdown

GH-112141 is a backport of this pull request to the 3.12 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Nov 16, 2023

Copy link
Copy Markdown

GH-112141 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Nov 16, 2023

Copy link
Copy Markdown

GH-112142 is a backport of this pull request to the 3.12 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dpr-0 dpr-0 mentioned this pull request Nov 16, 2023
[3.11] gh-109538: Catch closed loop runtime error and issue warning (GH-111983) #112141
Merged
gvanrossum pushed a commit that referenced this pull request Nov 16, 2023
@dpr-0
[3.12] gh-109538: Catch closed loop runtime error and issue warning (G…
2ef3676 
…H-111983) (#112142)

* [3.12] gh-109538: Avoid RuntimeError when StreamWriter is deleted with closed loop (GH-111983)

Issue a ResourceWarning instead.

(cherry picked from commit e0f5127)
gh-109538: Avoid RuntimeError when StreamWriter is deleted with closed loop (#111983)

Issue a ResourceWarning instead.

Co-authored-by: Hugo van Kemenade <[email protected]>
(cherry picked from commit e0f5127)

* Fix missing warnings import
@dpr-0 dpr-0 mentioned this pull request Nov 17, 2023
Improve test case test_loop_is_closed_resource_warnings #112186
Closed
gvanrossum pushed a commit that referenced this pull request Nov 20, 2023
@dpr-0 @hugovk
[3.11] gh-109538: Catch closed loop runtime error and issue warning (G…
1397505 
…H-111983) (#112141)

Issue a ResourceWarning instead.

Co-authored-by: Hugo van Kemenade <[email protected]>
(cherry picked from commit e0f5127)
@gschaffner gschaffner mentioned this pull request Jan 17, 2024
BaseSubprocessTransport.__del__ fails if the event loop is already closed, which can leak an orphan process #114177
Closed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@dpr-0 @hugovk @aisk
pythongh-109538: Avoid RuntimeError when StreamWriter is deleted with…
ebd457a 
… closed loop (python#111983)

Issue a ResourceWarning instead.

Co-authored-by: Hugo van Kemenade <[email protected]>
@gvanrossum gvanrossum mentioned this pull request Feb 19, 2024
Abandoned StreamWriter isn't reliably detected #114914
Closed
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@dpr-0 @hugovk @Glyphack
pythongh-109538: Avoid RuntimeError when StreamWriter is deleted with…
a6f4795 
… closed loop (python#111983)

Issue a ResourceWarning instead.

Co-authored-by: Hugo van Kemenade <[email protected]>
@bonzini bonzini mentioned this pull request Feb 3, 2025
Feature/question: Only show the verbose output of failed tests mesonbuild/meson#13322
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hugovk hugovk hugovk left review comments
@willingc willingc willingc left review comments
@terryjreedy terryjreedy terryjreedy left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@gvanrossum gvanrossum gvanrossum approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gvanrossum gvanrossum

Labels

topic-asyncio type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@dpr-0 @hugovk @gvanrossum @willingc @terryjreedy @kumaraditya303 @AlexWaygood @Eclips4
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.