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-114914: Avoid keeping dead StreamWriter alive by CendioOssman · Pull Request #115661 · python/cpython

Conversation

@CendioOssman

@CendioOssman CendioOssman commented Feb 19, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

In some cases we might cause a StreamWriter to stay alive even when the application has dropped all references to it. This prevents us from doing automatical cleanup, and complaining that the StreamWriter wasn't properly closed.

Fortunately, the extra reference was never actually used for anything so we can just drop it.

  • Issue: Abandoned StreamWriter isn't reliably detected #114914

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@CendioOssman
pythongh-114914: Avoid keeping dead StreamWriter alive
ed9bca5 
In some cases we might cause a StreamWriter to stay alive even when the
application has dropped all references to it. This prevents us from
doing automatical cleanup, and complaining that the StreamWriter wasn't
properly closed.

Fortunately, the extra reference was never actually used for anything so
we can just drop it.
@CendioOssman
CendioOssman requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners February 19, 2024 14:53
@bedevere-app bedevere-app Bot added the awaiting review label Feb 19, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 19, 2024
Abandoned StreamWriter isn't reliably detected #114914
Closed
gvanrossum
gvanrossum reviewed Feb 19, 2024
View reviewed changes
Comment thread Lib/test/test_asyncio/test_streams.py Outdated
addr = srv.sockets[0].getsockname()
with socket.create_connection(addr):
# Give the loop some time to notice the connection
await asyncio.sleep(0.1)

@gvanrossum gvanrossum Feb 19, 2024

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.

Like in the other PR, I believe that one or two sleep(0) should deterministically reach the state you're waiting for?

Sorry, something went wrong.

Uh oh!

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

@CendioOssman CendioOssman Feb 27, 2024

Copy link
Copy Markdown
Contributor Author

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.

Three, actually.

But I'm cautious about this change. Right now, it seems to require three. But someone might change something so that tomorrow this requires four. At which point this test will start reporting "ok" even if everything is broken. :/

Sorry, something went wrong.

Uh oh!

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

@CendioOssman CendioOssman Feb 27, 2024

Copy link
Copy Markdown
Contributor Author

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 think adding a future to wait on could give something more reliable in this specific case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_asyncio/test_streams.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@CendioOssman
More reliable waiting for connection
cd0b45d
@CendioOssman
CendioOssman requested a review from gvanrossum February 27, 2024 14:54
gvanrossum
gvanrossum approved these changes Feb 28, 2024
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.

Looks great. Will merge. Thanks for finding and fixing this!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_asyncio/test_streams.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 28, 2024
@gvanrossum
gvanrossum merged commit a355f60 into python:main Feb 28, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 28, 2024
@JelleZijlstra JelleZijlstra mentioned this pull request Feb 28, 2024
macos-13 builds are failing #116026
Closed
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Feb 28, 2024
@CendioOssman @gvanrossum
pythongh-114914: Avoid keeping dead StreamWriter alive (python#115661)
9359f94 
In some cases we might cause a StreamWriter to stay alive even when the
application has dropped all references to it. This prevents us from
doing automatical cleanup, and complaining that the StreamWriter wasn't
properly closed.

Fortunately, the extra reference was never actually used for anything so
we can just drop it.
@CendioOssman
CendioOssman deleted the abandon_stream branch February 28, 2024 10:51
@sobolevn

sobolevn commented Feb 29, 2024

Copy link
Copy Markdown
Member

Please, see #116112
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@CendioOssman @woodruffw
pythongh-114914: Avoid keeping dead StreamWriter alive (python#115661)
f07fa55 
In some cases we might cause a StreamWriter to stay alive even when the
application has dropped all references to it. This prevents us from
doing automatical cleanup, and complaining that the StreamWriter wasn't
properly closed.

Fortunately, the extra reference was never actually used for anything so
we can just drop it.
@sobolevn sobolevn mentioned this pull request Mar 5, 2024
gh-116112: Fix ResourceWarning in test_asyncio.test_stream #116371
Merged
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@CendioOssman @adorilson
pythongh-114914: Avoid keeping dead StreamWriter alive (python#115661)
60b9a78 
In some cases we might cause a StreamWriter to stay alive even when the
application has dropped all references to it. This prevents us from
doing automatical cleanup, and complaining that the StreamWriter wasn't
properly closed.

Fortunately, the extra reference was never actually used for anything so
we can just drop it.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@CendioOssman @diegorusso
pythongh-114914: Avoid keeping dead StreamWriter alive (python#115661)
4ba54cc 
In some cases we might cause a StreamWriter to stay alive even when the
application has dropped all references to it. This prevents us from
doing automatical cleanup, and complaining that the StreamWriter wasn't
properly closed.

Fortunately, the extra reference was never actually used for anything so
we can just drop it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@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
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner
@willingc willingc Awaiting requested review from willingc willingc is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

3 participants

@CendioOssman @sobolevn @gvanrossum
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.