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-88968: Reject socket that is already used as a transport by gvanrossum · Pull Request #98010 · python/cpython

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Oct 7, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Add a check to _make_socket_transport() that a socket passed in isn't already in use by a transport.
If it was, this would indicate that someone had closed the socket but not the transport (or the writer built on top of it).

  • Issue: asyncio.StreamReader.read hangs for reused socket file descriptors when asyncio.StreamWriter.close() is not called #88968

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gvanrossum added 3 commits October 6, 2022 17:49
@gvanrossum
In _make_socket_transport, ensure the fd isn't already used by a tran…
6783dda 
…sport
@gvanrossum
Make existing tests pass
fda2111
@gvanrossum
Test that _ensure_fd_no_transport is called
6224b18
@gvanrossum
gvanrossum requested review from 1st1 and asvetlov as code owners October 7, 2022 00:57
@bedevere-bot bedevere-bot added the awaiting core review label Oct 7, 2022
@gvanrossum
gvanrossum requested a review from kumaraditya303 October 7, 2022 00:57
@kumaraditya303

kumaraditya303 commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

I think SSL socket and datagram have the same issue though I haven't verified this. Perhaps also add this check to _make_datagram_transport and _make_ssl_transport?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
Same check for _make_ssl_transport and _make_datagram_transport
1c348f1
@gvanrossum

gvanrossum commented Oct 7, 2022

Copy link
Copy Markdown
Member Author

Done.

I worried a bit about how thoughly to test this, but then realized that this code is tested every time a transport is created using one of these functions -- even if the user didn't pass sock=..., since it is called if the socket was created by the higher-level code (e.g. create_connection) too.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 approved these changes Oct 7, 2022
View reviewed changes

@kumaraditya303 kumaraditya303 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.

LGTM, I think news can be skipped as the change is very small, leaving up to you to decide.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kumaraditya303 kumaraditya303 added the topic-asyncio label Oct 7, 2022
@gvanrossum gvanrossum added the skip news label Oct 7, 2022
@gvanrossum
gvanrossum merged commit c062764 into python:main Oct 7, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Oct 7, 2022
@gvanrossum
gvanrossum deleted the reject-sock-reuse branch October 7, 2022 19:57
carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
27a4980 
* main: (38 commits)
  pythongh-92886: make test_ast pass with -O (assertions off) (pythonGH-98058)
  pythongh-92886: make test_coroutines pass with -O (assertions off) (pythonGH-98060)
  pythongh-57179: Add note on symlinks for os.walk (python#94799)
  pythongh-94808: Fix regex on exotic platforms (python#98036)
  pythongh-90085: Remove vestigial -t and -c timeit options (python#94941)
  pythonGH-83901: Improve Signature.bind error message for missing keyword-only params (python#95347)
  pythongh-61105: Add default param, note on using cookiejar subclass (python#95427)
  pythongh-96288: Add a sentence to `os.mkdir`'s docstring. (python#96271)
  pythongh-96073: fix backticks in NEWS entry (pythonGH-98056)
  pythongh-92886: [clinic.py] raise exception on invalid input instead of assertion (pythonGH-98051)
  pythongh-97997: Add col_offset field to tokenizer and use that for AST nodes (python#98000)
  pythonGH-88968: Reject socket that is already used as a transport (python#98010)
  pythongh-96346: Use double caching for re._compile() (python#96347)
  pythongh-91708: Revert params note in urllib.parse.urlparse table (python#96699)
  pythongh-96265: Fix some formatting in faq/design.rst (python#96924)
  pythongh-73196: Add namespace/scope clarification for inheritance section (python#92840)
  pythongh-97646: Change `.js` and `.mjs` files mimetype to conform to RFC 9239 (python#97934)
  pythongh-97923: Always run Ubuntu SSL tests with others in CI (python#97940)
  pythongh-97956: Mention `generate_global_objects.py` in `AC How-To` (python#97957)
  pythongh-96959: Update HTTP links which are redirected to HTTPS (python#98039)
  ...
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@gvanrossum @mpage
pythonGH-88968: Reject socket that is already used as a transport (py…
8cb415b 
…thon#98010)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 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

No one assigned

Labels

skip news topic-asyncio

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

@gvanrossum @kumaraditya303 @bedevere-bot
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 01402503 — 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.